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 14 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 |
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 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 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.
Introduces the gamemath.cmake module and wires HAS_GAMEMATH / USE_DETERMINISTIC_MATH through the compiler configuration.
…TH switches Moves RETAIL_COMPATIBLE_CRC into BaseDefines.h so that WWMath can see it without depending on GameDefines.h, and adds USE_DETERMINISTIC_MATH which is disabled automatically when retail CRC compatibility is required.
Adds the WWMath wrappers that dispatch between the deterministic gamemath implementation and the platform libm, plus the _Legacy variants used by rendering code that must stay outside the simulation.
Replaces direct libm calls in the game simulation of both Generals and Zero Hour with the WWMath wrappers, so that the simulation uses the deterministic implementation when it is enabled.
03c861f to
5a84bf1
Compare
Keeps the GameLogic sources free of direct libm calls after the nuke radius debug draw was added.
5a84bf1 to
56a9dbf
Compare
|
Hi @xezon @OmniBlade @Caball009 @bobtista The old history had a lot of dead ends, so instead of replaying all 96 commits I rebuilt it as 12. The tree is identical to what merging current main into the old head gives, so the diff itself is unchanged. RETAIL=1 - OK
DET=1 - OK
2 reps (mac-win and win-mac sides) |
| WWINLINE double WWMath::Atan(double x) | ||
| { | ||
| #if USE_DETERMINISTIC_MATH | ||
| return (double) gm_atanf((float)x); |
There was a problem hiding this comment.
Why is this and some of the other double overloads still using the float precision version?
| value&=0x7fffffff; | ||
| return *(float*)&value; | ||
| #if USE_DETERMINISTIC_MATH | ||
| return (double)gm_powf((float)x, (float)y); // gm_pow diverges on x87, gm_powf is bit-identical |
There was a problem hiding this comment.
Report this upstream with a test case and we can look into fixing it if possible?
stephanmeesters
left a comment
stephanmeesters
left a comment
There was a problem hiding this comment.
I think many files that do not contribute to the CRC are currently routed through WWMath unnecessarily. Review did not flag all cases please do a pass yourself
| if (WWMath::Fabs(pos.X - bcX) > (beX + extent) || | ||
| WWMath::Fabs(pos.Y - bcY) > (beY + extent) || | ||
| WWMath::Fabs(pos.Z - bcZ) > (beZ + extent)) | ||
| if (WWMath::Fabsf_Legacy(pos.X - bcX) > (beX + extent) || |
There was a problem hiding this comment.
If not participating in crc can be fabsf? Many times in this file
| #endif | ||
|
|
||
| Bool reallyscale = (WWMath::Fabs(scale - ident_scale) > scale_epsilon); | ||
| Bool reallyscale = (WWMath::Fabsf_Legacy(scale - ident_scale) > scale_epsilon); |
There was a problem hiding this comment.
If not participating in crc can be fabsf?
| if (light.Get_Flag(LightClass::FAR_ATTENUATION)) { | ||
|
|
||
| if (WWMath::Fabs(atten_end - atten_start) < WWMATH_EPSILON) { | ||
| if (WWMath::Fabsf_Legacy(atten_end - atten_start) < WWMATH_EPSILON) { |
There was a problem hiding this comment.
If not participating in crc can be fabsf?
| const static Vector3 offset_a = Vector3(WWMath::Cos(WWMATH_PI / 2), WWMath::Sin(WWMATH_PI /2 ), 0); | ||
| const static Vector3 offset_b = Vector3(WWMath::Cos(7 * WWMATH_PI / 6), WWMath::Sin(7 * WWMATH_PI / 6), 0); | ||
| const static Vector3 offset_c = Vector3(WWMath::Cos(11 * WWMATH_PI / 6), WWMath::Sin(11 * WWMATH_PI / 6), 0); | ||
| const static Vector3 offset_a = Vector3(WWMath::Cosf_Legacy(WWMATH_PI / 2), WWMath::Sinf_Legacy(WWMATH_PI /2 ), 0); |
There was a problem hiding this comment.
If not participating in crc can be cosf?
| if (!ClampFix) { | ||
| offset_u = offset_u - WWMath::Floor(offset_u); | ||
| offset_v = offset_v - WWMath::Floor(offset_v); | ||
| offset_u = offset_u - WWMath::Floorf(offset_u); |
There was a problem hiding this comment.
If not participating in crc can be floorf?
| float c,s; | ||
| c=WWMath::Cos(CurrentAngle); | ||
| s=WWMath::Sin(CurrentAngle); | ||
| c=WWMath::Cosf_Legacy(CurrentAngle); |
There was a problem hiding this comment.
If not participating in crc can be cosf?
| c=WWMath::Cos(CurrentAngle); | ||
| s=WWMath::Sin(CurrentAngle); | ||
| c=WWMath::Cosf_Legacy(CurrentAngle); | ||
| s=WWMath::Sinf_Legacy(CurrentAngle); |
There was a problem hiding this comment.
If not participating in crc can be sinf?
| if (!ClampFix) { | ||
| CurrentStep.U -= WWMath::Floor(CurrentStep.U); | ||
| CurrentStep.V -= WWMath::Floor(CurrentStep.V); | ||
| CurrentStep.U -= WWMath::Floorf(CurrentStep.U); |
There was a problem hiding this comment.
If not participating in crc can be floorf?
For maintainability isn't it better to just route everything? That way the standard is to use the routing functions for everything and you don't have to worry about if a calculation you are doing takes part in CRC or not to select the correct function to use. |
It's certainly simpler but I don't think we can justify the performance penalty. I don't know if it's strictly true right now but I would expect anything in |
Do we have benchmarks for the current iteration of this PR for with and without deterministic math enabled so the scale of the performance difference is established before we look at optimising? |
The override was a precaution and never had a measurement behind it. A Windows replay run built with intrinsics enabled produced CRC logs that are byte-identical to the run with them disabled, so the override only cost speed.
|
Hi @OmniBlade @stephanmeesters @xezon All three double overloads that game logic actually calls — Measured, mac vs Windows under In game this showed up as: frame 1 on Akas Magic, object 312 rotating,
I'll open an issue on GameMath with test cases if needed |
|
I thought using _PC_53 was discussed as with the current setting ALL returns in the game is done at 32bit precision in all cases on 32bit builds. _PC_24 should only be left in VC6 builds for retail compatibility. Seems like it was done in the belief that it speeds up calculations on the fpu. Because the 32bit ABI returns results in the x87 register for functions returning floating point values, any double returns anywhere will be truncated even if the code gen otherwise uses SSE for the maths and does everything at an appropriate precision. |
Yes, that's the mechanism: We did try Back in July bobtista dropped If we move to |
I think we should move to that because it should give IEEE behaviour or close to on 32bit builds while still making double work as double. @bobtista would need to confirm but when I see replay desync I normally assume its retail which I would have expected needs to keep |
Yeah that desync was not retail, sorry that wasn't clear. Both experiments were under RETAIL_COMPATIBLE_CRC=0 with USE_DETERMINISTIC_MATH=1.
You're right about double returns through the x87 ABI. The other side is that 53-bit x87 intermediates do not round like SSE/NEON binary32 operations. The float-path differences were bigger than the double-return problem. GPT says "the synthetic dump was misleading in the second experiment: without _PC_24 it matched x64, while the real simulation still diverged around frame 200. I would use replay parity to decide this." |
|
btw I mentioned it a few weeks ago - Div_Safe enables its zero-divisor guards under USE_DETERMINISTIC_MATH. BaseDefines.h undefines that macro when gmath.h is unavailable, so a RETAIL_COMPATIBLE_CRC=0 build without GameMath silently falls back to the unguarded divisions. Whether those guards are enabled is a retail-parity decision, not a GameMath-availability decision; the guard should be keyed to !RETAIL_COMPATIBLE_CRC.
|
I ran the whole library on both platforms. Each function is called on the same Test and dumps: https://github.com/Okladnoj/GeneralsGameCode/blob/okji/test/deterministic-math-v2.2.8/tests/math-diff.txt macOS against win32, differing lines: Under Should we rework the math for !!! |
Four divisions in Generals were left unguarded while Zero Hour already routed the same places through WWMath::Div_Safe. Fallback values match the Zero Hour side so the two games behave alike.
There is no dependency on If that dependency really belongs there, then let's drop The other two are done: the description now matches what |
8 participants
Rework of #2602, incorporating review feedback:
USE_DETERMINISTIC_MATHdefaults on for non-VC6.BaseDefines.hturns it off automatically whengmath.his not available or whenRETAIL_COMPATIBLE_CRCis set, so a build without GameMath falls back to the CRT path rather than failingGM_ENABLE_INTRINSICS=OFFoverride was dropped after a Windows replay run showed byte-identical CRC logs with intrinsics on and offmain, no merge commitsOpen 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.