refactor(camera): share property list between init and clone#8878
Merged
Conversation
Hoist the property list from initializeComponentData to a module-level _properties array (matching light/gsplat/render) and drive cloneComponent from the same list, so the two can no longer drift. The lists had already drifted: cloneComponent was missing fog and clearDepth, so cloning an entity silently dropped those camera settings. The new CameraComponent unit test demonstrated the bug before the fix and now locks in clone behavior for all shared properties. Also drops the unused legacy `properties` parameter from the initializeComponentData override. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Refactors the CameraComponentSystem to drive both initialization and cloning from a single shared property list, preventing drift between the two code paths and fixing previously-missed camera settings during entity cloning.
Changes:
- Hoists camera component property names to a module-level
_propertieslist and reuses it ininitializeComponentDataandcloneComponent. - Fixes cloning drift so
fogandclearDepthare now cloned as well. - Adds a new
CameraComponenttest suite covering defaults and clone behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/framework/components/camera/system.js |
Centralizes the camera property list and uses it for both initialization and cloning to prevent future drift. |
test/framework/components/camera/component.test.mjs |
Adds new unit tests for camera defaults and cloning behavior (with a few suggested additions to better cover all cloned properties). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Address review feedback: set non-default values for calculateProjection, calculateTransform, gammaCorrection and toneMapping in the clone round-trip test so regressions in cloning those fields are caught. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
willeastcott
added a commit
that referenced
this pull request
Jun 11, 2026
…sts (#8879) Button, scroll-view, scrollbar and collision systems already define a module-level _properties array used by initializeComponentData, but each duplicated the same list as a hand-written object literal in cloneComponent. Build the clone data from the shared list instead (the camera pattern from #8878) so the two paths can no longer drift. Collision keeps its two intentional differences explicit: type is initialized outside the property list, and shape is excluded because the type implementation creates it during initialization. No behavior changes - all four clone literals matched their lists 1:1. Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
willeastcott
added a commit
that referenced
this pull request
Jun 11, 2026
…body (#8880) 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>
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.
Summary
First in a planned series of small PRs improving structural consistency across
framework/components, continuing #8876 / #8873.Most component systems duplicate their property list between
initializeComponentDataandcloneComponent(an inline array in one, a hand-written object literal in the other). The camera system shows why that matters: the two lists had drifted —fogandclearDepthwere initialized but missing from the clone literal, so cloning an entity silently dropped those camera settings.This PR:
_propertiesarray (the pattern already used by light, gsplat, render and particle-system) and drives bothinitializeComponentDataandcloneComponentfrom it, so the lists can no longer drift.fogandclearDepth. This is the one intentional behavior change in the PR.test/framework/components/camera/component.test.mjs(no camera component test existed). The clone round-trip test was written first and failed onclearDepth/fogbefore the system change; it now passes and locks in clone behavior for all shared properties.propertiesparameter from theinitializeComponentDataoverride.Everything else is behavior-preserving: init order, type conversions (
rect/scissorRect/clearColorarrays) and clone reference semantics are unchanged.Planned follow-ups (separate PRs, in order)
_properties(button, scroll-view, scrollbar, collision)beforeremovehandler naming across all systemsTesting
CameraComponenttests: defaults, full clone round-trip, enabled-state clonenpm testsuite passes (1817 passing)🤖 Generated with Claude Code