Fixing pesky divide-by-zero errors

[buckmaster]

@Azaezel has been running into cases where divide-by-zero errors, particularly on vectors, are causing errors. Az reckons this is behind some decal stretching issues (653 and 1160 suspected). We had a bit of a chat about this today and haven't reached an agreement, so we'd like to enlist YOUR OPINIONS on the issue.

Basically, the question is this. Should we make the vector division operations safe by default, or should we require that you check before each call that might end up doing a divide by zero?

Here's an example of the first approach:

 

inline Point3F Point3F::operator/(const Point3F &_vec) const
{
   Point3F tVec = _vec;
   // new code ->
   if (tVec.x == 0.0f) tVec.x = POINT_EPSILON;
   if (tVec.y == 0.0f) tVec.y = POINT_EPSILON;
   if (tVec.z == 0.0f) tVec.z = POINT_EPSILON;
   // <- new code ends
   return Point3F(x / tVec.x, y / tVec.y, z / tVec.z);
}

 

I.e. inside the division operator, we check if any of the denominators are zero, and if so, set them to some small value to avoid the divide-by-zero error. This means you can always use point1 / point2 without worrying about whether point2 is safe to divide by. It's more foolproof and is fixing the bugs in a single place, rather than having to fix every call site where vector division happens.

The second option looks more like this:

 

Point3F x = getPoint(), y = getAnotherPoint();
if (y.divisorSafe())
{
   return x / y;
}
else
{
   return z; // something else appropriate in this situation
}

 

In this case, divisorSafe would be a new method that essentially returns true if none of the vector's components are 0. We require that every user who might want to divide by a vector should check this, unless for example they know the vector will never have 0 components. This approach means not changing the implementation of Point3F::operator/, except for adding a lot more assertions like this one so that div-by-zero errors were caught as soon as they happened, instead of introducing NaNs and Infs to cause mischief at some later point in the program. Note that this is kind of the way / works for floats already (i.e. it's unsafe).

So - thoughts? Az has characterised the former option as 'speed of implementation' (not having to change all the call sites, not having to worry about whether your division is safe), and the latter as 'speed of execution' (no branching inside operator/).

[Caleb]

The only benefit I can see of the latter is possible debugging. Adding a new method would use the same number of checks as making the original safe, but would bulk up code that uses division frequently. I'd say just make vector division safe, but add asserts so we at least know we tried dividing by zero when debugging. Now everyone is happy!

[GuyA]

Is it possible to leave the division function as it is now, but add an assert in debug mode for division by zero?

Doing that won't affect performance of release builds at all, but will highlight potential problems when run in debug mode.

[andrewmac]

There are a few cases throughout the engine where slower safe methods are implemented, usually following the pattern of safe(). For that reason, and performance concerns, I'd lean towards safeDivide() as the best option.

[Azaezel]

Is it possible to leave the division function as it is now, but add an assert in debug mode for division by zero?

Doing that won't affect performance of release builds at all, but will highlight potential problems when run in debug mode.

 

Entirely possible. rolling https://github.com/MusicMonkey5555/Torque3D/commit/c7e0d83587ebd18e8b870669db8e17aa69a5b6ea in and getting application-halt every 5 seconds due to things like https://github.com/GarageGames/Torque3D/blob/c152ae86f3f09b3c1d736954b352723d274582f1/Engine/source/ts/tsCollision.cpp#L1633 was in fact the primary motivation for bringing this up for discussion in the first place. https://github.com/Azaezel/Torque3D/commit/1c52b777ce12b35dec36ed6f860368fa1e555c7d would be the full list of spots to touch either way.

Edit: Should note there are two additional options in terms of 'do x or do y':

1) add a compiler flag to let folks choose for themselves between resolutions

2) more problem specific, so doesn't really address the underlying philosophical/guideline-in-future query would be to add a point-epsilon rather than branching.

This is the *type* of problem that has cropped up in the past, and will crop up in the future, so best get regs sorted as far as preferences.

[Timmy]

Is it possible to leave the division function as it is now, but add an assert in debug mode for division by zero?

Doing that won't affect performance of release builds at all, but will highlight potential problems when run in debug mode.

 

x2

[buckmaster]

This approach means not changing the implementation of Point3F::operator/, except for adding a lot more assertions like this one so that div-by-zero errors were caught as soon as they happened

Yep, the asserts would go in if we didn't change the division functions.

@Azaezel I'm not in favour of either of those solutions. 1) adds complexity, and 2) is just really ad-hoc and might cause other behavioural oddities - though to be fair, probably nothing more odd than regular floating-point behaviour.

@Caleb the idea would be that in some cases, you know you can skip the checks (for example, if you're dividing by some constant point you know, or if the maths used to calculate a point is such that it can't result in 0, or if you want to do the check once then divide by the same point several times). These cases are all hypothetical, but in each of them you either don't need to check, or only need to check once.

[JeffR]

Is it possible to leave the division function as it is now, but add an assert in debug mode for division by zero?

Doing that won't affect performance of release builds at all, but will highlight potential problems when run in debug mode.

 

I think I prefer this route as well.

Keeps things light for release, but in debug you can see where your math is nipping you in the butt. In both versions, the math is the same, so it's unlikely we'd get different results between versions, we merely get deeper notifications in debug.

tl;dr:

Keep the deep methods fast, if theoretically unsafe from endusers. HOWEVER, in Debug, we should be very consistent with informing them that they have utilized things incorrectly. This way they know there is an issue and can follow it back to correct it with minimal work.

[rlranft]

Take the path of least resistance - I think it's "best practice" for programmers to just avoid divide by zero themselves. In any case where then engine can do it without help from a user it should be avoided, but otherwise the onus should be on the developer.