feat(box2d): native Go port of Box2D v3.2.0, swap physics2d off CGO - #928
Open
smsunarto wants to merge 37 commits into
Open
feat(box2d): native Go port of Box2D v3.2.0, swap physics2d off CGO#928smsunarto wants to merge 37 commits into
smsunarto wants to merge 37 commits into
Conversation
|
Claude finished @smsunarto's task in 12m 24s —— View job Review — round 5 (
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
❌ 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 |
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 |
smsunarto
force-pushed
the
scott/box2d-go-cardinal-ecs-ce311c
branch
from
August 4, 2026 03:33
c778ac9 to
56070bd
Compare
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
smsunarto
force-pushed
the
scott/box2d-go-cardinal-ecs-ce311c
branch
from
August 4, 2026 05:00
56070bd to
dbf051e
Compare
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>
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>
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>
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>
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>
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>
…stages The six per-color contact-solver stages shared one dispatch grain (solverColorGrain = 32), but only solve and relax do enough work per item to cover a barrier. Wall-clock stage instrumentation (MixedRain 5000 bodies, 600 steps, Apple M5 Max, 18 workers) showed the other four losing to serial at that grain — warm start 0.1444 -> 0.2387 ms, prepare contacts 0.0603 -> 0.0728, restitution 0.0339 -> 0.0575, store impulses 0.0368 -> 0.0540 — because each per-color dispatch pays a ~6-10 us barrier that exceeds the stage's own work. Give those stages (plus prepare joints) their own grain, solverLightColorGrain = 1024, so forRangeWorkers keeps them on the dispatching goroutine until a color is genuinely large. Solve and relax stay on solverColorGrain = 32, where parallel wins 1.56x. The value was swept end to end over 32/64/128/256/512/1024/2048/1<<30, runs interleaved across candidates (a sequential sweep let build order masquerade as an effect), 10 reps of -benchtime=50x each at Workers_18. Every candidate >= 64 gains ~5.5% on StepPyramid; MixedRain 1000 and 5000 are flat everywhere; a denser pyramid (colors of 322-409 items) separates them, with 64 gaining only 8.4% where 256/1024/1<<30 gain 12-14% — a grain of 64 wins the 20-row pyramid only because its colors happen to fall under 2*64. Among the tied candidates, 1024 matches the ~3.7 ns per item cost of these stages against the barrier and still lets colors of 2048+ items dispatch. Determinism is unaffected: grain changes only how many workers engage, and per-color partitions are byte-identical by the graph coloring. None of the retuned stages write per-worker state, so the presize/dispatch/ merge invariant is untouched — the only per-color stage with a per-worker output is solve/relax (jointStateBitSet), and maxColorWorkers still derives its bound from the same (itemCount, solverColorGrain) pair. Gates: go build ./...; go test ./pkg/box2d/; go test -race -run 'TestGolden(Step|Continuous)|TestWorkerStress|TestPool' ./pkg/box2d/; go test ./pkg/plugin/physics2d/...; go test -run TestNoFusedMultiplyAdd ./pkg/box2d/; golangci-lint run ./pkg/box2d/... — all pass, goldens unchanged. benchstat n=6 -benchtime=50x, PRE vs POST, interleaved: StepPyramid/Workers_18 278.4µ -> 263.6µ -5.30% (p=0.041) every other row ~ (no significant change) allocs/op and B/op unchanged Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…eep events Two behavior divergences from upstream v3.2.0 found by the C-parity audit: - PrismaticJointSpeed panicked when called from a step-time callback (locked world). Upstream b2PrismaticJoint_GetSpeed is the one joint accessor that deliberately bypasses the world-lock early-out (b2GetWorld + b2GetJointSim) so it stays readable during callbacks; the port routed it through getJointSimCheckType, which returns nil under lock. Resolve the joint without the lock check, matching C. - transferBody blanket-invalidated bodyMoveIndex whenever a body left the awake set, suppressing legitimate FellAsleep notifications after a SetBodyType or DisableBody round trip within one post-step window. Upstream b2TransferBody does not touch bodyMoveIndex. Restore parity at the source and make the trySleepIsland consumer stale-safe instead (bounds + body-identity check before stamping FellAsleep), which also guards the out-of-bounds read upstream is exposed to. Also document that WorldDef.EnableContactSoftening is inert in this port: upstream reads it only in the SIMD prepare path, which the scalar-only port does not transliterate. Both fixes carry regression tests proven to fail without the change. Golden determinism files are byte-identical. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
NaN-poison testing proved the solver never reads a stale field (prepare initializes everything it reads for points < pointCount), but removing the clear showed no win above the noise floor in interleaved min-of-6 A/B runs, so the defensive clear stays. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
/code-review of the last five commits found no shipped correctness bug, but three checks had quietly stopped checking. The cross-architecture golden step was changed in place to -pgo, so the default no-profile build lost golden coverage on amd64 and arm64. Split it back into two steps; the PGO run stays as the second one. The light per-color stages moved to a grain of 1024, which put their multi-worker path out of reach of every scene in the suite: prepare joints, prepare contacts, warm start, apply restitution and store impulses now ran inline everywhere, including under -race. Add a scene that packs 2200 joints and 2200 contacts into ONE color, which works because a static-vs-dynamic constraint claims only the dynamic body's bit, so thousands of boxes on one ground and thousands of bobs on static anchors all pack together. TestWorkerStressLightStageColorMatches Serial is the byte-identity gate (its name matches the CI -race pattern already), and an internal test asserts the premise against the constraint graph so the suite cannot silently fall back to the inline path. Cost: the -race step goes 6.5s -> 8.2s. Growing the existing stress scene instead would have cost about a minute. Three solver comments still claimed presize == dispatch == merge after the grain split. The bound is merely >= now, and it holds only while the light grain stays above the heavy one, so state that and make it a compile error rather than a comment. Also: rename TestBodyMoveIndexInvalidatedOnAwakeSetExit, which named a mechanism the shipped fix deliberately does not use; rewrite the E14a fuzz note as fixed; record the locked-world accessor exemption on getJointSimCheckType, where it is enforced; correct the dynamic_tree header, which claimed int in every local while five locals are int32; and drop -a from the PGO no-FMA builds (3.62s -> 2.89s measured). Goldens are byte-identical: testdata is untouched. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…CS memory
Second half of the post-review remediation. Every change is
byte-identical: the physics2d golden trace and the box2d goldens pass
unmodified.
gatherLiveContacts sorted the tracked body ids on every call to buy a
determinism the result never needed. World.BodyContactData fills
ShapeIDA/ShapeIDB and the manifold from the contact record, not from the
endpoint that asked, so both endpoints of a pair produce a
byte-identical ContactPairInfo and first-wins dedupe cannot depend on
iteration order; every consumer is order-free too. Drop the sort, the id
gather and the liveIDsScratch field, and iterate rt.Bodies directly.
Box2D reports each contact twice, once per endpoint, and the old dedupe
ran after two shapeIdentity resolutions had already spent six world
lookups on the copy that was about to be discarded. Stamp contact
indices in a generation-stamped []uint32 and skip before resolving
identities. The map check stays as a backstop, so a stamp bug can only
cost work, never drop a pair.
BenchmarkStep, Apple M5 Max, n=6, benchstat vs the previous commit:
Bodies_100 -3.5% (p=0.002)
Bodies_500 -4.0% (p=0.002)
Bodies_1000 -5.4% (p=0.002)
Bodies_5000 ~ (p=0.937)
geomean -3.3% allocations flat
gatherLiveContacts also returned a map aliasing runtime scratch under a
comment-only contract. It takes a destination now, which lets the
adopt path gather straight into ActiveContacts and removes the round
trip through a second map.
Separately, the pipeline system kept its two per-tick gather buffers in
a closure and truncated them with a bare [:0]. WritebackEntry holds
cardinal.Refs and PhysicsRebuildEntry holds each collider's shape
slice, so everything past the new length went on pinning archetype
storage for entities that no longer existed, out of Reset()'s reach.
Move both onto the Runtime with the rest of the per-tick scratch, clear
the tail after each gather, and state the rule on the struct. The init
path shares the buffer now, which also restores the pre-size it lost.
Also drops a gosec suppression in the reconcile binary search by
computing the midpoint as lo+(hi-lo)/2, and replaces the benchWorkers
global with a threaded parameter.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… pin Address three PR review threads: Assert net (dead code in every build): debugAsserts now lives behind a build-tag pair (core_asserts_off.go / core_asserts_on.go) and the determinism workflow runs the full suite with -tags box2d_asserts on both architectures, so the ~hundreds of internal invariant checks finally execute somewhere. Tests that pin release-build guard semantics (B2_ASSERT-then-ignore in C) skip or fork under the tag via a buildWithAsserts mirror; foreign-id destroys and the degenerate hull gained tagged twins pinning the panic tier. Wiring the net in exposed three latent issues, all fixed here: - destroyArena asserted allocation == 0, which upstream b2DestroyArenaAllocator does not; it fired on the supported Destroy-after-callback-panic path. - the RotationBetweenUnitVectors oracle's float64 loop accumulator crosses zero at ~7.5e-16, so the x==0&&y==0 guard missed the singular point and that iteration compared zero against zero vacuously. - TestOracleShapeHelpersDefaultArm treated every default arm as release-only when only two assert in C; the assert-free arms now stay covered in both builds. Scratch retention: reconcileSortScratch was the one Runtime scratch buffer still truncated with a bare [:0]; it now goes through clearScratchTail so a shrinking scene cannot pin shape/vertex memory of destroyed entities. Contact stamp direction: the map-check comment claimed a stamp bug could only cost work, but the check only covers stamp false negatives — a false positive drops the pair before it. Rewrote the comment to state which side each mechanism holds, documented PackContactID slot 0 (1-based dense index) as a contract, and added a slot-discriminating assertion to TestOracleContactID_CReference: its round-trip checks were invariant under a coordinated swap of the index1/generation slots, so the layout the physics2d seen-stamp depends on was not actually pinned. Golden determinism files are byte-identical; both build variants green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…build Address two PR review threads on c4a4b2a: assert(cond) evaluates its argument even though the check compiles out, and the seven multi-line validators (IsValidVec2/AABB/Rotation/Transform/ Ray/Plane, ValidateHull) exceed the inline budget, so the release binary kept real calls in per-step paths (finalizeBodiesTask per body, MoveProxy/EnlargeProxy per moved proxy, every local raycast) that upstream's `((void)0)` release macro never evaluates. Wrap those ~30 call sites in `if debugAsserts { ... }` so the build-tag const folds the evaluation away, and document the per-site cost rule on assert and in core_asserts_off.go. Release codegen now shows no validator calls outside the always-on requireValidDefField tier (verified with -gcflags=-S); golden files are byte-identical, both build variants, no-FMA and race gates green. Also restore both-tier coverage for the two asserting default arms in TestOracleShapeHelpersDefaultArm: the box2d_asserts build now pins that computeShapeAABB and makeShapeDistanceProxy panic (matching shape.c:650 and :1020) instead of skipping them, mirroring how every other tag fork in c4a4b2a pins both tiers. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A panic out of a user callback unwinds past Step's unlock, latching w.locked forever: every later Step hit the silent reentrancy early-out and the world froze with no error anywhere (or tripped a bare assert under box2d_asserts). A server with panic-recovery middleware around its tick would keep calling Step and silently stop simulating. Recover-and-continue is not an option for this engine: a world resumed from a half-integrated step would silently diverge from every deterministic replica, and determinism is the product. So make the poison model explicit and loud. A completion-flag defer (catches runtime.Goexit too, and re-raises the original panic value untouched) marks the world stepPanicked; the next Step panics in every build with a message naming the cause and the remedy, checked before the reentrancy guard so the latched lock cannot mask it. Destroy stays valid on a poisoned world (the destroyArena side of this unwind path landed in c4a4b2a). The new regression test re-steps the SAME panicked world — the case TestPanicInCallbackPropagatesOriginalValue (Destroy + fresh world) never covered — on both the serial and pool unwind paths, and fails without the fix. Golden files are byte-identical; both build variants, race and wrapper gates green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
smsunarto
force-pushed
the
scott/box2d-go-cardinal-ecs-ce311c
branch
from
August 5, 2026 04:05
e18c7c0 to
34ec774
Compare
e18c7c0 made Step fail loudly on a poisoned world, but the latched world lock also gates every query and event accessor, and those guards return a zero value: a poisoned world answered raycasts with no hits and overlaps with no shapes — a wrong answer, not a no-op. A recovering game loop reads (line of sight, triggers, event drain) before it steps, so it would act on "nothing is there" for a tick before the Step panic ever fired — the same silent divergence the poison exists to prevent, one call earlier. Hoist the poison check into a shared panicIfPoisoned helper with one message const, and call it first in Step, the seven world queries (OverlapAABB/OverlapShape/CastRay/CastRayClosest/CastShape/CastMover/ CollideMover), the four event accessors, and Draw — before the assert(!w.locked) reentrancy guards, so the box2d_asserts build reports the real cause instead of a bare assertion failure. The genuinely reentrant path (a query from inside a step callback) is unchanged: stepPanicked is false there, and upstream's identical locked guards are only reachable reentrantly because C's flag cannot latch. The regression test now also drives CastRay, OverlapAABB, BodyEvents and ContactEvents on the poisoned world and requires the explicit message; the read-path half fails without this fix. Golden files are byte-identical; both build variants, race and wrapper gates green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
35320c8 covered the world-level queries, event accessors and Draw, but body.go and shape.go carry nine more read paths with the identical latched-lock zero-value guard: BodyContactCapacity, BodyContactData, ComputeBodyAABB, ChainSegmentCount, ChainSegments, ShapeContactCapacity, ShapeContactData, ShapeSensorCapacity and ShapeSensorData. The consequence was live in this repo: physics2d builds its contact gather from BodyContactCapacity+BodyContactData, so a poisoned world gathered an empty live set and FlushBufferedContacts emitted a spurious ContactEnd for every persisted touching pair — a wrong answer written into game state one tick before the Step panic. ComputeBodyAABB was worse than a zero count: an empty AABB at the origin is a plausible-looking value a caller culls with. panicIfPoisoned is now the first statement of all nine, and its doc states the contract line this closes on: Step and every read path fail loudly on a poisoned world; the mutators deliberately keep their plain locked-guard shape, because a recovered tick cannot complete without hitting a read or Step, and threading the check through ~30 setters buys no additional safety. The regression test now also drives BodyContactCapacity, BodyContactData and ComputeBodyAABB on the poisoned world and fails without the fix. Golden files byte-identical; both build variants, race and wrapper gates green. 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 the Go engine is still behind: +2.4% at 100 bodies (small enough that it disappears into run-to-run noise), +9.8% at 500, +9.6% at 1,000, +11.6% at 5,000 — it does double-precision math where the C build did single. The widest gap is 0.6 ms on a 16.7 ms frame. Real, but not something a player can see.
The C bridge ran on a single core. The Go engine steps on all of them, and produces byte-identical results whether it runs on one core or many. On an 18-core machine, full game tick:
At 1,000 bodies all three are 6–7% of a frame. At 5,000 both engines take roughly a third of a frame, and the Go one with workers is the faster of the two. Small scenes stay on the single-core path by design — splitting work across cores costs more than it saves until there are enough bodies to go around.
Queries got cheaper in the big scenes that matter: raycasting a 5,000-body world is less than half the cost it was. And the Go engine allocates 27–40% less memory per tick, which a long-running server feels as less garbage-collection pressure even where the tick timings look similar.
"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 benchmark scene and driver on both sides (pkg/plugin/physics2d/test, falling-circles),-benchtime=50x -count=6, medians, significance via benchstat. The surrounding Cardinal ECS differs between the two branches, so tick totals are comparable but per-function ECS costs are not.These numbers supersede the earlier round in this PR, which was measured before
b844d470(per-worker scratch for the continuous sweep),a02ccd3c(int32 tree links,TreeNodedown to one cache line) andb30339a7(per-tick scratch reuse in reconcile/contact gather).Full Cardinal tick (ECS reconcile + physics step + writeback + events)
Workers: 18All deltas except the 100-body row are p=0.002 (n=6). Serial float64 Go pays +2.4% to +11.6% at the tick level; the earlier "20–30%" figure in this PR predates the three commits above and is retired. The worker pool closes the remainder and beats CGO at 5000 bodies.
Caveat on this table, stated because it flatters us: the bench harness runs with
Debug: true, which makes Cardinal serialize world state every tick (persistState/ToProto, ~3 ms at 5000 bodies). That work is identical on both sides and dilutes the physics difference. WithDebug: false— closer to a production shard — the physics-only gap is larger:Workers: 18So: single-core the C build is genuinely 22–27% faster on physics (float32 arithmetic and a SIMD contact solver); the worker pool closes that and wins at 5000 bodies. Memory goes the other way by a wide margin — with
Debug: falseat 5000 bodies CGO allocates 4.17 MB per tick to marshal contacts across the boundary, against 440 B here.Memory moved the other way at every size — same allocation count (±0.2%), materially fewer bytes:
Engine-only (
pkg/box2d, no ECS overhead)No CGO counterpart exists at this layer, so this is informational for our side only — not a float32-vs-float64 engine comparison:
Workers: 18Pyramid is the worst case for the worker pool and worth stating plainly: one dense stack is a single constraint island, so the per-color stages have little to split while still paying every dispatch barrier.
Workersis a throughput knob for large or many-island scenes, not a default — results are byte-identical either way, so setting it wrong costs time, never correctness.For scale on what the optimizations bought at the engine layer: before
b844d470, MixedRain 5000 ran 2.395ms serial / 1.086ms at 18 workers (night_base_engine.txt). An intermediate run afterb844d470alone still measured 2.375ms, so essentially the entire ~30% serial gain comes froma02ccd3c— the 64-byte tree node — andb30339a7is wrapper-side only.Queries (per-op, dense grid scene)
Read this with the noise floor in mind: these are sub-microsecond operations and the small-scene rows swing run to run, which is the most likely reading of CircleSweep's ±50% spread across adjacent sizes. What survives is the 5000-body column — large-scene queries get materially cheaper, most likely because query results no longer cross the language boundary per candidate, though the profiles locate boundary cost in the step's contact gather rather than inside the query calls, so the mechanism is inferred rather than measured.
CPU profiles, 5000-body tick (
-benchtime=400x)CGO (
pproftop; 4.81s total samples):The boundary crossing costs ~4.5× the step itself: every tick re-marshals all live contacts through CGO, and none of the C internals are attributable in profiles.
Pure Go (2.94s total samples):
Every frame is attributable to a named Go function, so the step is transparent to pprof, PGO and the race detector. In the same profile,
cardinal.(*World).persistState/ecs.ToProtois 39.46% cum — on the Go side, physics is no longer the dominant tick cost. The bench harness runs with Debug enabled, which forces state serialization every tick, so that share is partly a harness artifact; it is not physics, not comparable across the two branches, and is tracked as separate follow-up work.On determinism, stated as what is enforced rather than what is hoped: CI checks the golden traces byte-for-byte on amd64 and arm64 at
WorkerCount2/4/8, plus an instruction-level no-FMA gate that also runs under PGO codegen. The C build's cross-host behaviour was not measured here, so no claim is made about it.