feat(box2d): native Go port of Box2D v3.2.0, swap physics2d off CGO - #928
Open
smsunarto wants to merge 28 commits into
Open
feat(box2d): native Go port of Box2D v3.2.0, swap physics2d off CGO#928smsunarto wants to merge 28 commits into
smsunarto wants to merge 28 commits into
Conversation
Port the full Box2D v3.2.0 feature set to pure Go at pkg/box2d: rigid bodies, all 5 shape types, the TGS-soft solver with sub-stepping, all 7 joint types, sensors, continuous collision (bullets), and world queries/explosions/debug draw. Float64 throughout, with package-wide FMA-safe arithmetic and ported deterministic trig so results are bit-identical across architectures (amd64/arm64), verified by golden hash tests gated in CI. Migrate pkg/plugin/physics2d off its CGO backend onto pkg/box2d: - Kill the package-global runtime singleton; Plugin now owns a per-instance Runtime, enabling parallel tests and multiple physics worlds per process. - Replace the WorldID() uint32 CGO escape hatch with a typed Engine() *box2d.World accessor. - Delete internal/cbridge and the vendored Box2D C source (third_party/box2d); the plugin builds with CGO_ENABLED=0. - Validate against CGO-recorded golden traces: contact/sensor event sequences and query results match exactly, body state matches within tolerance for the float64 vs float32 backend difference. Also fixes a latent bug this port's fuzzer found in the ported solver: a body leaving the awake set via SetBodyType/DisableBody kept a stale body-move-event index, causing an out-of-bounds read on a later forced sleep (upstream C has the same gap, unguarded). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Claude finished @smsunarto's task in 6m 2s —— View job Code reviewReviewed the port with a focus on the determinism machinery, the worker pool, and the
Overall this is unusually disciplined work. The determinism argument is mechanically enforced rather than asserted, and the reasoning is written down where a future maintainer will actually hit it. Specifically worth calling out:
Three findings below, one of which I'd fix before merge. 1. Non-deterministic body destruction on the
|
Move the cross-arch (amd64/arm64) golden-trace determinism job out of common-go-ci.yaml into a dedicated box2d-determinism.yaml workflow, scoped to pkg/box2d changes instead of every Go change repo-wide. Also run the deterministic op-sequence fuzz corpus (added by pkg/box2d's hardening tests) alongside the golden traces, and give the check its own independently visible status on PRs. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Temporary. TestGoldenDistance/Joint/Step/Continuous/Math all failed cross-arch on the CI amd64 runner while arm64 matched the committed golden (generated on arm64). This logs per-iteration bit patterns for the failing vector_pipeline_chain golden case so the divergence point can be bisected from a single CI run. Will be removed once found. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
❌ 3 Tests Failed:
View the top 3 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
The golden bit-pattern tests were generated on arm64 and failed on the CI amd64 runner. Root cause: the package's FMA rule only guarded products written inline as an operand of + or -. Fusion is also legal when an unrounded product reaches the + through a local, a struct field, or a function parameter that is later inlined -- e.g. MulSV's `s * v.X` flowing into mulAdd's addend, which arm64 emitted as FMADDD and baseline amd64 did not. Disassembly showed 161 such sites. Round products where they are formed, and additionally round the operands the math_fma.go helpers receive so a caller's product is rounded once the call is inlined (the smaller fix at most sites). arm64 now emits zero FMA instructions, and the regenerated goldens carry exactly the values amd64 was already producing. Add TestNoFusedMultiplyAdd, which compiles the package for arm64 and amd64/v3 and fails on any FMA instruction in the assembly. Reviewing source for this is unreliable, so the invariant is now machine-checked and wired into the determinism workflow ahead of the golden tests, so a regression reports its cause rather than just "bits differ". Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The templates still carried wire.gen.go from before commit 7a20719, which moved components, commands and events to a single Serializable contract (UnmarshalWire returns (any, error), MarshalWire required) and removed cardinal.RegisterCommandCodec. As a result `go build ./pkg/...` failed and the repo Lint and Test jobs were red regardless of the change under review. Update the affected files to the current contract, following the shapes in pkg/plugin/physics2d, and drop the codec registration blocks. These are normally produced by `world sdk generate`, which needs Docker; each file notes that it was hand-updated pending a regeneration. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
CodeQL flagged the workflow for relying on the default GITHUB_TOKEN scope. The job only checks out and builds, so contents: read is sufficient. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The previous commit removed fusion from the package but the amd64 golden tests still failed. Golden values are partly computed by test code -- scene layout, seeded input tables, the spring integration loop -- and `go build` never compiles test files, so the gate could not see those sites. Three goldens diverged for exactly that reason. Round the products in those computations, including one that is only a product after the compiler strength-reduces a division by a power of two into a multiply. Compile the test binary in the gate as well, so a fusion introduced by a future test is caught the same way. arm64 now emits no FMA instruction in either the package or the test binary, and goldens verify clean on repeated runs. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Member
Author
|
The 16 failures were the amd64 golden tests: golden values are partly computed by test code (scene layout, seeded input tables, the spring integration loop), and 🤖 Addressed by Claude Code |
The review job has failed on every run since it was added, always with total_cost_usd 0 and duration_ms around 100 -- the first model call is rejected before any tokens are billed, so retrying cannot help. The action hides the API error by default, which leaves no way to tell a 401 from a quota or malformed-request failure. Turn on show_full_output so the next run prints the real message. Revert once the cause is known. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… error" This reverts commit 09fdb06.
The workflow only had a path-filtered `push` trigger, so the status landed on whichever commit happened to touch pkg/box2d and never on the pull request head itself. A later commit outside those paths left the PR with no determinism result at all -- the state this very PR was in -- and pull requests from forks never ran it. Add the same path-filtered `pull_request` trigger. The path list is duplicated rather than shared through a YAML anchor because the Actions workflow parser does not support anchors. Reported by an external review of this PR. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Destroying a body, rebuilding it, or replacing its fixtures ends every contact it was part of, but no End event reached consumers. The engine does report end-touch for a destroyed body; by the time those records are drained the shapes no longer exist, so they cannot be resolved to entities and are dropped. Meanwhile the reconciler had already removed the pair from ActiveContacts, so the rebuild diff could not report it either. A consumer that latches state on Begin -- an "is grounded" flag, say -- stayed latched forever. Synthesize the End from the persisted pair metadata as it is pruned and hold it until the next flush, sorted so the sequence stays deterministic. The existing destroy test only asserted the tick did not crash, which is how this went unnoticed; the new test asserts the End actually arrives and fails without this change. Reported by an external review of this PR. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Dropping upstream's global world registry left every World sharing one id namespace, so equivalent allocation slots produced identical ids. worldB.IsBodyValid(idFromWorldA) returned true and DestroyBody with a foreign id silently destroyed an unrelated body in world B -- confirmed by reverting the fix, where the new test panics inside BodyPosition because the body really was gone. Give each World a distinct owner token from a process-wide counter, stamp it into the ids it issues, and check it in the validators, the internal lookups and the destructive entry points. The token is checked at runtime, not only under assertions, since a foreign id's index and generation routinely collide with a live object. Tokens wrap at the uint16 range; a reused token only weakens detection and never accepts an id that fails the index, generation and liveness checks. The token never reaches the solver or any golden. Cross-world tests that compared whole ids now compare the slot index and generation and additionally assert that world0 is the owning world's token, so they check more than before, not less. Reported by an external review of this PR. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The package exposes SetLengthUnitsPerMeter, but the tolerances upstream derives from it were frozen Go constants, so a caller working in non-meter units silently got upstream-divergent collision, continuous collision and broad-phase behavior. Upstream evaluates them as macros at use time. Convert the length-scaled quantities -- LinearSlop, Huge, SpeculativeDistance, ContactRecycleDistance and MaxAABBMargin -- to variables recomputed by the setter, and leave the dimensionless constants alone. Each default is derived from the same untyped constant expression as before and only multiplied by a length unit of 1.0, which is exact, so the defaults stay bit-identical; a test pins that with Float64bits and the goldens are untouched. Because LinearSlop is no longer constant-foldable, several expressions that the compiler used to evaluate at compile time became runtime products that could fuse into an FMA. TestNoFusedMultiplyAdd caught them and they are rounded at their definitions. Reported by an external review of this PR. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Internal invariants are asserted through assert(), which compiles out
because debugAsserts is false. The checks that a definition came from
its Default*Def constructor rode along with them, so
CreateBody(&BodyDef{}) silently produced a body whose Rotation is
Rot{0,0} -- not a normalized rotation -- and the damage surfaced later
as garbage motion or a confusing panic far from the cause.
Split the two tiers: internal invariants stay compiled out, while the
13 public creation entry points always check their definition and
panic with a message naming the constructor to use. Also always
validate the handful of values where a bad input corrupts quietly
rather than failing loudly: a body's rotation must be normalized, and
a shape's density, friction and restitution must be finite.
Reported by an external review of this PR.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Scoping the gosec exclusion to G115 (see the lint change) surfaced 27
G602 slice-bounds diagnostics that a blanket exclusion had hidden.
Triaging them found five reachable from the public API: Polygon.Count
and SimplexCache.Count are exported fields, so a hand-built
Polygon{Count: 20} or SimplexCache{Count: 9} indexes past the
fixed-size arrays the port inherits from upstream.
Validate both at the entry points that let such a value into the
engine -- CreatePolygonShape, SetShapePolygon, ComputePolygonMass,
CollideChainSegmentAndPolygon and ShapeDistance -- so the failure is a
clear message at the boundary instead of a bounds panic deep in the
solver. The polygon lower bound is 1 rather than 3 because the port
itself routes 1- and 2-vertex polygons from capsule collision through
these paths.
The remaining sites are bounded by construction; each carries a
nolint naming the invariant, since G602 is intraprocedural and does
not see a guard even in the same function. One site, the tree rebuild
stack, is documented as not provably bounded -- upstream guards it
with an assert alone and the only alternative is swapping one panic
for another.
Reported by an external review of this PR.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Excluding all of gosec for pkg/box2d silenced the 161 expected G115 integer-width conversions from the ported id packing and hashing, but it also hid 16 G602 slice-bounds diagnostics -- five of which turned out to be reachable from the public API. Silencing a whole linter to quiet one rule is how those stayed invisible. Exclude only G115 and leave the rest of gosec enforced there. Reported by an external review of this PR. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The system factories were exported but uncallable: both take an *internal.Runtime, which Go's internal-package rule puts out of reach of every consumer outside pkg/plugin/physics2d. They had replaced directly-registerable exported systems. Nothing outside the plugin referenced them, so move the package to internal/system where its scope is visible from its location; RegisterPlugin remains the supported entry point. Engine() promised access to "any Box2D feature not directly exposed" while its own caveats said mutation desynchronizes the ECS and that engine-created objects are lost on any rebuild. Document it as what it actually is -- a read-only escape hatch for queries and inspection -- and make that usable by adding BodyID and ShapeIDs lookups from an entity, so callers can reach the engine objects the reconciler owns without guessing. Changes that must persist still go through the ECS components. Also return a non-nil empty slice from an AABB overlap miss: a nil slice marshals as null where the previous backend produced [], and a query result crosses process boundaries, so that is part of the contract rather than an implementation detail. Reported by external review and differential testing of this PR. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The G602 triage flagged two more reachable panics in the same class as the ones already fixed but left them out of scope, which was the wrong call: they are the same bug, in the same file, and cheap to close. ShapeProxy.Count and SimplexCache.IndexA/IndexB are exported fields, so a hand-built proxy can claim more points than its fixed-size array holds, and a stale cache reused against smaller shapes can name points that no longer exist. Both are then copied or subscripted directly. Validate them at ShapeDistance, and validate proxy counts at ShapeCast and TimeOfImpact, which take proxies the same way. Removing the checks again makes the new tests fail with out-of-range panics, so these were reachable crashes rather than theoretical ones. The supported warm-start path -- feeding back the cache from the previous call -- is covered and unaffected. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The oracle-test sweep found that src/mover.c was never ported: CastMover and CollideMover gather collision planes, but the solver that consumes them -- b2SolvePlanes and b2ClipVector, the core of the character mover documented in docs/character.md -- had no Go counterpart, along with the b2CollisionPlane and b2PlaneSolverResult types. The earlier "0 functions missing" API audit scanned box2d.h only; these two live in collision.h. Port the file (72 lines of C) with the package's FMA discipline, and add oracle tests whose expectations are hand-derived from mover.c and character.md: single-wall pushout to -LinearSlop, push-limit clamping with exact convergence iteration counts, corner resolution, the push-reset-on-entry contract, all four ClipVector skip/clip branches, and an end-to-end CollideMover -> SolvePlanes -> ClipVector pipeline against a real world. One of those hand-derivations was initially wrong and the code was right, which is the point of deriving expectations from the C rather than from the port. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Raise package coverage from 88.7% to 96.9% with ~1,100 new assertions whose expected values come exclusively from the vendored C (extracted from git history -- upstream has no v3.2.0 tag), upstream's own test/*.c suite, and docs/, never from running this port. Each nontrivial expectation cites its C file and line. Where our float64 diverges numerically from C float32, tolerances are documented rather than constants adjusted. Every upstream unit test that applies is ported: test_math, test_id, test_bitset, test_table, test_collision, test_distance, test_shape, test_dynamic_tree, test_world, plus contracts from simulation.md and character.md. The foundations agent compiled and RAN the vendored C to produce byte-exact expectations for hashes, slot layouts and block words. Behavioral divergences from the C: none found. Two deliberate constant deviations are now documented at their definitions instead of being accidental: Pi keeps the decimal literal (C's float32 literal actually evaluates ~8.7e-8 higher, so angle wrapping differs ~1.75e-7 per turn), and getIDBytes reports Go's real 64-bit int footprint. C-faithful quirks pinned by tests: joint setters never wake bodies in v3.2, boundingPowerOf2(x<=1) == 1, minFloat NaN asymmetry, and an upstream out-of-range hazard in chain surface materials on open chains (inherited faithfully, avoided by the tests). Remaining uncovered code is almost entirely compiled-out debug validation and defensive branches that are dead in the C as well. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ecks Profiling against the old CGO backend found circle sweeps 62-86% slower (3 heap allocations per call from any-boxed callback contexts), ~1.2 MB allocated per 5000-body step, and World.solve at 75% of engine CPU. Three structural change sets, none touching a single float expression -- byte-identical goldens after every batch are the proof: - Query paths: callback contexts, sub-inputs and plane results move to per-World/per-tree scratch (saved and restored, so re-entrant queries from user callbacks stay correct). Raycast and circle sweep are now 0 B/op, 0 allocs/op; 13-29% faster in the wrapper. Scratch fields sit at the end of their structs -- placing one mid-struct pushed hot scalars onto extra cache lines and cost 12% on overlap queries. - Step scratch: broadphase move results/pairs return to World-owned buffers (upstream keeps them on b2BroadPhase; the port had made them per-call locals), and all arena slots grow geometrically and never shrink, matching upstream arena high-water behavior. Bytes per step drop 67-96%; wall clock is unchanged within noise, the win is GC pressure on any process sharing the heap. - Hot loops: reslice-to-length in the contact solver color routines, manifold SAT loops and simplex handling so the compiler drops per-iteration bounds checks (verified zero remaining with ssa/check_bce), pointer-held sweeps instead of 80-byte copies in the TOI separation function, caller-provided simplex in ShapeDistance. ShapeDistance -25%, TimeOfImpact -12 to -19%, mid-size steps -6%. The two behavioral clarifications are documented in code: worlds and trees are single-threaded for queries as well as stepping, and the *PlaneResult passed to PlaneResultFcn is valid only during the callback, exactly as in the C. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Restore upstream's task-system parallelism as an internal goroutine pool with a hard guarantee upstream does not make: simulation results are byte-identical for every WorkerCount, verified against the unchanged golden files. Engine (WorldDef.WorkerCount, 0 = serial, clamped to MaxWorkers and GOMAXPROCS; serial worlds run today's exact code behind pool == nil): - worker_pool.go: persistent workerCount-1 goroutines, channel dispatch + WaitGroup barrier, Step goroutine is worker 0. Static contiguous ascending ranges by ceiling division - no work stealing, so ascending-worker merges reproduce serial item order exactly. - Per-worker taskContexts (the upstream b2TaskContext array this port had collapsed to one): bitsets merged by inPlaceUnion in worker order (contact state, joint events, enlarged sims, awake islands, sensor bits); sensorHits and bulletBodies concatenated in worker order; split-island candidate reduced with upstream's strict-> in worker order. - Parallel stages: broadphase pair finding (per-worker pair arenas, worker-local head indices, serial createContact loop unchanged in moveArray order), narrow-phase collide, integrate velocities/positions, per-color constraint stages (one barrier per color; overflow always serial on worker 0 in upstream's stage position), finalize bodies, sensors. - Deliberately serial: bullet loops (bullets query trees other bullets mutate - upstream tolerates that race, the -race gate cannot), island split, tree rebuild, contact creation, all event assembly, arena and id pools. Counters().TaskCount stays stage-counted, identical for every worker count. Wrapper: physics2d Config.Workers (clamped, documented as a pure throughput knob - determinism is unaffected). Gates: golden suites re-asserted at workers 2/4/8 against the same testdata; fuzz corpus replayed at w=1 vs w=4 with checkpoint hash equality; TaskCount parity test; -race over goldens plus a stress scene sized to push every dispatch past its grain (also added to determinism CI); no-FMA disassembly gate; golangci-lint clean. Bench (Apple M5 Max, darwin/arm64): MixedRain 5000 bodies 2.68ms -> 1.26ms per step at workers=18 (2.1x); 1000 bodies 1.2x; small scenes stay on the inline serial path below grain thresholds. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A 41-agent adversarial review of the multithreading commit confirmed no simulation-correctness defect but found the verification gates weaker than claimed and small scenes slower with workers enabled. Fixes: Gate integrity: - Remove the GOMAXPROCS clamp on WorkerCount. It silently degraded every worker-matrix test and CI row to GOMAXPROCS-way partitions (vacuously green on small runners: w=8 rows ran 4-way partitions on 4-vCPU CI, serial-vs-serial on 1 CPU). Worker count is now the caller's explicit choice, as upstream; oversubscription costs throughput, never correctness. - Pool-engagement canary test (WorkerCount silently ignored can no longer stay green), exhaustive workerRange/forRangeWorkers property tests, and a real fan-out probe. - Fuzz lockstep worlds now start from a base scene sized past every grain threshold, and the 25-seed op-churn table replays serial vs workers=4 - previously neither ever dispatched to the pool. - Stress signature now folds full event-stream contents, and the stress scene runs a second pass with preSolve/custom-filter/mixing callbacks installed, exercising the concurrent-callback contract under -race. - Wrapper coverage: golden traces replayed at Config.Workers=4, clamp tests for negative/oversized Workers. Performance: - Capped worker engagement: forRangeWorkers = clamp(n/grain, 1, workers) replaces the all-or-nothing split; grain now means minimum items per worker (upstream minRange semantics). MixedRain at Workers=18: 500 bodies +26% slower -> 1.4x faster, 1000 bodies +18% slower -> 1.6x faster, 5000 bodies 1.86x -> 2.3x. - Per-stage presize/merge loops bound by the same pure function as their dispatch (10-body world at w=64: +27%/step -> +2.6%). - stepContext and dispatch capture vars moved onto World: the serial path is back to 0 allocs/op (the dispatch closures had forced a heap escape even with the pool disabled). Robustness: - runtime.Goexit inside a dispatched callback (e.g. t.Fatal) previously killed a worker silently and deadlocked a later Step; the pool now raises a sentinel panic and respawns the worker. Regression-tested. - Worker panics re-raise the original panic value (type preserved, consistent with the inline path); byte-identical determinism means any panic reproduces at WorkerCount=1 with a full stack. - Goroutine-leak and worker-panic paths now have regression tests. All golden suites pass unchanged at workers 1/2/4/8; testdata untouched. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… end PGO: commit default.pgo (merged CPU profile from the step benchmarks: mixed rain 1000/5000 bodies at workers 1 and NumCPU, pyramid and jointed at workers 1). Measured with benchstat (n=6): MixedRain 5000 -9.8% serial / -6.4% at 18 workers, Jointed -10.2%, MixedRain 500 -5.8%, geomean -4.4%, no significant regressions, allocs unchanged. Determinism is unaffected and now mechanically proven under PGO: - the full golden + worker-matrix suites pass byte-identical when built with -pgo=default.pgo; - TestNoFusedMultiplyAdd repeats its disassembly scan with the profile applied on arm64 and amd64/v3 (PGO steers inlining, and inlining is what exposes new fusion sites; the float64() roundings are semantic and survive it) — and now FAILS LOUDLY if the committed profile goes missing instead of silently dropping that coverage; - the determinism CI golden step builds with the profile. Go does not auto-apply a library's default.pgo: consumers put a profile in their own main package (doc.go explains). SoA: a full structure-of-arrays repack of the ~320-byte contact constraint (mirroring upstream b2ContactConstraintSIMD's grouping, arithmetic and iteration order untouched, goldens byte-identical) was implemented, benchmarked, and REVERTED: it measured 3-15% slower across every scene on arm64. Root cause, not noise: the solve loop went from one constraint pointer to ~24 live slice headers (381 -> 629 instructions, register spills 34 -> 90) and the AoS record is exactly 5 cache lines that sequential prefetch already streams perfectly. The negative result is recorded in the contact_solver.go header so the experiment is not blindly repeated; SoA returns only with real SIMD lane kernels that require it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Extracts the falling-circles step scene into stepBenchScene and adds a Workers=NumCPU variant at 1000/5000 bodies, used for the CGO-vs-pure-Go comparison in the PR description. Results are byte-identical at every worker count; this measures throughput only. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
solveContinuous declared its continuousContext as a local and passed its address through DynamicTree.Query's `any` context parameter, forcing one heap allocation per fast body per step (~80% of all steady-state step allocations: 545 -> 13 allocs/op and 209 KiB -> 26 KiB per step on MixedRain 5000 Workers_1). The context now lives at the tail of the per-worker taskContext and every field a sweep consumes is reinitialized at the top of solveContinuous, so results are identical by construction: no float arithmetic, ordering, or id assignment changed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Narrow TreeNode.Child1/Child2/Parent to upstream's int32 width so the node is exactly 64 bytes (one cache line; two nodes per Apple M-series 128-byte fetch) instead of straddling two lines. Indices stay Go int in every signature and local - the narrow width is storage only, converts losslessly, and no index approaches 2^31, so results are byte-identical (all goldens pass unchanged at every worker count). The dynamic tree is the write-and-read-hot structure in proxy-churn scenes (insert/remove/balance/refit/rebuild/query are all node-bound), so this single layout change is the largest serial win of the perf effort. Same-hour benchstat A/B (n=6, Apple M5 Max): MixedRain 100 bodies serial -53% workers -41% MixedRain 500 bodies serial -30% workers -13% MixedRain 1000 bodies serial -31% workers -15% MixedRain 5000 bodies serial -29% workers -13% Jointed -5% Pyramid (static tree) unchanged - the expected signature Raycast/Overlap 5000-body queries improve ~10-20% as well. Implemented during the overnight experiment run (E3); verified and adjudicated against the keep rule (p=0.002 on every improving row, no regressions). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…gather
Cut wrapper-side per-tick allocations (-32% B/op at 1000 bodies, -36% at
5000) by reusing buffers that were re-allocated every tick:
- gatherRebuildEntries appends into a caller-owned scratch slice reused
across ticks (was: fresh cap-64 slice regrown to N every tick)
- ReconcileFromECS sorts into rt.reconcileSortScratch (was slices.Clone)
- destroyOrphanBodies uses index-based binary search over the
EntityID-sorted entries (was: per-tick map[EntityID]struct{})
- gatherLiveContacts reuses its result map, body-id slice, and
ContactData buffer via runtime scratch fields (runs up to twice/tick)
- writeback entries gathered into a closure-owned scratch slice
Behavior-preserving: iteration orders, written component values, and
emitted event sequences unchanged; golden traces pass.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
pkg/box2d: rigid bodies, all 5 shape types, the TGS-soft solver with sub-stepping, all 7 joint types, sensors, continuous collision (bullets), and world queries/explosions/debug draw.common-go-ci.yamldeterminism job).pkg/plugin/physics2doff its CGO backend ontopkg/box2d:Pluginnow owns a per-instanceRuntime, enabling parallel tests and multiple physics worlds per process.WorldID() uint32CGO escape hatch with a typedEngine() *box2d.Worldaccessor.internal/cbridgeand the vendored Box2D C source (third_party/box2d); the plugin now builds withCGO_ENABLED=0.SetBodyType/DisableBodykept a stale body-move-event index, causing an out-of-bounds read on a later forced sleep (upstream C has the same gap, unguarded). Regression test:TestBodyMoveIndexInvalidatedOnAwakeSetExit.Why the pure-Go port
World Engine games carry a hard requirement: every machine — each player's server, every replay, every verifier — must compute the exact same game state from the same inputs. Physics is chaotic, so a difference in the 15th decimal place on frame 1 is a visibly different game by frame 1000. When machines disagree, players feel it: rollback netcode "corrects" positions and characters teleport, replays can't be trusted, and state verification fails with no way to tell who's right.
"Why rewrite 29k lines instead of patching?"
Because both fatal problems lived outside code we could patch. First, the calculators disagreed: different CPU families (Intel/AMD vs Apple/ARM) are legally allowed to round certain fused math operations differently, and the C build used them — identical inputs produced microscopically different results per machine. That is a property of the toolchain, not a fixable bug. Second, the Go-to-C bridge charged a translation tax on every frame: 32% of each tick went to copying collision data across the border, six times the physics computation itself (5.7%). The port also unlocks what the bridge forbade: builds with no C compiler on any platform, multiple physics worlds per process, and Go's race detector, fuzzer, and profiler seeing inside the engine — the fuzzer already caught a real solver bug that still sits unguarded in upstream C.
"Wasn't the C engine faster?"
Performance is the supporting act, honestly stated. On a single core, yes: the Go engine is 20–30% slower, because it uses double-precision math where the C build used single. But at the scene sizes where that gap is widest, both engines finish in roughly 1–13% of a 60 Hz frame budget — not something a player can see. And the C bridge was locked to one core forever, while the Go engine steps on all cores and stays byte-identical at every worker count. On an 18-core machine, full game tick:
Large-scene raycasts are 2x faster in Go (no per-hit border crossing), query performance overall is at parity, and opt-in profile-guided builds (the compiler optimizes using a recorded CPU profile) recover a further ~5–10% of engine speed.
"How do we know the Go one is actually correct?"
Not by promise — by machine. The port follows upstream Box2D v3.2.0 file-by-file. CI compiles the engine for both CPU families and compares recorded simulations bit-for-bit. One test disassembles the compiled machine code and fails the build if the divergent instruction family (FMA) appears anywhere. The same checks prove byte-identical output at every thread count.
Notable for reviewers
pkg/box2dis large — treat it as a faithful, mechanical port. Each source file's header comment states its Box2D upstream provenance (files carry a// Ported to Go from Box2D v3.2.0 ...line);pkg/box2d/LICENSEcovers the MIT (Box2D) and zlib (ByteArena Go port, used for some seed algorithms) attributions.*expression may appear as a direct operand of+/-anywhere in the package (seemath_fma.go) — this prevents FMA-instruction divergence between architectures.testdata/golden_*.jsonfiles are committed bit-pattern regression fixtures; do not regenerate them casually.pkg/plugin/physics2d/test/testdata/golden_*.jsonare the pre-removal CGO reference traces, kept as a permanent cross-backend regression anchor (see the header comment ingolden_trace_test.gofor the do/don't on regenerating them).go test ./pkg/box2d/... ./pkg/plugin/physics2d/..., including withCGO_ENABLED=0.golangci-lintis clean.Test plan
go build ./pkg/box2d/... ./pkg/plugin/physics2d/...go test ./pkg/box2d/... ./pkg/plugin/physics2d/... -count=1(all green, including golden hash and golden-trace tests)CGO_ENABLED=0 go build/test ./pkg/plugin/physics2d/...(proves the CGO dependency is gone)golangci-lint run ./pkg/box2d/... ./pkg/plugin/physics2d/...— 0 issues🤖 Generated with Claude Code
Benchmarks and profiling vs the CGO backend
Setup: Apple M5 Max (arm64, 18 cores), go1.26.5. CGO =
origin/main(vendored Box2D v3.2.0 C, float32, single-threaded bridge). Pure Go = this branch, default build (no PGO — Go does not auto-apply a library'sdefault.pgo; it is opt-in for game binaries). Same wrapper bench harness on both sides (pkg/plugin/physics2d/test, falling-circles scene),-benchtime=50x -count=6, medians via benchstat.Full Cardinal tick (ECS reconcile + physics step + writeback + events)
Workers: 18Serial float64 Go pays ~20-30% vs float32 C at the tick level; the deterministic worker pool closes that and beats CGO at 5000 bodies. Engine-only (no ECS overhead, no CGO counterpart existed at this layer): MixedRain 5000 bodies steps at 2.42ms serial / 1.05ms at 18 workers (2.3×), and the opt-in PGO profile takes a further −9.8% serial at the engine level.
Queries (per-op, dense grid scene)
Raycast at 5000 bodies: 1808ns → 924ns (−48.9%); CircleSweep at 100 bodies −50.9%; CircleSweep mid-sizes +17-32%; remaining rows are statistically noise on sub-µs micro-queries (high run-to-run variance). Geomean across all 16 query rows: +2.9% ≈ parity, with the big-scene raycast halved (no per-candidate CGO crossing).
CPU profiles, 5000-body tick
CGO (
pproftop):The boundary cost ~6× the step itself: every tick re-marshals all live contacts through CGO, and none of the C internals are attributable in profiles.
Pure Go:
The 32%-of-tick contact-gather tax is gone (direct slice access), the step is transparent to pprof/PGO, and the engine additionally guarantees byte-identical results across architectures and worker counts (goldens + no-FMA gate in CI) — a property the float32 C build never had on heterogeneous hosts.