Daemon
Daemon copied to clipboard
Honor nan and inf
This changes the compiler flags so that gcc and clang don't assume NaN and Inf cannot exist
This change applies to both Daemon and Unvanquished, and fixes https://github.com/Unvanquished/Unvanquished/issues/2299 and fixes https://github.com/Unvanquished/Unvanquished/issues/2311.
This also includes a tiny logging change.
I think this deserves more explanation. How does it fix those two issues?
It fixes the https://github.com/Unvanquished/Unvanquished/issues/2299 issue because now if (nan > x) isn't true half of the time like it was before. Instead it is always false so the collision condition will never be entered.
It fixes the https://github.com/Unvanquished/Unvanquished/issues/2311 issue because most of the nonsensical results tend to have to do with fast-math doing garbage with nan and infinities, which is now disabled. The rest of fast-math is mostly fine, and acceptable in our case.
Does this have performance implications? I think these explanations should be in the commit messages. Otherwise LGTM.
@necessarily-equal can you edit the commit message with that comment? Then we can merge it.
It fixes the Unvanquished/Unvanquished#2299 issue because now
if (nan > x)isn't true half of the time like it was before. Instead it is always false so the collision condition will never be entered.It fixes the Unvanquished/Unvanquished#2311 issue because most of the nonsensical results tend to have to do with fast-math doing garbage with nan and infinities, which is now disabled. The rest of fast-math is mostly fine, and acceptable in our case.
#2299 was fixed so it seems we don't need it.
Ah! Do we have remaining bugs that are known to come from nan/inf problems? #2311 seems to just be a general topic on the subject of potential problems. If no we can close this MR.
We now know how to detect floating-point overflows (#770 proposes making this more convenient). I don't believe there are other bugs associated with that. I cleaned up sgame to not regularly have overflows; cgame and engine have yet to be examined.
Ok then, so we better close this PR ! 🙂️
I still believe it's better to not have wildly inconsistent behaviours lurking around; even if we do try to make them not trigger, a wrong value may appear in an unexpected situation and break things, like it did with the train bug: we didn't test trains with such a high speed that were stopped and started so often.
I just don't think we will consistently catch every possible bugs that could trigger with the unsafe optimisation that this disable.
One middle ground would be to disable this optimisation just for sgame, or to benchmark it a bit to see if there is an actual performance difference (on other programs the answer was "no", for the daemon renderer it could.)
I think @Sweet and @bmorel expressed they dislike -ffast-math on IRC:
Sweet:
wait, it may be equal to itself with fast-math i'm even more scared whenever i think about this […] removing divisions by 0 gives good luck in the future
bmorel:
what, fast-math is still used around? fast-math does not fix bugs, it only causes them * bmorel remembers other game, that fast-math made crash for perfectly good code
I still stand that we should at the very least disable the -ffinite-math-only part of -ffast-math, as in this PR. because it gives nonsensical and unpredictable results.
I've also reworded the commit messages a bit.
Is there any evidence that the mover bug is actually related to a nan > x type condition? It could be one of those bugs that just randomly triggers or not depending on the luck of how exactly something was compiled (we've had some GLSL issues like that).
What's with the random revert commit?