Conversation
There was a problem hiding this comment.
Pull request overview
This PR experiments with a new cluster-based (“cell”) constraint solver pipeline, replacing the prior constraint-graph/coloring approach and refactoring how awake body state is packed and accessed during solving.
Changes:
- Introduces cluster computation/classification and new cluster-oriented solver stages.
- Refactors solver sets to store body IDs (vs. body sim/state arrays) and updates wake/sleep transfer logic accordingly.
- Updates multiple joint/contact solver paths and temporarily disables determinism assertions/tests.
Reviewed changes
Copilot reviewed 40 out of 40 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
src/cluster.c |
Computes body clusters and packs awake b2BodyState for the step. |
src/solver.c |
Integrates cluster-based stage scheduling and bullet processing buffers. |
src/solver.h |
Updates step context (cluster data, borders, bullet body list type, stage enums). |
src/solver_set.h |
Changes solver sets to store bodyIds array. |
src/solver_set.c |
Updates wake/sleep/merge/transfer logic for new solver set storage. |
src/contact_solver.h |
Defines scalar contact-constraint pipeline and contact constraint indexing contract. |
src/contact_solver.c |
Prepares/warm-starts/solves contact constraints using packed states. |
src/body.c |
Updates body creation and runtime state storage on b2Body. |
src/wheel_joint.c |
Switches wheel joint preparation/solve to use stateIndex indirection. |
src/weld_joint.c |
Switches weld joint preparation/solve to use stateIndex indirection; adds extra asserts. |
test/main.c |
Disables running determinism test in the test runner. |
test/test_determinism.c |
Comments out determinism validation checks. |
test/test_container.c |
Removes redundant array declaration from the container test. |
Comments suppressed due to low confidence (2)
src/contact_solver.c:111
- Warm-start currently does
int indexA = constraint->indexA - 1;/indexB = ... - 1;and then treatsindex == B2_NULL_INDEXas the dummy state. With the current prepare path storing rawstateIndex, this makesstateIndex==0become -1 and any other index off-by-one. This needs to be consistent with how indices are stored inb2PrepareContactConstraints(either base-1 everywhere or base-0 everywhere).
b2ContactConstraint* constraint = constraints + i;
int indexA = constraint->indexA - 1;
int indexB = constraint->indexB - 1;
b2BodyState* stateA = indexA == B2_NULL_INDEX ? &dummyState : states + indexA;
b2BodyState* stateB = indexB == B2_NULL_INDEX ? &dummyState : states + indexB;
src/solver.c:1559
b2StepContext::bulletBodiesis declared asb2Body**(solver.h), but this allocatesawakeBodyCount * sizeof(int). This will under-allocate on 64-bit and corrupt memory when storing pointers. AllocateawakeBodyCount * sizeof(b2Body*)(or change the field back toint*consistently).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/cluster.c
Outdated
| // First-time or too few bodies: full k-means with seeding | ||
| if ( manager->initialized == false || awakeCount < B2_CLUSTER_COUNT ) | ||
| { | ||
| int seedCount = b2MinInt( awakeCount, B2_CLUSTER_COUNT ); | ||
| for ( int i = 0; i < seedCount; ++i ) | ||
| { | ||
| b2Body* body = b2BodyArray_Get( &world->bodies, bodyIds[i] ); | ||
| clusters[i].center = body->center; | ||
| b2Array_Push( clusters[i].bodyIds, bodyIds[i] ); | ||
| body->clusterIndex = (int16_t)i; | ||
| } | ||
|
|
||
| if ( awakeCount < B2_CLUSTER_COUNT ) | ||
| { | ||
| return; | ||
| } |
There was a problem hiding this comment.
b2ComputeClusters returns early when awakeCount < B2_CLUSTER_COUNT after seeding, leaving context->states unallocated and leaving body->stateIndex unset for the awake bodies. Later solver/joint/contact code appears to assume context->states is valid and uses body->stateIndex, which would be stale/uninitialized in this path. Allocate/copy context->states (and set body->stateIndex) even in the small-awake-count case, or otherwise ensure downstream code is gated off when context->states is NULL.
| @@ -298,14 +276,15 @@ b2BodyId b2CreateBody( b2WorldId worldId, const b2BodyDef* def ) | |||
| body->islandId = B2_NULL_INDEX; | |||
| body->islandIndex = B2_NULL_INDEX; | |||
| body->bodyMoveIndex = B2_NULL_INDEX; | |||
| body->id = bodyId; | |||
| body->mass = 0.0f; | |||
| body->inertia = 0.0f; | |||
| body->sleepThreshold = def->sleepThreshold; | |||
| body->sleepTime = 0.0f; | |||
| body->minExtent = B2_HUGE; | |||
| body->maxExtent = 0.0f; | |||
| body->type = def->type; | |||
| body->flags = bodySim->flags; | |||
| body->enableSleep = def->enableSleep; | |||
| body->clusterIndex = B2_NULL_INDEX; | |||
There was a problem hiding this comment.
b2CreateBody is not initializing several new b2Body runtime fields (e.g., linearVelocity, angularVelocity, force, torque, and stateIndex). Since bodies are allocated from a pool and can be reused, these fields can contain stale data; additionally, the initial velocity from b2BodyDef is currently ignored. Initialize these fields explicitly (and set stateIndex to B2_NULL_INDEX until b2ComputeClusters assigns it).
| b2ContactSim* contactSim = contacts[i]; | ||
| b2Body* bodyA = bodies + contactSim->bodyIdA; | ||
| b2Body* bodyB = bodies + contactSim->bodyIdB; | ||
| int indexA = bodyA->stateIndex; | ||
| int indexB = bodyB->stateIndex; | ||
|
|
||
| const b2Manifold* manifold = &contactSim->manifold; | ||
| int pointCount = manifold->pointCount; | ||
|
|
||
| B2_ASSERT( 0 < pointCount && pointCount <= 2 ); | ||
|
|
||
| int indexA = contactSim->bodySimIndexA; | ||
| int indexB = contactSim->bodySimIndexB; | ||
|
|
||
| #if B2_ENABLE_VALIDATION | ||
| b2Body* bodyA = bodies + contactSim->bodyIdA; | ||
| int validIndexA = bodyA->setIndex == b2_awakeSet ? bodyA->localIndex : B2_NULL_INDEX; | ||
| B2_ASSERT( indexA == validIndexA ); | ||
|
|
||
| b2Body* bodyB = bodies + contactSim->bodyIdB; | ||
| int validIndexB = bodyB->setIndex == b2_awakeSet ? bodyB->localIndex : B2_NULL_INDEX; | ||
| B2_ASSERT( indexB == validIndexB ); | ||
| #endif | ||
|
|
||
| b2ContactConstraint* constraint = constraints + i; | ||
|
|
||
| // 0 is null | ||
| constraint->indexA = indexA + 1; | ||
| constraint->indexB = indexB + 1; | ||
| constraint->indexA = indexA; | ||
| constraint->indexB = indexB; | ||
| constraint->normal = manifold->normal; | ||
| constraint->friction = contactSim->friction; |
There was a problem hiding this comment.
b2PrepareContactConstraints assigns constraint->indexA/indexB directly from body->stateIndex, but b2ContactConstraint documents these indices as “base-1, 0 for null”. Downstream warm-start/solve subtracts 1, so an awake body with stateIndex == 0 will be treated as null. Store stateIndex + 1 (and ensure static bodies store 0) to match the base-1 convention, or remove the base-1 convention consistently.
| b2Body* bodyA = b2BodyArray_Get( &world->bodies, idA ); | ||
| b2Body* bodyB = b2BodyArray_Get( &world->bodies, idB ); | ||
|
|
||
| B2_ASSERT( bodyA->setIndex == b2_awakeSet || bodyB->setIndex == b2_awakeSet ); | ||
| b2SolverSet* setA = b2SolverSetArray_Get( &world->solverSets, bodyA->setIndex ); | ||
| b2SolverSet* setB = b2SolverSetArray_Get( &world->solverSets, bodyB->setIndex ); | ||
| base->stateIndexA = bodyA->stateIndex; | ||
| base->stateIndexB = bodyB->stateIndex; | ||
|
|
||
| int localIndexA = bodyA->localIndex; | ||
| int localIndexB = bodyB->localIndex; | ||
|
|
||
| b2BodySim* bodySimA = b2BodySimArray_Get( &setA->bodySims, localIndexA ); | ||
| b2BodySim* bodySimB = b2BodySimArray_Get( &setB->bodySims, localIndexB ); | ||
|
|
||
| float mA = bodySimA->invMass; | ||
| float iA = bodySimA->invInertia; | ||
| float mB = bodySimB->invMass; | ||
| float iB = bodySimB->invInertia; | ||
|
|
||
| base->invMassA = mA; | ||
| base->invMassB = mB; | ||
| base->invIA = iA; | ||
| base->invIB = iB; | ||
| float mA = bodyA->invMass; | ||
| float iA = bodyA->invInertia; | ||
| float mB = bodyB->invMass; | ||
| float iB = bodyB->invInertia; |
There was a problem hiding this comment.
base->stateIndexA/stateIndexB are copied from body->stateIndex for all bodies. For static bodies (and any body not in the awake cluster state pack), body->stateIndex must be B2_NULL_INDEX to avoid indexing into context->states with a stale/garbage value. Ensure b2Body.stateIndex is initialized to B2_NULL_INDEX for non-awake bodies (e.g., in body creation and when moving between solver sets), or compute these indices here based on body type/set membership instead of trusting body->stateIndex.
| b2Body* bodyA = b2BodyArray_Get( &world->bodies, idA ); | ||
| b2Body* bodyB = b2BodyArray_Get( &world->bodies, idB ); | ||
|
|
||
| B2_ASSERT( bodyA->setIndex == b2_awakeSet || bodyB->setIndex == b2_awakeSet ); | ||
| b2SolverSet* setA = b2SolverSetArray_Get( &world->solverSets, bodyA->setIndex ); | ||
| b2SolverSet* setB = b2SolverSetArray_Get( &world->solverSets, bodyB->setIndex ); | ||
| base->stateIndexA = bodyA->stateIndex; | ||
| base->stateIndexB = bodyB->stateIndex; | ||
|
|
||
| int localIndexA = bodyA->localIndex; | ||
| int localIndexB = bodyB->localIndex; | ||
|
|
||
| b2BodySim* bodySimA = b2BodySimArray_Get( &setA->bodySims, localIndexA ); | ||
| b2BodySim* bodySimB = b2BodySimArray_Get( &setB->bodySims, localIndexB ); | ||
|
|
||
| float mA = bodySimA->invMass; | ||
| float iA = bodySimA->invInertia; | ||
| float mB = bodySimB->invMass; | ||
| float iB = bodySimB->invInertia; | ||
|
|
||
| base->invMassA = mA; | ||
| base->invMassB = mB; | ||
| base->invIA = iA; | ||
| base->invIB = iB; | ||
| float iA = bodyA->invInertia; | ||
| float iB = bodyB->invInertia; |
There was a problem hiding this comment.
base->stateIndexA/stateIndexB are taken from body->stateIndex, but static/sleeping bodies won’t be part of context->states and must use B2_NULL_INDEX so warm-start/solve can safely use dummyState. Make sure b2Body.stateIndex is reliably set to B2_NULL_INDEX for non-awake bodies (including newly-created statics and any bodies moved out of the awake set), or derive the state index here instead of copying it blindly.
src/weld_joint.c
Outdated
| B2_ASSERT( b2IsValidVec2( vA ) ); | ||
| B2_ASSERT( b2IsValidFloat( wA ) ); | ||
| B2_ASSERT( b2IsValidVec2( vB ) ); | ||
| B2_ASSERT( b2IsValidFloat( wB ) ); |
There was a problem hiding this comment.
These assertions are duplicated back-to-back, which adds noise without increasing coverage. Remove the repeated block to keep validation concise.
| // RUN_TEST( DeterminismTest ); | ||
| RUN_TEST( DistanceTest ); | ||
| RUN_TEST( DynamicTreeTest ); | ||
| RUN_TEST( IdTest ); |
There was a problem hiding this comment.
DeterminismTest is commented out from the main unit test runner. Given this PR is a major solver refactor, disabling determinism coverage makes regressions much harder to catch. Prefer keeping this test enabled (or gating it behind an explicit build flag / platform check) rather than commenting it out unconditionally.
| } | ||
|
|
||
| enkiDeleteTaskScheduler( scheduler ); | ||
|
|
||
| ENSURE( data.sleepStep == EXPECTED_SLEEP_STEP ); | ||
| ENSURE( data.hash == EXPECTED_HASH ); | ||
| //ENSURE( data.sleepStep == EXPECTED_SLEEP_STEP ); | ||
| //ENSURE( data.hash == EXPECTED_HASH ); | ||
|
|
There was a problem hiding this comment.
The determinism test no longer validates the expected sleep step/hash because the ENSURE checks are commented out. If the expected values changed due to solver updates, it’s better to update the constants or explicitly gate the check with a flag, rather than silently skipping the assertions (which makes the test always pass).
| done = UpdateFallingHinges( worldId, &data ); | ||
| } | ||
|
|
||
| ENSURE( data.sleepStep == EXPECTED_SLEEP_STEP ); | ||
| ENSURE( data.hash == EXPECTED_HASH ); | ||
| //ENSURE( data.sleepStep == EXPECTED_SLEEP_STEP ); | ||
| //ENSURE( data.hash == EXPECTED_HASH ); | ||
|
|
There was a problem hiding this comment.
CrossPlatformTest’s determinism validation is currently disabled by commenting out the ENSURE checks. This makes the subtest non-verifying. Consider updating EXPECTED_* (if the new solver legitimately changes them) or gating the check, but keep an assertion so the test can fail on nondeterministic output.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 40 out of 40 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| { | ||
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
b2Body_SetLinearVelocity no longer wakes a sleeping body when a non-zero velocity is set. This can make the call a no-op for sleeping bodies (they won’t be simulated) and is a behavioral regression from typical Box2D semantics. Consider waking the body when setting a meaningful non-zero velocity (or documenting/renaming if intentional).
| if ( b2LengthSquared( linearVelocity ) > 0.0f ) | |
| { | |
| b2WakeBody( world, body ); | |
| } |
| @@ -808,18 +750,7 @@ void b2Body_SetAngularVelocity( b2BodyId bodyId, float angularVelocity ) | |||
| return; | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
b2Body_SetAngularVelocity no longer wakes a sleeping body when a non-zero angular velocity is set, which can prevent the body from ever advancing. Consider waking the body when setting a meaningful non-zero angular velocity (or clearly documenting the new behavior).
| if ( body->setIndex != b2_awakeSet ) | |
| { | |
| float maxVelocity = b2AbsFloat( angularVelocity ) * body->maxExtent; | |
| if ( maxVelocity >= body->sleepThreshold ) | |
| { | |
| b2WakeBody( world, body ); | |
| } | |
| } |
| // Array of bullet bodies that need continuous collision handling | ||
| int* bulletBodies; | ||
| b2Body** bulletBodies; | ||
| b2AtomicInt bulletBodyCount; |
There was a problem hiding this comment.
b2StepContext::bulletBodies is now a b2Body** (pointer array). Any arena allocations and indexing must use sizeof(b2Body*) and store b2Body* elements; allocating as sizeof(int) (or treating it as int*) will cause memory corruption on 64-bit builds.
| fprintf( file, "body id: %d\n", bodyIdCapacity * (int)sizeof( int ) ); | ||
| fprintf( file, "joint sim: %d\n", jointSimCapacity * (int)sizeof( b2JointSim ) ); | ||
| fprintf( file, "contact sim: %d\n", contactSimCapacity * (int)sizeof( b2ContactSim ) ); | ||
| fprintf( file, "island sim: %d\n", islandSimCapacity * (int)sizeof( islandSimCapacity ) ); |
There was a problem hiding this comment.
The memory stats print uses sizeof(islandSimCapacity) (an int) instead of the element type stored in set->islandSims. This under-reports memory and is likely unintended; use sizeof(b2IslandSim) (or the correct island-sim type) instead.
| fprintf( file, "island sim: %d\n", islandSimCapacity * (int)sizeof( islandSimCapacity ) ); | |
| fprintf( file, "island sim: %d\n", islandSimCapacity * (int)sizeof( b2IslandSim ) ); |
| bool drawClusterColor = true; | ||
| if ( drawClusterColor && body->clusterIndex != B2_NULL_INDEX ) | ||
| { | ||
| B2_ASSERT( 0 <= body->clusterIndex && body->clusterIndex < B2_CLUSTER_COUNT ); | ||
| color = b2_clusterColors[body->clusterIndex]; |
There was a problem hiding this comment.
Cluster-color rendering is hard-coded on (drawClusterColor = true) and overrides custom shape colors for clustered bodies. Consider wiring this to a debug draw option (similar to the removed drawGraphColors) or defaulting it off so existing visual/debug semantics aren’t changed unexpectedly.
src/contact_solver.c
Outdated
| b2TracyCZoneEnd( store_impulses ); | ||
| } | ||
|
|
||
| #if 0 |
There was a problem hiding this comment.
Large SIMD contact solver implementation is now kept in the file but disabled with #if 0. This adds substantial dead code/maintenance surface. Consider deleting it, moving it behind a real feature flag (e.g., B2_ENABLE_SIMD_CONTACT_SOLVER), or splitting it into a separate file to keep the active solver path clear.
| #if 0 | |
| // Optional SIMD contact solver implementation. Disabled by default unless explicitly enabled. | |
| #if defined( B2_ENABLE_SIMD_CONTACT_SOLVER ) && B2_ENABLE_SIMD_CONTACT_SOLVER |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 45 out of 45 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -38,7 +40,7 @@ void* b2AllocateArenaItem( b2ArenaAllocator* alloc, int size, const char* name ) | |||
| b2ArenaEntry entry; | |||
| entry.size = size32; | |||
| entry.name = name; | |||
| if ( alloc->index + size32 > alloc->capacity ) | |||
| if ( alloc->index + size32 > alloc->capacity || B2_FORCE_HEAP == 1 ) | |||
| { | |||
| // fall back to the heap (undesirable) | |||
| entry.data = b2Alloc( size32 ); | |||
There was a problem hiding this comment.
B2_FORCE_HEAP is hard-coded to 1, which forces all arena allocations to fall back to heap allocations (b2Alloc). This defeats the purpose of the arena allocator (performance, fragmentation control) and changes memory accounting/behavior globally. Suggest removing this override or gating it behind a debug-only compile flag (e.g., #if defined(B2_DEBUG) && B2_FORCE_HEAP).
| fprintf( file, "solver sets\n" ); | ||
| fprintf( file, "body sim: %d\n", bodySimCapacity * (int)sizeof( b2BodySim ) ); | ||
| fprintf( file, "body state: %d\n", bodyStateCapacity * (int)sizeof( b2BodyState ) ); | ||
| fprintf( file, "body id: %d\n", bodyIdCapacity * (int)sizeof( int ) ); | ||
| fprintf( file, "joint sim: %d\n", jointSimCapacity * (int)sizeof( b2JointSim ) ); | ||
| fprintf( file, "contact sim: %d\n", contactSimCapacity * (int)sizeof( b2ContactSim ) ); | ||
| fprintf( file, "island sim: %d\n", islandSimCapacity * (int)sizeof( islandSimCapacity ) ); |
There was a problem hiding this comment.
The memory stat for island sims multiplies by sizeof(islandSimCapacity) (i.e., sizeof(int)), which is unrelated to the element type and will report incorrect sizes. This should use sizeof(b2IslandSim) (or the actual island-sim element type stored in set->islandSims).
test/test_determinism.c
Outdated
| printf( "workers=%d sleepStep=%d hash=0x%08X\n", workerCount, data.sleepStep, data.hash ); | ||
|
|
||
| //ENSURE( data.sleepStep == EXPECTED_SLEEP_STEP ); | ||
| //ENSURE( data.hash == EXPECTED_HASH ); |
There was a problem hiding this comment.
The multithreaded determinism test no longer enforces determinism because the ENSURE checks are commented out. This will allow non-deterministic behavior to pass CI silently. Suggest re-enabling the assertions (and updating expected values if necessary) and keeping the printf behind a debug flag if you still want the diagnostic output.
| s_context.Load(); | ||
| s_context.workerCount = b2MinInt( 8, (int)enki::GetNumHardwareThreads() / 2 ); | ||
| s_context.workerCount = 1; | ||
|
|
There was a problem hiding this comment.
s_context.workerCount is computed from hardware threads and then immediately overridden to 1. If this was for debugging, it will unintentionally disable multithreading in the sample app for everyone. Suggest removing the forced assignment or making it conditional (e.g., a command-line flag / debug option).
Experiment with clustering (cell) solver