Repository navigation
Conversation
|
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.
This comment was marked as spam.
This comment was marked as spam.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
|
Hey guys, I wanted to share an interesting finding from the community that might be very relevant to this PR and the overall direction of cross-platform determinism in the engine. With just It's an amazing work you guys are doing in this PR, but I'm wondering if introducing GameMath/fdlibm is truly necessary (or even desirable) to achieve cross-platform determinism. I'm sharing below a technical case study based on @MYSOREZ 's Android port, and how full cross-play was achieved there. (Note: The technical breakdown below was compiled with the assistance of an AI tool to summarize the findings clearly) Real-world Case Study: Android ARM64 ↔ Windows PC Cross-PlayIn the recent releases of the Android port (MYSOREZ/GeneralsZH-Android-Port, v1.4.0), the author successfully achieved live cross-play against the official Windows PC client ( Crucially: the Android port achieved this entirely with native system math, without GameMath or fdlibm. In fact, the author initially pursued the theory that cross-architecture desync was an arithmetic/FPU divergence issue between ARM and x86. However, testing the 12 transcendental simulation expressions across ARM64 and x86_64 proved that single-precision IEEE 754 math produces bit-identical results as long as What actually caused the desyncs?When investigated frame-by-frame against the PC client, every single desync was tracked down to codebase diffs, runtime shims, or state leaks rather than transcendental math:
Insight on the "Open Question"Regarding:
This aligns with what was observed above: the simulation's standard IEEE 754 float logic with Food for ThoughtGiven that:
Is bringing in |
|
Thanks for the mention — one correction to the case study. The Android port did not use the platform libm as-is: bionic's Generated by Claude Code |
|
Thanks @fbraz3 and @MYSOREZ — valuable data. The goal of this PR is to encapsulate all CRC-affecting math in one layer (WWMath) — not to replace native math with GameMath, but to define one point where it's decided which math feeds game logic and the CRC. The desync sources you list are handled through that same layer in Three modes: native/CRT (what you describe — A retail build does not go through GameMath — |
|
I think what they try to say is that GameMath is unnecessary or even counterproductive. But is this really true? This implies that all CPU architectures and system math libraries have the exact implementations and produce the same results. This sounds unlikely to be plausible. |
The idea is just bring it to discussion, there are no silver bullet in IT world =) |
|
@xezon exactly — and this is what my tests show. FMA contraction is off on both sides, but by different means:
On modern x86/ARM64 native math gives bit-exact parity.
Tests: https://github.com/Okladnoj/GeneralsGameCode/tree/okji/test/deterministic-math-v2.2.8/tests |
|
The reason round tripping through the double version and back to float works is because you are basically doing the calculation at a higher precision than you need the result at and then throwing away the excess. As long as the rounding mode remains the same and the double CRT functions rate of error is less than the precision of float it will hold. What this is not doing is getting the transcendental functions to return the same value between platforms and there is no way to guarantee this will work across all platforms and C runtimes. Maintaining compatibility with the VC6 clients from all builds long terms isn't a goal either as it places a lot of constraints on what improvements can be made to the engine. Finally, performance with GameMath is close to the CRT performance now with the use of the sqrt intrinsics. |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
|
|
||
| #include "WWMathTestCase.h" | ||
|
|
||
| static const std::map<std::string, WWMathDump> wwmathDumps = { |
There was a problem hiding this comment.
It looks like wwmathDumps needs to be kept in sync with wwmathCalls. This is probably no problem for the AI to handle, but there is no compiler guarantee and the human can easily break the order by fat fingering it. It would be better to redesign this implementation.
There was a problem hiding this comment.
Reworked in commit 9 (abe1a94):
- Each key is now written once, in
WWMathKeys.h, and both tables refer to it asWWMathKey::Sin_0_5f. A typo in a key no longer compiles. - The tables are merged by key, so the order of rows does not matter.
WWMathTables.KeysAreUniquereports every duplicate key in either table. A call without a dump, or a dump without a call, fails its own test.
There was a problem hiding this comment.
Would the DET column not be sufficient? Why do we want to test the win32 CRT output? Right now this locks the test to only succeed on a certain platform?
There was a problem hiding this comment.
CI builds the tests only in the win32* presets, and none of them sets RTS_BUILD_OPTION_RETAIL_COMPATIBLE_GAME=OFF, so all of them use CRT math. With only the DET column the test would always be skipped in CI. A build configuration without a column skips the test rather than fails it. Once the CRT path goes away, its column goes with it: one column in WWMathDumps.h and one entry in WWMathTestCase.h.
There was a problem hiding this comment.
Why do we want to test the win32 CRT output
So the only reason is not to skip the test?
There was a problem hiding this comment.
So the only reason is not to skip the test
No. The CRT column guards the retail-compatible branch of WWMath. That branch has its own code (x87 assembly in Sinf_Legacy and Inv_Sqrt_Legacy, no epsilon guard in Div_Safe), and VC6 compiles the same code. VC6 does not build the tests, so this column is where a change to that branch shows up.
There was a problem hiding this comment.
Ah I see, this is just to test those legacy branches and not the win32 CRT. Perhaps clarify somewhere?
There was a problem hiding this comment.
Done, thanks for the improvement.
…ns (TheSuperHackers#2670) Introduces the gamemath.cmake module and wires HAS_GAMEMATH / USE_DETERMINISTIC_MATH through the compiler configuration.
…ATH 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. BaseFunctions.h and Coord2D.h include BaseDefines.h, which enables the RETAIL_COMPATIBLE_CRC condition of REAL_TO_INT_CEIL, REAL_TO_INT_FLOOR and Coord2D::toAngle. config-retail.cmake collects the RETAIL_COMPATIBLE_ defines of BaseDefines.h as well, so RTS_BUILD_OPTION_RETAIL_COMPATIBLE_GAME keeps controlling RETAIL_COMPATIBLE_CRC.
…ers#2670) Moves the WWMath declarations and definitions into the layout used by the deterministic math entry points, so that the next commit only adds and changes functions in place. Merges the per-platform duplicate definitions into one body with the platform branches inside, moves the inline bodies of Fabs, Atan, Atan2, Ceil and Floor out of the class and drops the section banners. No functional change.
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. Adds WWMath::Div_Safe, which clamps the divisor to an epsilon under deterministic math. Routes the Trig.cpp functions through WWMath, adds Sqrt(Real) and Sqrt(double) there and drops the unused REGENERATE_TRIG_TABLES generator. Coord3D::length() keeps the original sqrt call under RETAIL_COMPATIBLE_CRC. Existing callers of the changed functions move to the _Legacy and float variants in this commit, so retail behaviour holds between commits.
…rappers (TheSuperHackers#2670) Replaces the libm calls inside the WWMath library itself (matrices, quaternions, vectors, spheres and vehicle curves) with the WWMath wrappers.
…Hackers#2670) Replaces the remaining direct libm calls in the shared engine code and 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.
…heSuperHackers#2670) Adds BezierMath with D3DXVec4Transform and D3DXVec4Dot. Under USE_DETERMINISTIC_MATH they are computed in plain C++ with a fixed order of additions; otherwise they call D3DX as before.
Removes the M_* math constants from visualc.h and switches their remaining uses in W3DMouse.cpp and W3DView.cpp to the WWMATH_* constants. Without them visualc.h is empty, so it is removed together with its include in always.h.
| GoogleTestMain.cpp | ||
| GameEngine/Common/AsciiStringTest.cpp | ||
| Libraries/Compression/CompressionManagerTest.cpp | ||
| Libraries/WWVegas/WWMath/WWMathTest.cpp |
|
|
||
| inline std::string toBitsString(long value) | ||
| { | ||
| return std::to_string(value); |
There was a problem hiding this comment.
This one now is not longer hex. Is that intended?
There was a problem hiding this comment.
Yes. It was decimal from the start (%ld): Float_To_Long returns an integer, and long is 4 bytes on Windows but 8 on Linux and macOS.
|
|
||
| namespace WWMathKey | ||
| { | ||
| constexpr const char *Sin_0_5f = "Sin(0.5f)"; |
There was a problem hiding this comment.
Instead of using const char* as a key, can we use enum values starting at 0 as keys? This should have the benefit that lookups are array indexes and not string searches.
There was a problem hiding this comment.
Done. The keys are an enum, both tables are indexed by it, and a static_assert checks that each table has one row per key, in key order.
| EXPECT_EQ(computed, mathCase.dump); | ||
| } | ||
|
|
||
| INSTANTIATE_TEST_SUITE_P(WWMath, WWMathBits, ::testing::ValuesIn(mergeCallsWithDumps()), testNameOf); |
There was a problem hiding this comment.
The way the test is setup here is difficult to intuitively understand. I do not understand it. It's fine if it works, but maybe we can simplify it somehow and add some comments that explain the flow.
There was a problem hiding this comment.
Simplified: each key is one test that runs its row in wwmathCalls and compares it with its row in wwmathDumps. The flow is described at the top of WWMathTest.cpp.
) Adds a unit test that checks the bits of each WWMath function with a deterministic implementation on fixed inputs, so a change in the math shows up from one build to the next. Deterministic math has one set of reference bits for every platform, the retail-compatible math one set for win32, where its branch is built with the tests; the test is skipped where a build configuration has none. A Google Benchmark times each WWMath function next to the CRT function it replaces.

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 was verified with
SimulationMathCrc::runBenchmark— computes CRC over 10 000 iterations of sin/cos/tan/atan2/sqrt/pow across a fixed input set. That function has since been removed; the comparison now lives in Google Benchmark asBM_SimulationMathCrcandBM_SimulationMathCrcWithSystemMath.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.Note
Route game logic math through
WWMathwith 3-mode deterministic supportWWMathAPI in wwmath.h with explicit float, double, and legacy float overloads for all math operations. Deterministic builds dispatch to single-precision GameMath functions; non-deterministic builds use precision-matched C math functions.USE_DETERMINISTIC_MATHandRETAIL_COMPATIBLE_CRCconfiguration macros in BaseDefines.h. Deterministic math enables only when GameMath is available and retail CRC compatibility is disabled. Adds GameMath as a CMake FetchContent dependency for non-VS6 builds via gamemath.cmake.sqrt,sin,cos,atan2,fabs,ceil, etc.) withWWMathwrappers across the entire engine:Locomotor,Weapon,PartitionManager,PhysicsUpdate,AIUpdate, pathfinding, terrain, geometry, projectiles, and dozens of other systems.WWMath::Div_Safeto replace direct division in ~15 call sites (mine placement, scaffold count, health ratios, build progress, flight-path segments, turn-radius), clamping the divisor to a supplied epsilon in deterministic builds.-ffp-contract=offfor non-VS6 compilers in compilers.cmake to prevent fused multiply-add contraction, and adds a deterministic-vs-native math benchmark viaSimulationMathCrc::runBenchmark.PartitionManager::calcMinRadiusandcalcRadiusVecnow useRealintermediates instead ofdoubleoutsideRETAIL_COMPATIBLE_CRCbuilds, which may change radius results.Coord3D::lengthand severalWWMathdouble overloads route through single-precision GameMath in deterministic builds, reducing precision.visualc.hno longer defines local math constants; consumers must useWWMATH_*equivalents.PolygonClass::Compute_Planenow casts squared length tofloatbeforeWWMath::Sqrt, which may lose precision for large polygons.📊 Macroscope summarized 32d3717. 188 files reviewed, 3 issues evaluated, 1 issue filtered, 2 comments posted
🗂️ Filtered Issues
GeneralsMD/Code/Libraries/Source/WWVegas/WW3D2/motchan.cpp — 0 comments posted, 1 evaluated, 1 filtered
filtertableis used to decode the packed motion-channel data, but this changes its generated values fromWWMath::Sin(float)(which used the double-precisionsinpath in non-deterministic builds) toSinf_Legacy(which usessinfon non-x86 builds). The table is not serialized with each motion channel, so existing assets decoded by a modern non-deterministic build can reconstruct different deltas/poses, and the change also makes the decoder's quantization behavior platform/configuration dependent rather than preserving the previous table generation. [ Out of scope (triage) ]