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.
|
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 |
Just looking at your results it seems that _PC_53 differs less from macOS 64bit than does _PC_24. I'm arguing that Pow. Atan and Atan2 with double returns should be using the double versions of the GameMath functions and would presumable work correctly under _PC_53? When you say erfc at float you mean gm_erfcf right? Tricky for me to test this as I only have x86 available so it would be interesting if the same discrepancy exists between x86 32bit and 64bit and if 64bit windows matches macOS. At a guess based on disassembly in godbolt, at -O2 msvc doesn't load the results from gm_expf into the sse registers and instead uses x87 to multiply the results together resulting in intermediate precision being higher than it should be.
I would say yes if we want to use the double type, otherwise refactor to only use float math throughout the code base. On RETAIL setting _PC_24 basically makes the game behave exactly like that had been done and the double variables throughout the code base are basically wasting memory. On modern is more complicated at a lot of the math will be done at the correct precision due to favouring SSE2 instructions, but returns will be incorrectly dropping precision for double returns. This will also affect the internal behaviour of GameMath itself, any internal calls will have precision dropped potentially causing differences even before the precision is dropped at the final return. |
|
Further to my last comment, its seems setting /fp:strict makes the code gen in godbolt look more correct, might be worth testing msvc with that set and see how it compares to macOS then with Edit: My bad, it also requires a few changes to gm_erfcf to change the code gen, but might be worth checking behaviour with just the fp mode set. |
|
Hi! @OmniBlade I ran the whole matrix: x86 and x64, Against macOS, out of 2904 rows:
math-summary.txtRow suffixes: Test and dumps: https://github.com/Okladnoj/GeneralsGameCode/tree/okji/test/deterministic-math-v2.2.8/tests |
|
So I've done more research into this and it seems that any floating point maths done in a single expression involving a floating point function return is subject to optimisation to use the x87 FPU. To get the correct behaviour in optimised code, every Note that setting _PC_24 is a fragile fix and that any double math done in the way described above would then be subject to potentially differing results between platforms. Correct solutions are in my opinion to sweep the code base to make the changes suggested to make MSVC happy or forget MSVC as a valid target for 32bit windows binaries post VC6 being dropped. |
Benchmark: what the deterministic math costs, and which configuration costs leastOnly two of the four combinations applyThe grid was run in full — every combination of argument type and x87 precision
The remaining cells are marked Today the game calls both the float and the double entry points, so either Step 1. Call frequency
Counters sit at the 52 places where the code reaches GameMath. A counter is keyed Results: https://github.com/OKJID/GameClient/blob/6c6dff1e43f6d38e683af5542aaf8622f72780c0/tests Two multiplayer battles on Ring Ring 8, eight slots, 44 and 69 minutes. From each Calls per logic frame:
The profile stores the counter readings as they are, as whole numbers; the rate There are no nanoseconds at this step, deliberately. A call count is a property Step 2. The benchmarkSet up here: https://github.com/Okladnoj/GeneralsGameCode/blob/53a81f86bbc829049d21fa223b6103c5eb482bfd/tests
Windows, from an ordinary shell — no Developer Command Prompt needed, the script It walks x86 and x64, each in two compiler modes — macOS: Then, from the Nanoseconds per call, win32 x86,
Only ResultsGenerated here: https://github.com/Okladnoj/GeneralsGameCode/blob/53a81f86bbc829049d21fa223b6103c5eb482bfd/tests/bench-weighted.txt Milliseconds per logic frame. The same set of calls and the same number of them — The game builds with no explicit
float + The ratio holds across both battles and both run orders: 2.108 and 2.118 on the Without the frequencies the same comparison gives 1.90 — that is the nanoseconds |
|
Nice tests. The performance advantage speaks for using float _PC_24. |
|
We could look at using CPU intrinsics for sqrt as its supposedly had defined results under the IEEE standard. Also, why is the arm code so slow compared to the x86 code? Being run on a slower CPU? I'd like to see comparison on the C Runtime math vs game math as well. |
Are you reading the same table I am? Doesn't look like it makes much difference to performance but overall float precision is faster if all math is done at that precision. |
|
Hi @OmniBlade
win32 x86,
The machines the measurements were taken on:
Nanoseconds per call, GameMath:
Milliseconds per logic frame, the same calls at the same counts, costed through Per function, win32 x86
Under |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7a4f6f1b7c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
| { | ||
|
|
||
| #if RETAIL_COMPATIBLE_CRC | ||
|
|
There was a problem hiding this comment.
When RETAIL_COMPATIBLE_CRC=0 (which is required to enable the new deterministic-math mode), under-construction objects skip startPowerRecharge(), so removing this branch leaves m_availableOnFrame at zero instead of 0xFFFFFFFF. Consequently, isReady() treats their non-shared special powers as immediately available until the creation callback initializes the timer; restore the conditional initialization in both the Zero Hour and mirrored Generals implementation.
Useful? React with 👍 / 👎.
| #define DEFINE_WEAPONSLOTTYPE_NAMES | ||
|
|
||
| #define NO_DEBUG_CRC | ||
| //#define NO_DEBUG_CRC |
There was a problem hiding this comment.
In a DEBUG_LOGGING build, commenting out this definition lets CRCDebug.h define DEBUG_CRC, activating the 27 DUMP* calls later in this file. Object creation then constructs and potentially records large amounts of deep-CRC diagnostic data, adding unintended overhead and noise to release-logging/replay experiments; restore NO_DEBUG_CRC unless this instrumentation is explicitly requested.
Useful? React with 👍 / 👎.
|
|
||
| // once we've ceiled/floored, trunc and round are identical, and currently, round is faster... (srj) | ||
| #if RTS_GENERALS /*&& RETAIL_COMPATIBLE_CRC*/ | ||
| #if RTS_GENERALS && RETAIL_COMPATIBLE_CRC |
There was a problem hiding this comment.
When Generals is built with RETAIL_COMPATIBLE_CRC=0, this new condition switches REAL_TO_INT_CEIL/FLOOR to the approximate helpers, which are incorrect at integral boundaries: fast_float_ceil(1.0f) produces 2.0f, while fast_float_floor(-1.0f) produces -2.0f. This directly breaks gameplay callers such as BridgeBehavior.cpp, where an even numObjects makes REAL_TO_INT_CEIL(numObjects / 2.0f) add an extra loop iteration, triggering the “Creating too many scaffold objects” assertion in debug builds and creating an extra scaffold in release builds.
Useful? React with 👍 / 👎.
| #if USE_DETERMINISTIC_MATH | ||
| return gm_sinf(val); | ||
| #else | ||
| return (float)Sin((double)val); |
There was a problem hiding this comment.
With RETAIL_COMPATIBLE_CRC=1, deterministic math is disabled, but the float Sin and Cos overloads now delegate to the double CRT overloads instead of preserving their previous Win32 x87 implementations (or sinf/cosf on other platforms). Existing simulation callers such as Geometry.cpp, BuildAssistant.cpp, AISkirmishPlayer.cpp, and AIStates.cpp still invoke these float overloads, so their results can change by a bit and break the retail-CRC guarantee; select the legacy implementation in retail-compatible mode or migrate every such caller to the appropriate legacy entry point.
Useful? React with 👍 / 👎.
| */ | ||
| Real minTurnRadius = (maxTurnRate > 0.0f) ? (maxSpeed / maxTurnRate) : 999999.0f; | ||
| // determine required turn radius based on our current speed and max turn rate | ||
| Real minTurnRadius = WWMath::Div_Safe(maxSpeed, maxTurnRate, 999999.0f); |
There was a problem hiding this comment.
When deterministic math is disabled—the default retail-compatible configuration—WWMath::Div_Safe performs an unguarded division, so replacing the prior maxTurnRate > 0.0f conditional here loses its explicit zero-rate handling. A delivery aircraft whose locomotor has a zero maximum turn rate now receives an infinite turn radius instead of 999999.0f, and that infinity propagates into timeToTravelThatDist; retain the caller-side conditional or make Div_Safe honor its fallback in every math mode.
Useful? React with 👍 / 👎.
|
@codex Explain and summarize what this pr do? |
|
@Okladnoj Yes, the sqrt intrinsics aren't used in GameMath at the moment but looking at the results you provide it looks like that is low hanging fruit for significant performance increase presuming they can be shown to generate identical results. |
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ 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". |
9 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.