all-is-cubes icon indicating copy to clipboard operation
all-is-cubes copied to clipboard

Fix signed zero bugs & refine numeric restrictions

Open kpreid opened this issue 1 year ago • 8 comments

Problem

There's a correctness bug lurking in our use of ordered_float::NotNan: positive and negative zero, -0.0 and 0.0, compare equal, and this can cause different outcomes of further operations (notably division or reciprocal). This means that using them as cache keys is invalid except where the sign is known to make no difference.

Closely related, but not strictly a bug: there are cases where NotNan is less restrictive than we would benefit from; for example, Rgba allows an alpha less than 0 or greater than 1, but this is meaningless.

Solution

All of our uses of NotNan (we don't have any uses that aren't for cache-key/change-detection purposes, I believe) should be replaced with wrappers that are either more restrictive or consider positive and negative zero distinct, as is appropriate for the application.

If some of the resulting wrappers seem more generally applicable, it might make sense to contribute them back to ordered_float.

kpreid avatar Oct 03 '24 01:10 kpreid

Commit 20cd9d6bef132120a70e0b9a92e945aa98fb24b1 replaces all uses of NotNan within color types with the new numeric wrapper PositiveSign.

Next steps:

  • There's a lot of places where what is needed is a 0-1 restricted type: alpha, reflectance RGB, bloom intensity option, progress bar fill. I haven’t thought of a good name for it.
  • Replace NotNan with PositiveSign in uses like the view distance in graphics options. (That’s separately clamped to a nonzero minimum value, so it's not strictly required, but we might as well.)

kpreid avatar Oct 19 '24 05:10 kpreid

As of commit ef1a45e9e030a8c03a23541c67e8e4687ecf7c09, there is a ZeroOne type, and it is used by Rgba for alpha and GraphicsOptions for bloom blending.

kpreid avatar Oct 21 '24 20:10 kpreid

As of commit 30ceaed2035745b3dbbec06b13f70c9cbff85e01, GraphicsOptions no longer uses NotNan.

kpreid avatar Oct 29 '24 17:10 kpreid

Remaining non-incidental uses of NotNan:

  • Points/vectors (can't be PositiveSign since they can be negative):
    • Spawn::eye_position
    • Spawn::look_direction
    • SpacePhysics::gravity
  • Fluff::BlockImpact::velocity (can be PositiveSign) (update: done in commit e6eb4a29e719e14d6119faf9a8ced2fa4c623114)
  • FpsCounter::average_frame_time_seconds (can be PositiveSign) (update: done in commit fc3f723b0459772d32cbd408d6f7a337ae3e86b1)
  • Conversion methods on color types

kpreid avatar Oct 30 '24 04:10 kpreid

Whoops, the design of PositiveSign is not consistent. In FP arithmetic, 0.0 * INFINITY is NaN, so as long as PositiveSign includes INFINITY, it isn’t closed under multiplication.

My first thought is to define multiplication such that 0.0 * INFINITY == 0.0, because in the actual applications for PositiveSign, positive infinity should almost always appear only as essentially “would be finite but it is too large to be represented” — we don’t generally get an infinity correctly from a division by zero. That would make PositiveSign no longer merely a restriction, but it might be the least bad thing to do. Redefining arithmetic should be done with care, though.

kpreid avatar Nov 25 '24 05:11 kpreid

Implemented 0.0 * INFINITY == 0.0 in b24f917f923bf488685ae1c1d1de46b1454b22bf.

kpreid avatar Nov 25 '24 16:11 kpreid

Aab is not using NotNan but it enforces its own lower ≤ upper restriction, and has no consideration of negative zero, so it needs work like the other types here.

kpreid avatar Dec 28 '24 07:12 kpreid

Commit eefacde9a23fd384445c40c6cfaed097b7ac470b adds NotNan to Body position and velocity. It’s not the right type; besides signed zero issues, this use case kind of wants to prohibit infinities too. However, it was necessary in order to interact with Aab well.

kpreid avatar Jan 30 '25 17:01 kpreid