Fix signed zero bugs & refine numeric restrictions
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.
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
NotNanwithPositiveSignin 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.)
As of commit ef1a45e9e030a8c03a23541c67e8e4687ecf7c09, there is a ZeroOne type, and it is used by Rgba for alpha and GraphicsOptions for bloom blending.
As of commit 30ceaed2035745b3dbbec06b13f70c9cbff85e01, GraphicsOptions no longer uses NotNan.
Remaining non-incidental uses of NotNan:
- Points/vectors (can't be
PositiveSignsince they can be negative):Spawn::eye_positionSpawn::look_directionSpacePhysics::gravity
Fluff::BlockImpact::velocity(can bePositiveSign) (update: done in commit e6eb4a29e719e14d6119faf9a8ced2fa4c623114)FpsCounter::average_frame_time_seconds(can bePositiveSign) (update: done in commit fc3f723b0459772d32cbd408d6f7a337ae3e86b1)- Conversion methods on color types
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.
Implemented 0.0 * INFINITY == 0.0 in b24f917f923bf488685ae1c1d1de46b1454b22bf.
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.
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.