refactor(collision): drop shape from shared properties, modernize initialize/clone#8883
Merged
Merged
Conversation
…tialize/clone `shape` is runtime state created and owned by the collision type implementation, never component data: for non-mesh types a user-supplied shape was immediately overwritten by beforeInitialize, and for mesh types it was destroyed (or threw in destroyShape when a non-Ammo object was passed). Removing it from the shared `_properties` list lets cloneComponent become the unconditional loop used by the other refactored systems. Also modernizes initializeComponentData: drops the unused third parameter, replaces the slice/splice filtering of conflicting mesh inputs with filter, and switches both loops to for...of. The explicit `!== undefined` check is retained (and test-pinned) so class-field defaults survive explicitly-undefined data values. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This was referenced Jun 12, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
This PR continues the component-consistency refactors for the collision system by treating shape strictly as runtime-owned state (not component data), simplifying init/clone code paths to match other modernized component systems, and adding tests to lock in the behavior.
Changes:
- Remove
shapefromCollisionComponentSystem’s shared_propertieslist so it is no longer initialized/cloned from user data. - Modernize
initializeComponentDataby dropping the unused third parameter, usingfilterfor mesh input conflict resolution, and switching tofor...ofloops. - Add/extend tests to verify
shapepassed viaaddComponentdata is ignored (including mesh type) and that clones haveshape === null.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/framework/components/collision/system.js | Removes shape from shared init/clone properties and simplifies initialization/cloning loops. |
| test/framework/components/collision/component.test.mjs | Adds regression tests asserting shape is ignored from init data and remains null on clones. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ilimtvtr-alt
approved these changes
Jun 13, 2026
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
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.
Continues the component-consistency series (#8871, #8876, #8878-#8882).
Changes
'shape'from the shared_propertieslist inCollisionComponentSystem.shapeis runtime state created and owned by the collision type implementation, never component data, socloneComponentno longer needs its special-case skip and becomes the unconditional loop used by the other refactored systems.initializeComponentData: drops the unused third parameter, replaces the slice/splice filtering of conflicting mesh inputs (assetvsmodel/render) withfilter, and switches both loops tofor...of, matchingRigidBodyComponentSystem.shape === nullassertion to the existing clone test.Why this is behavior-preserving
shapein theaddComponentdata was always immediately overwritten byCollisionSystemImpl.beforeInitialize, so ignoring it is identical.shapewas eitherAmmo.destroy'd during the initial rebuild or, with a non-Ammo object, threw in the mesh implementation'sdestroyShape(getNumChildShapesis not a function). One of the new tests fails onmainfor this reason - the change turns that incidental crash into a silent ignore.cloneComponentalready skippedshape, so clone output is unchanged.!== undefinedcheck is retained (test-pinned) so class-field defaults survive explicitly-undefined data values.The undocumented public
shapesetter onCollisionComponentis intentionally left in place - removing it would be an observable API change (strict-mode throw on assignment, types-visible).🤖 Generated with Claude Code