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 15 commits into
Conversation
|
| Filename | Overview |
|---|---|
| Core/Libraries/Include/Lib/BaseDefines.h | Defines the retail-compatibility and deterministic-math feature selection used throughout the target graph. |
| Core/Libraries/Source/WWVegas/WWMath/wwmath.h | Adds the central deterministic, CRT, and legacy math routing interfaces. |
| Core/GameEngine/Source/Common/Diagnostic/SimulationMathCrc.cpp | Routes the CRC fingerprint through WWMath and adds deterministic-versus-native benchmark reporting. |
| cmake/gamemath.cmake | Integrates the GameMath dependency for supported non-VC6 configurations. |
| cmake/compilers.cmake | Disables floating-point contraction where necessary to preserve cross-compiler numeric parity. |
| Generals/Code/GameEngine/Source/Common/System/Trig.cpp | Preserves the Generals trigonometry bridge while routing its implementation through WWMath. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
Callers[Gameplay and engine math callers] --> WWMath[WWMath routing layer]
WWMath -->|VC6 build| X87[Legacy x87 path]
WWMath -->|Deterministic enabled| GameMath[GameMath / fdlibm]
WWMath -->|Deterministic unavailable or disabled| CRT[Native CRT math]
GameMath --> CRC[Simulation CRC agreement]
X87 --> Retail[Legacy compatibility]
CRT --> Native[Platform-native behavior]
Reviews (13): Last reviewed commit: "bugfix(gamelogic): Restore the upstream ..." | 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 |
- 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:
|
|
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 |
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.
|
Tested against The results match. Speed, nanoseconds per call:
On x64 it lands exactly on the CRT. Weighted by the call frequency measured across two live battles, for win32 x86 with the default
The frame cost drops by almost half, and the deterministic math ends up cheaper than the platform CRT. As a side effect this narrows the float against double gap we were discussing: it was 2.11×, now 1.68–1.79×. Almost all of what float was winning came from the expensive double
My own opinion, separate from the measurements. The current mixed arrangement should go in as it stands — it is quite possibly the first and last point where retail compatibility and the deterministic build still hold together. After that I would move the game logic to float entirely and drop retail support: retail has computations declared in double, and moving those to float gives a different result, so keeping both is not on the table. Float is both the faster one and by far the more common in the logic: by the call frequency measurement, 96% of all math calls go through the float forms and 3.9% through double, and that 3.9% is almost entirely |
|
PS To put a number on the second half of that: in the Zero Hour GameLogic sources there are 1714 declarations of |
|
PSS. Went through the outstanding review threads. Of the five findings from the Codex review, three were real and are fixed in The other two do not reproduce and I answered in the threads. On the float The comments about moving render call sites to Fifteen threads from July are anchored to code that no longer exists; closed. One is left open on purpose — an issue with a test case for the |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Well, later will do it |
OmniBlade
left a comment
OmniBlade
left a comment
There was a problem hiding this comment.
I'm going to tentatively approve this as I believe the majority of the concerns have been addressed and the remaining concerns relate to things that cannot be addressed until VC6 retail compatibility is dropped.
For performance reasons bumping the version of GameMath and enabling intrinsics to improve sqrt performance may be desirable, but I don't think its a deal breaker.
|
Codex Review: Didn't find any major issues. Another round soon, please! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@Okladnoj Are the 15 commits as is meant to be merged with rebase onto main? |
…ns (TheSuperHackers#2670) Introduces the gamemath.cmake module and wires HAS_GAMEMATH / USE_DETERMINISTIC_MATH through the compiler configuration.
…TH switches (TheSuperHackers#2670) 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.
…ckers#2670) 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.
TheSuperHackers#2670) Keeps the GameLogic sources free of direct libm calls after the nuke radius debug draw was added.
…erHackers#2670) 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.
…ckers#2670) 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.
TheSuperHackers#2670) Routing simulation math through WWMath took three pieces of surrounding logic with it that had nothing to do with math. SpecialPowerModule lost the guard that holds a special power unavailable while its object is still under construction, added upstream in TheSuperHackers#1218. Without it m_availableOnFrame starts at zero rather than 0xFFFFFFFF when RETAIL_COMPATIBLE_CRC is off, and isReady reports the power ready until the creation callback sets the timer. Both games had it, both lost it. DeliverPayloadAIUpdate lost the explicit maxTurnRate > 0 test around the turn radius division. Div_Safe only stands in for it where deterministic math is compiled in, and only for a divisor of exactly zero, so the retail path was left dividing by zero where it used to fall back to 999999. Removing that test changes game logic rather than math, so the retail form is now the original expression and the guarded division sits in the other branch. ObjectCreationList had NO_DEBUG_CRC commented out, which lets CRCDebug.h define DEBUG_CRC and wakes the 27 DUMP calls further down the file in any build with debug logging.
47bc6ac to
882089b
Compare
Individually the commits may fail to build, and moreover they do not support RET backward compatibility for VC6, and there are also no guarantees of cross-platform determinism — only the whole solution, as is, guarantees these 2 behaviors. But I still do not want to squash such a cyclopean volume of code, and therefore I have prepared the commits for rebase in order to preserve them — 15 commits is the minimum of how the changes can be structured per module. |
|
Yes we need individual commits that are reviewable. Each commit needs to compile on its own and not break the build. It is perfectly fine if only the last commit reaches math determinism. |
I have just done a factual check of the commits for self-sufficiency: The first 10 commits are not self-sufficient — they are a per-module grouping of a single monolithic refactor — I would leave them as they are, but if you insist — I will simply squash these 10 commits into one. The rest, from the 11th to the 15th, are self-sufficient and build individually. Branches, one per commit, with the CI run and the first error:
The runs for c11–c15 are marked red only because of the Replay Check — my fork has no replay data. All 13 build jobs in them are green. |
|
We always ensure that all commits that go into main branch compile on their own. Splitting this big change into small reviewable chunks is the right approach. If there is a compile issue, then maybe reorder them or fix the individual commit(s) to avoid the compile error(s). |
9 participants
Merge by rebase
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.