Skip to content

refactor(components): hoist property lists in model, sound and rigid-body#8880

Merged
willeastcott merged 1 commit into
mainfrom
refactor/shared-properties-lists
Jun 11, 2026
Merged

refactor(components): hoist property lists in model, sound and rigid-body#8880
willeastcott merged 1 commit into
mainfrom
refactor/shared-properties-lists

Conversation

@willeastcott

Copy link
Copy Markdown
Contributor

Summary

Third PR in the component-consistency series (follows #8878, #8879).

Model, sound and rigid-body each defined their property list inline inside initializeComponentData and duplicated it as a hand-written object literal in cloneComponent. This PR hoists each list to a module-level _properties array and drives both methods from it.

Behavior-preserving — each clone literal was verified against its init list before converting, and all special-case handling survives:

  • model: clone skips material/materialAsset in the loop (the existing default-material/asset resolution logic below is untouched) and still shallow-copies mapping via extend. The post-addComponent model/mesh-instance/customAabb cloning is unchanged. Init order (which the original commented on) is preserved in the hoisted list.
  • sound: slots are still converted back to plain option objects inside the loop; the other 7 properties matched 1:1.
  • rigid-body: the only mechanical difference — linearFactor/angularFactor previously round-tripped through [x, y, z] arrays which init converted back to Vec3. The loop now passes the component's Vec3s directly; the component setters .copy() their input, so the end state is identical and nothing is aliased.

Also drops the unused legacy properties parameter from the three initializeComponentData overrides.

Testing

  • Model component tests pass unchanged (15 passing, includes clone coverage)
  • Full npm test suite passes (1817 passing)
  • ESLint clean on changed files

Remaining PRs in the series

  1. Animation system (isolated due to its post-add clone assignment)
  2. Standardize beforeremove handler naming across all systems

🤖 Generated with Claude Code

…body

Continue the pattern from #8878/#8879: hoist the inline property list in
initializeComponentData to a module-level _properties array and drive
cloneComponent from the same list, so the two paths cannot drift.

Special-case handling is preserved:
- model: material/materialAsset are still resolved by the dedicated
  clone logic, mapping is still shallow-copied via extend
- sound: slots are still converted back to plain option objects
- rigid-body: linearFactor/angularFactor now pass the component Vec3s
  directly instead of converting to arrays; the component setters copy
  values, so the end state is identical with no aliasing

Also drops the unused legacy properties parameter from the
initializeComponentData overrides.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Refactors the model, sound, and rigid-body component systems to hoist their component property lists into module-level _properties arrays, using the shared list to drive both initializeComponentData and cloneComponent to avoid drift and duplication.

Changes:

  • Hoisted inline property arrays to module-level _properties constants in model, sound, and rigid-body systems.
  • Reworked cloneComponent implementations to build clone data from _properties (preserving special-case handling for model materials/mapping and sound slots).
  • Removed the unused legacy properties parameter from the three initializeComponentData overrides.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
src/framework/components/sound/system.js Hoists _properties, uses it for initialization and cloning; preserves slot option-object conversion during clone.
src/framework/components/rigid-body/system.js Hoists _properties, uses it for initialization and cloning; clone now passes factor Vec3s directly.
src/framework/components/model/system.js Hoists ordered _properties, uses it for initialization and cloning while preserving material/materialAsset special-casing and mapping shallow-copy.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@willeastcott willeastcott merged commit 4addd81 into main Jun 11, 2026
10 checks passed
@willeastcott willeastcott deleted the refactor/shared-properties-lists branch June 11, 2026 19:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement Request for a new feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants