Skip to content

refactor(animation): share property list between init and clone#8881

Merged
willeastcott merged 2 commits into
mainfrom
refactor/animation-shared-properties
Jun 11, 2026
Merged

refactor(animation): share property list between init and clone#8881
willeastcott merged 2 commits into
mainfrom
refactor/animation-shared-properties

Conversation

@willeastcott

Copy link
Copy Markdown
Contributor

Summary

Fourth PR in the component-consistency series (follows #8878, #8879, #8880). Animation got its own PR because its clone path was structured differently from every other system.

AnimationComponentSystem.cloneComponent previously added an empty component and then assigned properties onto it post-add — duplicating the property list and, notably, assigning assets before activate/enabled, the opposite of the order initializeComponentData documents as required ("assets needs to be last as it checks other properties to see if it should play").

This PR:

  • Hoists the ordered list to a module-level _properties array (with the ordering comment) and drives both initializeComponentData and cloneComponent from it.
  • Routes the clone data through addComponent like every other system, so clone applies properties in the documented init order.
  • Keeps the post-addComponent copying of the loaded animations / animationsIndex dictionaries, and the assets.slice() so the clone doesn't share the source's array.
  • Drops the unused legacy properties parameter.

On behavior: the ordering change (assets-first → assets-last) is provably unobservable. The auto-play gate in onSetAnimations requires this.entity.enabled, and GraphNode#enabled returns _enabled && _enabledInHierarchy — which is always false for the detached clone entity while cloneComponent runs, in both the old and new code paths. Auto-play decisions happen later, when the clone is added to the hierarchy (onEnable), by which point all properties were copied in both versions.

Testing

  • Animation component tests pass unchanged, including the async clone auto-play test
  • Full npm test suite passes (1817 passing)
  • ESLint clean

Remaining PRs in the series

  1. Standardize beforeremove handler naming across all systems

🤖 Generated with Claude Code

Hoist the ordered property list to a module-level _properties array and
build cloneComponent data from it, routing the clone through
addComponent like every other component system instead of assigning
properties after adding an empty component.

This also means clone now applies properties in the documented init
order (activate/enabled before assets) rather than assets-first. The
difference is not observable: the auto-play gate in onSetAnimations
requires entity.enabled, which is always false for the detached clone
entity while cloneComponent runs, in both the old and new code paths.

The loaded animations and animationsIndex dictionaries are still copied
onto the clone after addComponent, as before. Also drops the unused
legacy properties parameter from initializeComponentData.

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 AnimationComponentSystem to share a single ordered property list between initialization and cloning, ensuring clone-time property application follows the same documented order as initialization (notably assets last).

Changes:

  • Hoists the ordered init/clone property list to a module-level _properties array.
  • Updates initializeComponentData to iterate _properties directly and drops the unused legacy properties parameter.
  • Refactors cloneComponent to build an init data object from _properties and route cloning through addComponent, while preserving post-add cloning of animations / animationsIndex.

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

Comment thread src/framework/components/animation/system.js Outdated
Address review feedback: avoid assigning the shared assets array
reference into the clone data only to immediately overwrite it.

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

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

Comments suppressed due to low confidence (1)

src/framework/components/animation/system.js:54

  • initializeComponentData assigns enabled (as part of the ordered _properties loop) and then calls super.initializeComponentData(component, data) without a properties argument. The base implementation will then set component.enabled = data.enabled again (when provided), firing a second set event and also meaning assets is no longer the last setter invoked despite the ordering requirement.

To keep the ordered setters authoritative, pass an empty properties list to the base initializer ([]) and treat explicit undefined as “not provided” (matching the base initializer semantics) so defaults aren’t clobbered.

    initializeComponentData(component, data) {
        for (const property of _properties) {
            if (data.hasOwnProperty(property)) {
                component[property] = data[property];
            }
        }

        super.initializeComponentData(component, data);
    }

@willeastcott willeastcott merged commit f175165 into main Jun 11, 2026
10 checks passed
@willeastcott willeastcott deleted the refactor/animation-shared-properties branch June 11, 2026 19:41
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