feat(math): Route game logic math through WWMath with 3-mode deterministic support - #2670
feat(math): Route game logic math through WWMath with 3-mode deterministic support#2670Okladnoj wants to merge 95 commits into
Conversation
|
| Filename | Overview |
|---|---|
| Core/Libraries/Include/Lib/BaseDefines.h | Adds the shared math-mode defaults, but the default CRC setting still disables the GameMath path. |
| Core/GameEngine/Source/Common/Diagnostic/SimulationMathCrc.cpp | Updates the diagnostic CRC path to use WWMath wrappers for the benchmarked math operations. |
| Core/Libraries/Source/WWVegas/WWMath/wwmath.h | Provides the wrapper surface used by the changed math call sites. |
Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
Core/Libraries/Include/Lib/BaseDefines.h:35-36
**Deterministic math disabled**
`RETAIL_COMPATIBLE_CRC` defaults to `1`, so this condition is true even when `gmath.h` is present. That undefines `USE_DETERMINISTIC_MATH`, and the WWMath wrappers compile their CRT branches instead of the GameMath branches. A default non-VC6 build can therefore run the old platform math path and still pass through the new WWMath call sites, so cross-platform simulation can diverge even though GameMath was fetched.
Reviews (11): Last reviewed commit: "Merge remote-tracking branch 'origin/mai..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Here is what replay playback looks like at the moment.
I’m testing this on a separate branch: I slightly adjusted the CI there so I can run Win32 and get access to the game resources. |
854cc7b to
779f714
Compare
|
You did not review the changes you made with AI. It has issues that you should fix before asking it to be reviewed. |
|
This change does too many things. It is better to first consolidate trig and wwmath and maybe other sources of math, before going into gamemath territory. |
4b5675d to
ddea128
Compare
@xezon Hey! I understand your point, but the reason I didn't fully consolidate As we saw in PR #2602, fully removing That's exactly why I chose this "routing" approach for this PR. By keeping the Perhaps the best option would be to test this PR first, and if everything is fine — merge it. And only after that, we can focus on a second PR dedicated purely to the architectural cleanup (removing |
| static WWINLINE float ASinTrig(float x) { return asinf(x); } | ||
| #endif | ||
|
|
||
| // Origin wrappers: replace bare CRT math calls in GameLogic. |
There was a problem hiding this comment.
I am not a fan of the "origin" terminology for these functions. What is this supposed to mean?
There was a problem hiding this comment.
"Origin" means the original EA code called bare CRT functions (sqrt, acos, sinf...). The suffix marks which exact CRT function was used originally: SqrtOrigin(double) = was sqrt(), SqrtfOrigin(float) = was sqrtf(). These are not just type variants — they are different math paths. Will rename to _ convention: Sqrt_Origin, Sqrtf_Origin.
There was a problem hiding this comment.
This is not good naming. They should just be called "Sqrt", "Cos", "Sin", etc.
There was a problem hiding this comment.
Ceil(float) and Floor(float) are original EA code (line 157 in main). They are only used in rendering (visrasterizer.cpp) and Normalize_Angle. Not part of CRC game logic — no need to wrap.
There was a problem hiding this comment.
Keep it simple and consolidate code. No math function duplicates.
| Real x, y, z; | ||
|
|
||
| Real length() const { return (Real)sqrt( x*x + y*y + z*z ); } | ||
| Real length() const { return (Real)Sqrt( x*x + y*y + z*z ); } |
There was a problem hiding this comment.
This is now calling a Sqrt(double). Is this intentional? If yes, why?
There was a problem hiding this comment.
Yes, intentional. Sqrt(double) is a free function from trig.h → WWMath::SqrtOrigin(x). Original EA called bare sqrt(). Coord3D::length() is used in game logic and participates in CRC — must be deterministic.
There was a problem hiding this comment.
And (Real)sqrt( x*x + y*y + z*z ); was calling double sqrt(double) ?
- Merge gmath.h include + USE_DETERMINISTIC_MATH into single __has_include block - Replace all #ifdef/#if defined() with #if USE_DETERMINISTIC_MATH - Remove TheSuperHackers @fix prefix from cmake comment - Expand ODR abbreviation in gamemath.cmake comment - Add blank lines after setFPMode() in benchmark - Fix iters abbreviation in printf - Simplify benchmark: remove replay dependency, auto-trigger at frame 400
- Merge gmath.h include + USE_DETERMINISTIC_MATH into single __has_include block - Replace all #ifdef/#if defined() with #if USE_DETERMINISTIC_MATH - Remove TheSuperHackers @fix prefix from cmake comment - Expand ODR abbreviation in gamemath.cmake comment - Add blank lines after setFPMode() in benchmark - Fix iters abbreviation in printf - Simplify benchmark: remove replay dependency, auto-trigger at frame 400 - Rename WWMath wrappers to Function_Name convention (578 replacements, 79 files)
* feat(deterministic-math): scaffold phase 4 routing Port the first deterministic math batch derived from TheSuperHackers PR TheSuperHackers#2670 with incremental gating and attribution compliance. - add non-MSVC anti-FMA compile flag (-ffp-contract=off) - route trig and sqrt gateways through WWMath wrappers - add gamemath.cmake integration scaffold with deterministic flag - update project rule for upstream PR attribution comments - update lessons learned and May dev diary * fix(headless): stabilize replay simulation on macOS - Override ParticleSystemManagerDummy::update() as no-op to prevent headless replay from executing the full particle update path, which caused EXC_BAD_ACCESS crash at ParticleSystemManager::update()+560 - Route SDL3GameEngine::createRadar() and createParticleSystemManager() to their Dummy counterparts when dummy=true (headless mode), matching upstream Win32GameEngine factory behavior - Guard ParticleSystemManager::update() loop against stale null entries with early continue before sys->update() dispatch - Skip smudge rendering path in headless via m_headless guard in ParticleSystemManager::update() - Add null-file guards in RecorderClass::readNextFrame(), appendNextCommand(), and updatePlayback() for both Generals and ZH to prevent null dereference when playback file is closed mid-loop * fix(replay-headless): harden texture creation flow Guard D3DX8 and DX8 wrapper texture allocation paths when device or caps are unavailable in headless replay windows. Fail texture load tasks safely instead of dereferencing null state. Also harden missing texture fallback handling and record session notes in May diary and lessons. * fix(replay-recording): handle mixed path separators correctly when serializing map name The loop condition checking for path separators was incomplete on Linux/macOS paths: - realMapPathToPortableMapPath() converts platform paths to portable format - Portable paths may contain forward slashes (Linux/macOS standard) - Loop condition find(backslash) never matched forward-slash-only paths - This left newMapName EMPTY when writing replay header - Result: replays stored with corrupted map name field Fix: Check !isEmpty() AND (find(backslash) OR find(forward slash)) - Loop correctly terminates when last token (filename) is reached - Works with both Windows (backslash) and Unix (forward slash) separators - Applies to both GameInfoToAsciiString() and GameInfo::setMap() Test results: - macos_skirmish_1v1.rep: PASS - macos_6p_custom_map_2.rep: PASS (CRC fallback resolves map) - macos_1v1_custom_map_1.rep: CRC mismatch (expected, data incompatible) * fix(replay-mapcache): normalize map cache path and replay map field Fix cross-platform replay/map issues found on macOS:\n- write/read MapCache.ini using portable path join (no literal \ filename)\n- keep replay header path handling for absolute and directory-based -replay inputs\n- add explicit replay CRC mismatch diagnostics for headless runs\n- encode/decode replay map field to preserve special characters in map names\n\nValidation:\n- macOS z_generals build completed successfully\n- replay tests: official/custom map cases load natively; incompatible replay reports frame-0 CRC mismatch * fix(particle-emitter): null-safe strdup in copy constructor ParticleEmitterClass copy constructor called ::_strdup() on NameString and UserString without null checks, causing SIGSEGV when either field was null. Crash observed at: ParticleEmitterClass::Clone() -> copy ctor -> ::_strdup(nullptr) -> strlen(nullptr) -> SIGSEGV (KERN_INVALID_ADDRESS at 0x0) Triggered by W3DGhostObject::snapShot() during normal gameplay. Fix: guard strdup calls with null check before dereferencing. Applied to both GeneralsMD and Generals variants. * docs(replay): add headless testing reference and tech debt notes - HEADLESS_REPLAY_TESTING.md: commands, parameters, output interpretation, platform notes, debug tips (GDB/lldb) for macOS and Linux - REPLAY_MAPCACHE_TECH_DEBT.md: tracked known issues for custom map CRC fallback and (resolved) MapCache.ini backslash filename bug
|
Hi @xezon! I have addressed all your review feedback points and updated the PR. CI Status: To save you from hunting through all the comment threads, here is a consolidated list of the answers and solutions to your review points:
|
There was a problem hiding this comment.
Keep it simple and consolidate code. No math function duplicates.
| static WWINLINE float ASinTrig(float x) { return asinf(x); } | ||
| #endif | ||
|
|
||
| // Origin wrappers: replace bare CRT math calls in GameLogic. |
There was a problem hiding this comment.
This is not good naming. They should just be called "Sqrt", "Cos", "Sin", etc.
| static WWINLINE double PowOrigin(double x, double y) { return pow(x, y); } | ||
| static WWINLINE float PowfOrigin(float x, float y) { return powf(x, y); } | ||
| static WWINLINE double CeilOrigin(double x) { return ceil(x); } | ||
| static WWINLINE float CeilfOrigin(float x) { return ceilf(x); } |
There was a problem hiding this comment.
Ok. Then remove these duplicates and simply call ceil or std::ceil & Co at the non logical critical call sites. This way these extra functions can be removed here.
| static WWINLINE float Atan2fOrigin(float y, float x) { return atan2f(y, x); } | ||
| static WWINLINE double AtanOrigin(double x) { return atan(x); } | ||
| static WWINLINE float AtanfOrigin(float x) { return atanf(x); } | ||
| static WWINLINE double ACosOrigin(double x) { return acos(x); } |
There was a problem hiding this comment.
Then use Atan, Asin, Acos, like the original. No inconsistent casing styles.
| static WWINLINE double AtanOrigin(double x) { return (double)gm_atanf((float)x); } | ||
| static WWINLINE float AtanfOrigin(float x) { return gm_atanf(x); } | ||
| static WWINLINE double ACosOrigin(double x) { return (double)gm_acosf((float)x); } | ||
| static WWINLINE float ACosfOrigin(float x) { return gm_acosf(x); } |
There was a problem hiding this comment.
The reason f suffix math functions exist is for C. C does not support function overloading.
I do not agree with your arguments for dangerous overloads. Overloading is very common in C++ and is desired to call the right function for the right type. Programmer does not need to remember to call f version for floats.
auto f1 = getValue();
auto f2 = acos(f1); // function overload picks the right version for the supported float type| Real x, y, z; | ||
|
|
||
| Real length() const { return (Real)sqrt( x*x + y*y + z*z ); } | ||
| Real length() const { return (Real)Sqrt( x*x + y*y + z*z ); } |
There was a problem hiding this comment.
And (Real)sqrt( x*x + y*y + z*z ); was calling double sqrt(double) ?
| Real Sin(Real x) | ||
| { | ||
| return sinf(x); | ||
| return WWMath::Sin_Trig(x); |
There was a problem hiding this comment.
What is the point of moving the function body to WWMath, when it is just meant to be called through this trig file? Better keep it simple and just do it in here. No trampoline to WWMath.
|
Hi @xezon! Thanks for the detailed review. I agree with some of your points regarding code cleanliness (I will remove the However, there are a couple of critical architectural points concerning the preservation of old replays (suffixes) and determinism ( 1. C++ Overloads vs Explicit types (why suffixes are needed)I want to explain why I had to come to an explicit separation of functions via suffixes instead of using C++ overloads. This is tied to the necessity of preserving 100% backwards compatibility for old builds (VC6 Retail Compatibility). I introduced 3 types of functions because they reflect 3 completely different mathematical paths (math paths) in the original EA engine. Our codebase serves three build modes at once (VC6, Win32, and Deterministic), and if we don't strictly fix the paths, we will lose Retail compatibility on old compilers:
Explicit suffixes strictly lock the original execution path. They guarantee that the exact function intended in the original game is called, avoiding unpredictable compiler behavior during overload resolution. Examples (The mechanics of overload conflicts)Here is, with examples, how the overload mechanism breaks the original branches when compiling under VC6: Example A: Conflicting identical signatures (
C++ overloads only work with different argument types. How is the compiler supposed to know which of the two Example B: Path substitution via typing ( float myVal = 0.5f;
float result = acos(myVal); // In the original, this is a call to <math.h> double acos(double)Since What happens if we introduce the overloads 2. Sqrt(double) in BaseType.h:391
Yes, in the original game it fell back to the system CRT 3. "Trampolines" in Trig.cpp
The fact is that I was acting exactly according to your original task from the previous PR (#2602). I did exactly that. But But I moved the implementation itself to 4. Duplicates (Ceil / Floor)Regarding |
| @@ -0,0 +1,16 @@ | |||
| # FORCE is required to guarantee cross-platform bit-exact determinism. | |||
| # Intrinsics would use platform-specific SIMD, breaking CRC parity between architectures. | |||
| set(GM_ENABLE_INTRINSICS OFF CACHE BOOL "Disable intrinsics for cross-arch determinism" FORCE) | |||
There was a problem hiding this comment.
This shouldn't be needed, only intrinsics that match behaviour with the C functions are used and there are test cases that ensure this holds true.
There was a problem hiding this comment.
GameMath ships with a test for this that compares the intrinsic and none intrinsic versions.
There was a problem hiding this comment.
GameMath has tests that check both code paths.
| // GameMath only provides float-precision functions. All call sites pass float-width | ||
| // values, so the narrowing is lossless in practice. | ||
| #if USE_DETERMINISTIC_MATH | ||
| static WWINLINE double Sqrt_Origin(double x) { return (double)gm_sqrtf((float)x); } |
There was a problem hiding this comment.
Game math provides double versions of all math functions unlike the original math lib you were using so this needs updating.
It is a bit tough to fight through this much AI generated text. Please push the last state of the code and then I can take a look at it in Visual Studio and try to polish it up if it needs polishing. I expect this is faster than chatting about where to go with this. Generally, try to not trust the AI generated code too much. It generates code that is for machines, not humans. |
I wrote every point personally — I only asked AI to format it properly, fix spelling, and translate it into English, exactly like I’m asking now, because my English is not very strong. I personally worked through every point of that long text, so it would be better to read it carefully and understand the reasoning behind it — there is nothing unnecessary there. The main point is that suffixes like In the original project, before deterministic math was introduced, there were places with mixed math inside the game logic that affects the CRC. When If we could simply remove |
@xezon The project’s math was not always written with a clean and transparent architecture — or at least not all parts of it were. Maybe this was even done intentionally to make it harder to reverse-engineer the CRC logic. At the moment, all workflows build successfully, and all replays also play successfully both with deterministic math enabled and disabled. Above, I sent a screenshot of your job, plus one additional replay run that I configured specifically to verify Win32. |
|
Ok fair comments. I was under the impression I was chatting with AI generated text because of all the polished formatting. Can you push the latest state to the branch that you have now? I would like to take a look at it in Visual Studio next. Btw, Replay Check is currently broken. We need to wait until after that is fixed. |
The branch is already up to date — I haven't made any changes since the last push, I was waiting for your feedback. Feel free to take the current branch and work on it in VS. If you need my help — push your changes and I'll pick up from there. Regarding the broken Replay Check — the CI runner has no way to obtain the game data. I solved this by extracting a minimal set of files from the Steam distribution (no textures, audio, or GUI — just enough for replay verification), uploaded them as a release to a private repository ( |
The last push in from 08 May |
This comment was marked as resolved.
This comment was marked as resolved.
|
I've implemented all the changes Bob suggested, but it turned out they weren't enough. Over the past week I've been playing and debugging 2v2v2v2 matches (2 players I've organized all of those fixes into these PRs in my repository:
Could you please take a look at them first? It should make the review easier. I haven't merged these changes into #2670 yet. I'll wait for your feedback before doing that. |
Comment out per-file NO_DEBUG_CRC in ObjectCreationList and PhysicsUpdate so Windows DebugFrame dumps match the macOS build, which defines DEBUG_CRC globally and always emits them. This removes the instrumentation asymmetry that forced stripping Mac-only dump blocks before every cross-platform DebugFrame comparison. Also trim stale patterns from ArchiveCRCLogs.ps1, purge the CRCLogs folder in one bulk delete instead of per-file globbing, and add a -Clean switch to Rebuild.ps1 (cmake --clean-first).
|
So many problems. I suggest to make individual pulls for similar types of fixes and we get them reviewed and merged one by one. |
|
Guys, @bobtista @xezon @OmniBlade — huge thanks for stepping in and doing the review!) |
…d RETAIL_COMPATIBLE_CRC The single-precision (Real) radius math in calcMinRadius/calcRadiusVec is now compiled only when RETAIL_COMPATIBLE_CRC is off. Retail-compatible builds keep the original double-precision math, preserving VC6/retail CRC behavior. Addresses review feedback from @bobtista and @xezon on PR #4.
…overload at full precision Div_FixNaN is a div-by-zero guard, not a NaN fix (x/0 -> Inf, only 0/0 -> NaN), so rename it to Div_Safe across both engines. The double overload no longer demotes the division to float; parity testing (Win 32-bit x86 vs macOS ARM64) confirmed double division is bit-identical on both platforms (SSE2), so the downcast was unnecessary. Addresses review feedback from @bobtista on PR #4.
… gm_pow x87 divergence WWMath::Pow(x, 2) routes squaring through gm_pow (fdlibm), which runs on x87 under _PC_24 on the 32-bit Windows build and diverges from macOS ARM64. Parity testing confirmed Pow at double is not cross-platform bit-identical. Add WWMath::Sqr(float): under USE_DETERMINISTIC_MATH it squares with a plain multiply (deterministic and far cheaper); otherwise it keeps the original Pow(x, 2.0) path. Replaces all Pow(expr, 2) call-sites in PartitionManager (threat/shroud fill), POWTruckAIUpdate and BuildAssistant across both engines.
…erministic-math-v2.2.1-clean
…-clean feat: Clean deterministic math fixes for review (v2.2-clean)
….1-clean feat(determinism): Cross-platform deterministic simulation math and lockstep desync fixes
OmniBlade
left a comment
OmniBlade
left a comment
There was a problem hiding this comment.
I think some reconsideration or at least explanation is needed for the combinatorial explosion of functions we seem to have, several of which look like they will call the same underlying functions in both code paths. I've only commented on the Acos and Asin functions, but the comments there apply to all the transcendental functions in my opinion.
| @@ -0,0 +1,16 @@ | |||
| # FORCE is required to guarantee cross-platform bit-exact determinism. | |||
| # Intrinsics would use platform-specific SIMD, breaking CRC parity between architectures. | |||
| set(GM_ENABLE_INTRINSICS OFF CACHE BOOL "Disable intrinsics for cross-arch determinism" FORCE) | |||
There was a problem hiding this comment.
GameMath has tests that check both code paths.
| offset.normalize(); | ||
| Real theta = atan2(-offset.y, offset.x); | ||
| theta -= (Real)M_PI/2; | ||
| theta -= (Real)WWMATH_HALF_PI; |
There was a problem hiding this comment.
Why is this being refactored to use WWMath, but the calls to transcendental functions not being routed to the WWMath wrappers? If its not involved in deterministic math then the refactor IMO should do all or nothing with regards to using WWMath.
| static WWINLINE float Fabs(float x); | ||
| static WWINLINE double Fabs(double x); | ||
| static WWINLINE float Fabsf(float x); | ||
| static WWINLINE float Fabsf_Legacy(float val); |
There was a problem hiding this comment.
Does the CRT fabsf really return a different value compared to the bit twiddling legacy function? A lot of the content of this refactor is renaming WWMath::Fabs to WWMath::Fabsf_Legacy from what I can see which could be avoided with 1. using overloaded functions and 2. not creating a legacy version if the results compared to the CRT and gm_math functions are the same anyhow.
There was a problem hiding this comment.
If _Legacy variants can be safely removed without breaking VC6 replays then that is fine.
| #endif | ||
| } | ||
|
|
||
| WWINLINE double WWMath::Sqr(float x) |
There was a problem hiding this comment.
This looks like a new function, I'm not entirely convinced there should be a specialised function for taking a float and returning its square as a double.
| #endif | ||
| } | ||
|
|
||
| WWINLINE float WWMath::Sqrt_Legacy(float val) |
There was a problem hiding this comment.
This should just return WWMath::Sqrtf if retail CRC matching isn't needed to reduce code duplication.
The same goes for any of the other inline ASM math functions.
It would be interesting to test if the ASM is needed at all or if the CRT functions could be used and still get the same result as I'm sure the windows CRT falls back to the CPU instructions for functions where the CPU has dedicated support.
| #if USE_DETERMINISTIC_MATH | ||
| return gm_sqrtf(x); | ||
| #else | ||
| return (float)Sqrt((double)x); |
There was a problem hiding this comment.
Why does this not call sqrtf when not using GameMath?
| #endif | ||
| } | ||
|
|
||
| WWINLINE float WWMath::Sqrt(int x) |
There was a problem hiding this comment.
Why do we need a function to handle promotion to float, this should be handled at the call site IMO to clearly show the intention to perform a floating point operation on an int and allow proper warnings to flag if it wasn't.
| return (1.0f - frac) * _FastAsinTable[idx0] + frac * _FastAsinTable[idx1]; | ||
| } | ||
|
|
||
| WWINLINE float WWMath::Acos_Legacy(float val) |
There was a problem hiding this comment.
Why is this needed when the current Acos float version will perform exactly the same operation as it is currently implemented. Either scrap this or call acosf in the none deterministic path for Acos?
| #endif | ||
| } | ||
|
|
||
| WWINLINE float WWMath::Asin_Legacy(float val) |
There was a problem hiding this comment.
A recurring theme, again 4 functions when 2 or perhaps 3 would be sufficient. Really you just need the overloads for float and double here with the none deterministic path returning the output of the double functions properly cast to match VC6 when compiled with retail compatibility in mind and assume only the GameMath paths matter for none VC6. The only gotcha will be where VC6 called the actual float functions but I'm not sure they existed in that version of the CRT.
|
I ran a bunch of tests to add as much conclusive "do this vs that" as I could - results: Proven — should change
These pairs were compared branch-by-branch and have identical implementations in every current math mode.
Proven — should not change
Still requiring integration validationThe standalone numerical questions are resolved. Remaining validation is integration-level:
Overall, the independent macOS and Windows results agree: the redundant inverse-trig and square-root wrappers can be consolidated, while the x87 sine/cosine paths, legacy absolute-value behavior, VC6 Tested by
(These conclusions were independently tested on two machines against PR head |
|
Just my 2 cents. I achieved great results on GeneralsX cross-play (Linux x Mac) with a very similar approach to this one. So I'm sharing some commits I made over there, hoping they might be useful for TSH as well: |
|
@Okladnoj Please work on last comments when you can. |
|
Hi! @xezon Sounds good. I’ll continue on Monday. I also ran my own individual tests. In most cases, the advice from @bobtista that overlaps with @OmniBlade’s recommendations was confirmed. However, I still have concerns about whether we might affect the My brain has kind of overheated, so I decided to take a short break from deterministic math until Monday. I’ll probably revisit the changes with a fresh perspective—not all at once, but function by function. I’ll start with the legacy code. |
My point about the Sqr function is why it has been introduced at all as it doesn't exist in the original code base so there is no original function to retain the behaviour of? If anything multiplications should be left where they are in the code and pow should be replaced with a WWMath::Pow function that wraps either pow(f) or gm_pow(f). |
Also needs a rebase. |
Deterministic math cleanup — decisions and validationRefactor of Removed
Kept
Validation
ReviewThe changes are split into two stacked PRs on the branch, so they're easier to check separately. Please review: |
|
Please rebase on main. There are a bunch of conflicts now. |
|
@xezon The fixes are in my latest branch. The build succeeds, and replay testing also passes successfully. All ready to review!) : RETAIL_COMPATIBLE_CRC=1 RETAIL_COMPATIBLE_CRC=0 |
Port the WWMath cleanup landed on GeneralsGameCode (PR TheSuperHackers#2670 line) so the DET builds match: - Remove the redundant _Legacy wrappers (Acos/Asin/Atan/Atan2/Sqrt) and the Sqrt(int) overload from wwmath.h; route Fast_Acos/Fast_Asin through the non-Legacy siblings (Fabsf_Legacy guard kept). All collapse to the same gm_*f in deterministic math, so DET output is unchanged. - Rename every call site to the non-Legacy names (Inv_Sqrt_Legacy / Fabsf/Sinf/Cosf_Legacy are intentionally kept and untouched). - Preserve numeric behaviour at the double-argument sites: shattersystem uses Sqrt((float)(...)), camera uses Atan2(x, 2.0f), euler From_Matrix keeps sy/cy as float, AIPathfind casts the int cell-delta to float before Sqrt. - Route the W3DMouse scroll-cursor angles through WWMath::Atan2. Builds clean on macOS (both GeneralsVanilla and GeneralsOnlineZH link).
7 participants
Rework of #2602, incorporating review feedback:
USE_DETERMINISTIC_MATHunconditional for non-VC6 — missinggmath.his now a compile error instead of silent fallback to x87/CRTCI: win32 + vc6 ✅, replay checks ✅
Open question: Replay checks pass both with and without
USE_DETERMINISTIC_MATH, even though golden replays were recorded with an x87 build. The replays may not containMSG_LOGIC_CRCmessages, meaning the check only validates absence of crashes rather than game state CRC parity. If anyone has insight on this — please share.Testing results
Cross-platform deterministic math parity verified with
SimulationMathCrc::runBenchmark— computes CRC over 10 000 iterations of sin/cos/tan/atan2/sqrt/pow across a fixed input set.fdlibm(deterministic)76B53840fdlibm(deterministic)76B53840E8B6385AE8B6385AB7B838508BB5B841Key fix:
-ffp-contract=offincmake/compilers.cmake— prevents Clang from emitting FMA instructions (fmadd) that skip intermediate rounding, breaking bit-exact parity with MSVC's/fp:precisedefault.