Migrated some remaining floats in simulation to integers (and decimals) #10972
Conversation
| @@ -82,8 +82,8 @@ public static WVec LerpQuadratic(WVec a, WVec b, WAngle pitch, int mul, int div) | |||
| return ret; | |||
|
|
|||
| // Add an additional quadratic variation to height | |||
| // Uses fp to avoid integer overflow | |||
| var offset = (int)((float)(b - a).Length * pitch.Tan() * mul * (div - mul) / (1024 * div * div)); | |||
| // Uses long to avoid integer overflow | |||
There was a problem hiding this comment.
This isn't a hot path, so we could probably get away with using decimal here (there's another similar line somewhere using long that could also change) and remove the risk of overflow completely.
There was a problem hiding this comment.
I think I saw WPos use long in a similar case, should that change to decimal as well?
There was a problem hiding this comment.
Yeah, I think that is the line I was referring to.
reaperrr
force-pushed
the
fp-to-int1
branch
2 times, most recently
from
3d8b773
to
6340ca9
Compare
|
Updated. |
reaperrr
changed the title
| @@ -65,7 +65,7 @@ public static WPos LerpQuadratic(WPos a, WPos b, WAngle pitch, int mul, int div) | |||
|
|
|||
| // Add an additional quadratic variation to height | |||
| // Attempts to avoid integer overflow by keeping the intermediate variables reasonably sized | |||
| var offset = (int)(((((((long)(b - a).Length * mul) / div) * (div - mul)) / div) * pitch.Tan()) / 1024); | |||
| var offset = (int)(((((((decimal)(b - a).Length * mul) / div) * (div - mul)) / div) * pitch.Tan()) / 1024); | |||
There was a problem hiding this comment.
You can now drop most of the nested parenthesis, and the comment above will need to be updated (or just remove it).
There was a problem hiding this comment.
Compare with the calculation in WVec.
|
Looks good otherwise 👍 |
|
Updated. |
|
Needs a rebase. |
…ntages Additionally, MinHpPercent should now actually have the desired effect (previously there was not logic attached).
141 / PI ~= 44.88, so 45 should be accurate enough. This should reduce desync risk while improving performance of this calculation.
To reduce desync risk (without introducing overflow risk).
To remove overflow risk.
|
Rebased. |
|
Thanks! |
Partially adresses #10845.