refactor(collision): deduplicate rigid body / trigger rebuild#8886
Conversation
The block that rebuilds the rigid body or re-creates the trigger appeared nearly verbatim in CollisionSystemImpl.recreatePhysicalShapes and CollisionMeshSystemImpl.doRecreatePhysicalShape. Extract the two bodies into _recreateBody and _recreateTrigger on the base implementation while keeping the branch conditions at each call site, since they differ observably: the base path skips trigger creation for compound children, the mesh path does not (a mesh collider can be a compound child via _addEachDescendant). Behavior is preserved by construction. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
I am not sure I see the value in this PR. Is it to reduce the code base? +25 lines vs -22. |
There was a problem hiding this comment.
Pull request overview
Refactors collision shape rebuild logic to remove duplicated rigidbody/trigger recreation blocks across collision system implementations, while preserving behavioral differences in when triggers are created.
Changes:
- Extracted rigidbody rebuild logic into
CollisionSystemImpl._recreateBody(entity). - Extracted trigger create/re-init logic into
CollisionSystemImpl._recreateTrigger(entity, component). - Updated both the base collision implementation and mesh collision implementation to use the shared helpers, with an explicit comment documenting the mesh-vs-base trigger gating difference.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Fair question. The goal isn't line count. The two copies of this block had actually drifted: the base path skips trigger creation for compound children ( It also gives the disableSimulation → createBody → enableSimulation sequence a single home, so any future change to the rebuild ordering (there's a known latent bug in this area) gets made once instead of twice. Happy to close it if you don't think that's worth the indirection, though |
|
I usually de-duplicate to keep all paths consistent even if its just few lines. I'd merge. |
Continues the component-consistency series (#8871, #8876, #8878-#8882). Sibling of #8883 and #8884.
Changes
The block that rebuilds the rigid body or re-creates the trigger after a collision shape change appeared nearly verbatim in two places:
CollisionSystemImpl.recreatePhysicalShapesCollisionMeshSystemImpl.doRecreatePhysicalShapeThis extracts the two bodies into
_recreateBody(entity)and_recreateTrigger(entity, component)on the baseCollisionSystemImpl, while deliberately keeping the branch conditions at each call site.Why the gating stays at the call sites
The two call sites are not identical: the base implementation skips trigger creation for compound children (
else if (!component._compoundParent)), while the mesh implementation creates the trigger unconditionally. That difference is observable behavior - a mesh collider can be a compound child, since_addEachDescendantassigns_compoundParentto any descendant collision without a rigidbody regardless of type. Extracting only the bodies preserves behavior by construction; the asymmetry is now called out with a comment on the mesh side.Verified with
npm test/npm run lint/npm run build/npm run build:types+npm run test:types. The base-implementation block sits behind the Ammo gate, so unit tests cannot reach it; the mesh trigger branch is exercised Ammo-less by the existing model tests.🤖 Generated with Claude Code