Daemon icon indicating copy to clipboard operation
Daemon copied to clipboard

Honor nan and inf

Open necessarily-equal opened this issue 2 years ago • 12 comments

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.

necessarily-equal avatar Dec 11 '22 09:12 necessarily-equal

I think this deserves more explanation. How does it fix those two issues?

DolceTriade avatar Dec 12 '22 03:12 DolceTriade

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.

necessarily-equal avatar Dec 12 '22 10:12 necessarily-equal

Does this have performance implications? I think these explanations should be in the commit messages. Otherwise LGTM.

DolceTriade avatar Dec 12 '22 16:12 DolceTriade

@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.

illwieckz avatar Jan 17 '23 10:01 illwieckz

#2299 was fixed so it seems we don't need it.

slipher avatar Jan 17 '23 10:01 slipher

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.

illwieckz avatar Jan 17 '23 11:01 illwieckz

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.

slipher avatar Jan 18 '23 03:01 slipher

Ok then, so we better close this PR ! 🙂️

illwieckz avatar Jan 18 '23 10:01 illwieckz

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.)

necessarily-equal avatar Jan 18 '23 13:01 necessarily-equal

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.

necessarily-equal avatar Feb 20 '23 00:02 necessarily-equal

I've also reworded the commit messages a bit.

necessarily-equal avatar Feb 20 '23 00:02 necessarily-equal

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?

slipher avatar Feb 20 '23 02:02 slipher