Fix #128102: Integer Math division has only float precision #128123

Charlie Jolly · 2024-09-25T15:59:59+02:00

Charlie Jolly commented

2024-09-25 15:59:59 +02:00

The additional division modes were implemented with float
division then rounding to match float > int conversion in
existing float to int node.

As pointed out in the bug report the precision here is limited.

This patch replaces the float division with integer math which
increases the precision to much higher numbers.
Divide Round by due to the way it is calculated has less precision
than Divide Floor and Divide Ceil.

The additional division modes were implemented with float division then rounding to match float > int conversion in existing float to int node. As pointed out in the bug report the precision here is limited. This patch replaces the float division with integer math which increases the precision to much higher numbers. Divide Round by due to the way it is calculated has less precision than Divide Floor and Divide Ceil.

gn_integer_math-div-precision.blend

961 KiB

Charlie Jolly added 1 commit 2024-09-25 16:00:10 +02:00

Fix #128102 : Integer Math division has only float precision f23ec905d3

The additional division modes were implemented with float
division then rounding to match float > int conversion in
existing float to int node.

As pointed out in the bug report the precision here is limited.

This patch replaces the float division with integer math which
increases the precision to much higher numbers.
Divide Round by due to the way it is calculated has less precision
than Divide Floor and Divide Ceil.

Charlie Jolly added this to the Nodes & Physics project 2024-09-25 16:01:24 +02:00

Charlie Jolly requested review from Jacques Lucke 2024-09-25 16:02:33 +02:00

Jacques Lucke requested changes 2024-09-25 17:53:14 +02:00

Dismissed

source/blender/nodes/function/nodes/node_fn_integer_math.cc Outdated

						
				@ -117,3 +117,3 @@

				      "Multiply", [](int a, int b) { return a * b; }, exec_preset);

				  static auto divide_fn = mf::build::SI2_SO<int, int, int>(

				      "Divide", [](int a, int b) { return math::safe_divide(a, b); }, exec_preset);

				      "Divide", [](int a, int b) { return (b != 0) ? a / b : 0; }, exec_preset);

Jacques Lucke commented

2024-09-25 17:50:24 +02:00

Isn't this the same as the math::safe_divide?

Isn't this the same as the `math::safe_divide`?

Charlie Jolly commented

2024-09-25 18:14:32 +02:00

My mistake, I copied the function in full for testing.

CharlieJolly marked this conversation as resolved

source/blender/nodes/function/nodes/node_fn_integer_math.cc Outdated

						
				@ -130,1 +130,3 @@

				      [](int a, int b) { return int(math::round(math::safe_divide(float(a), float(b)))); },

				      [](int a, int b) {

				        const int c = abs(b);

				        return (a >= 0) ? math::safe_divide((2 * a + c), (2 * c)) * math::sign(b) :

Jacques Lucke commented

2024-09-25 17:53:12 +02:00

Would be good to extract that into a separate function in blenlib and to add some unit tests for it. (e.g. in BLI_math_base_safe_test.cc)

Where does this formular come from? Shouldn't it be possible to round based on the remainder?

Would be good to extract that into a separate function in blenlib and to add some unit tests for it. (e.g. in `BLI_math_base_safe_test.cc`) Where does this formular come from? Shouldn't it be possible to round based on the remainder?

Charlie Jolly commented

2024-09-25 18:56:55 +02:00

This was derived from divide_round_i but fixed to be safe and handle negative inputs to match float output from math::round(math::safe_divide(a, b)).

I've added a comment to this affect and pulled into a separate function.

MINLINE int divide_round_i(int a, int b)
{
  return (2 * a + b) / (2 * b);
}

/* Derived from `divide_round_i` but fixed to be safe and handle negative inputs. */
static int safe_divide_round_i(const int a, const int b)
{
  const int c = math::abs(b);
  return (a >= 0) ? math::safe_divide((2 * a + c), (2 * c)) * math::sign(b) :
                    -math::safe_divide((2 * -a + c), (2 * c)) * math::sign(b);
}

This was derived from `divide_round_i` but fixed to be safe and handle negative inputs to match float output from `math::round(math::safe_divide(a, b))`. I've added a comment to this affect and pulled into a separate function. ```Cpp MINLINE int divide_round_i(int a, int b) { return (2 * a + b) / (2 * b); } /* Derived from `divide_round_i` but fixed to be safe and handle negative inputs. */ static int safe_divide_round_i(const int a, const int b) { const int c = math::abs(b); return (a >= 0) ? math::safe_divide((2 * a + c), (2 * c)) * math::sign(b) : -math::safe_divide((2 * -a + c), (2 * c)) * math::sign(b); } ```

Charlie Jolly commented

2024-09-25 18:59:33 +02:00

I think it is easier to keep here rather than bloat blenlib but if you want it in blenlib can you advise which file it should go?

Jacques Lucke commented

2024-09-26 12:20:49 +02:00

ok, fine with me, it would just be nice to have some kinds of test here, a regression test can be ok as well though.