Skip to content

refactor(particlesystem): align ParticleSystemComponent with CameraComponent architecture#8873

Merged
willeastcott merged 7 commits into
mainfrom
refactor-particle-system-component
Jun 11, 2026
Merged

refactor(particlesystem): align ParticleSystemComponent with CameraComponent architecture#8873
willeastcott merged 7 commits into
mainfrom
refactor-particle-system-component

Conversation

@willeastcott

Copy link
Copy Markdown
Contributor

Description

Continues the component architecture migration series (#8871 collision, #8870 button, #8777, #8693, #8666) by moving the ParticleSystemComponent off the legacy data-object + set_* event architecture.

component.js

  • All ~60 properties are now private fields on the component with real getters/setters; the side effects previously spread across twelve onSet* event handlers are inlined into the setters, gated on this.emitter (the emitter only exists after onEnable).
  • The old SIMPLE/COMPLEX/GRAPH property category handlers became three private helpers (_setSimpleProperty, _setComplexProperty, _setGraphProperty) called from the setters.
  • Asset setters normalize Asset instances to ids and handle bind/unbind directly.
  • The unused mode and scene data fields are dropped; paused becomes a private runtime field.

data.js

  • Reduced to just enabled = true.

system.js

  • Schema is now ['enabled']; initializeComponentData keeps the legacy meshmeshAsset id remap and vec3/curve/curveset deserialization, then applies properties through the public setters; cloneComponent reads getters and deep-clones Vec3/Curve/CurveSet values; Component._buildAccessors provides the enabled accessor.

Tests

  • Added 8 tests: default values, JSON-style initialization (arrays→Vec3, raw {type, keys}Curve/CurveSet with type preserved), instance pass-through, legacy mesh-id remap, Asset→id normalization, deep cloning, and pause behavior.

Notes for reviewers

  • loop was registered with two legacy handlers (onSetLoop plus the SIMPLE_PROPERTIES handler), so its new setter performs both resetTime() and resetMaterial() to preserve behavior.
  • cloneComponent skips null/undefined values exactly like the old schema-based clone did — otherwise a cloned direct-assigned mesh would be wiped by a trailing meshAsset: null write.
  • Verified beyond unit tests by running the particles examples (spark, mesh) in the browser and live-mutating every setter category (complex rebuild, simple, graph, loop, blendType, depthSoftening depth-layer request/release, layers migration, pause()/play(), enabled toggle) — all propagate to the emitter with no console errors.

Checklist

  • I have read the contributing guidelines
  • My code follows the project's coding standards
  • This PR focuses on a single change

🤖 Generated with Claude Code

…mponent architecture

Move all ~60 properties from ParticleSystemComponentData onto the component
as private fields with real getters/setters, inlining the side effects that
previously lived in set_* event handlers. The data class now holds only
'enabled', the system schema shrinks to ['enabled'], and the system applies
incoming data through the public setters and clones via an explicit
property list.

Behavior is preserved, including the legacy mesh->meshAsset id remap,
vec3/curve/curveset deserialization, the dual resetTime+resetMaterial
side effect of the loop property, and skipping null values when cloning.

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

github-actions Bot commented Jun 11, 2026

Copy link
Copy Markdown

Public API report

This PR changes the public API surface (+10 / −10), per the docs' rules (@ignore / @Private / undocumented are excluded).

Show API diff
-AttributeSchema.type: "string" | "number" | "boolean" | "vec2" | "vec3" | "vec4" | "json" | "rgb" | "asset" | "curve" | "entity" | "rgba"
+AttributeSchema.type: "string" | "number" | "boolean" | "vec2" | "vec3" | "vec4" | "json" | "rgb" | "asset" | "entity" | "rgba" | "curve"
-ParticleSystemComponent.get colorMapAsset(): Asset
+ParticleSystemComponent.get colorMapAsset(): Asset | null
-ParticleSystemComponent.get meshAsset(): Asset
+ParticleSystemComponent.get meshAsset(): Asset | null
-ParticleSystemComponent.get normalMapAsset(): Asset
+ParticleSystemComponent.get normalMapAsset(): Asset | null
-ParticleSystemComponent.get renderAsset(): Asset
+ParticleSystemComponent.get renderAsset(): Asset | null
-ParticleSystemComponent.set colorMapAsset(arg: Asset)
+ParticleSystemComponent.set colorMapAsset(arg: Asset | null)
-ParticleSystemComponent.set meshAsset(arg: Asset)
+ParticleSystemComponent.set meshAsset(arg: Asset | null)
-ParticleSystemComponent.set normalMapAsset(arg: Asset)
+ParticleSystemComponent.set normalMapAsset(arg: Asset | null)
-ParticleSystemComponent.set renderAsset(arg: Asset)
+ParticleSystemComponent.set renderAsset(arg: Asset | null)
-ScriptAttributes.add(name: string, args: { array: boolean; assetType: string; color: string; curves: string[]; default: any; description: string; enum: any[]; max: number; min: number; placeholder: string | string[]; precision: number; schema: any[]; size: number; step: number; title: string; type: "string" | "number" | "boolean" | "vec2" | "vec3" | "vec4" | "json" | "rgb" | "asset" | "curve" | "entity" | "rgba" }): void
+ScriptAttributes.add(name: string, args: { array: boolean; assetType: string; color: string; curves: string[]; default: any; description: string; enum: any[]; max: number; min: number; placeholder: string | string[]; precision: number; schema: any[]; size: number; step: number; title: string; type: "string" | "number" | "boolean" | "vec2" | "vec3" | "vec4" | "json" | "rgb" | "asset" | "entity" | "rgba" | "curve" }): void

Informational only — this never fails the build.

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 ParticleSystemComponent to match the newer component architecture used by Camera/Light/Button/etc by moving most state off the legacy data bag and set_* event handlers, while keeping enabled schema-based.

Changes:

  • Migrates ParticleSystemComponent properties to private fields with real getters/setters; inlines side effects previously implemented via set_* listeners.
  • Reduces component data/schema to enabled only; updates system initialization/cloning to use public setters/getters and perform vec3/curve deserialization.
  • Adds a new unit test suite covering defaults, JSON-style initialization, asset/id normalization, cloning, and pause/play behavior.

Reviewed changes

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

File Description
src/framework/components/particle-system/component.js Moves runtime state to private fields; rewires property side effects and lifecycle behavior to operate on this.emitter and private fields.
src/framework/components/particle-system/system.js Shrinks schema to enabled, updates initialization and cloning to apply properties via setters and deep-clone Vec3/Curve/CurveSet values.
src/framework/components/particle-system/data.js Reduces data object to enabled only.
test/framework/components/particlesystem/component.test.mjs Adds tests for defaults, initialization conversions, legacy remap, asset normalization, cloning, and pause behavior.

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

Comment thread src/framework/components/particle-system/system.js
Comment thread src/framework/components/particle-system/system.js
Comment thread src/framework/components/particle-system/component.js
Comment thread src/framework/components/particle-system/component.js Outdated
willeastcott and others added 2 commits June 11, 2026 16:04
…laying()

Skip present-but-undefined keys in initializeComponentData so class-field
defaults are preserved (matching the base initializer) and curve
deserialization cannot throw on undefined. Make isPlaying() return false
when no emitter exists instead of throwing.

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 4 out of 4 changed files in this pull request and generated 3 comments.

Comment thread src/framework/components/particle-system/system.js
Comment thread src/framework/components/particle-system/component.js
Comment thread src/framework/components/particle-system/component.js
…ty JSDoc

Skip explicit nulls in curve/curveset deserialization so component creation
cannot throw on { alphaGraph: null }. Correct the JSDoc on the four asset
properties to Asset|number|null (they normalize Asset instances to ids),
matching the ModelComponent convention.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Per the convention from the collision migration discussion, the public API
documents encouraged usage (Asset|null) rather than everything tolerated
(ids remain accepted and stored internally).

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 4 out of 4 changed files in this pull request and generated 9 comments.

Comment thread src/framework/components/particle-system/system.js
Comment thread src/framework/components/particle-system/component.js
Comment thread src/framework/components/particle-system/component.js
Comment thread src/framework/components/particle-system/component.js
Comment thread src/framework/components/particle-system/component.js
Comment thread src/framework/components/particle-system/component.js
Comment thread src/framework/components/particle-system/component.js
Comment thread src/framework/components/particle-system/component.js
Comment thread src/framework/components/particle-system/component.js
Asset setters gate deferred loads on the component's enabled state, so the
intended enabled value must be in the data store before they run. Previously
they always saw the default (true) and could trigger loads for components
initialized disabled.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@willeastcott willeastcott merged commit 5f5e65c into main Jun 11, 2026
9 checks passed
@willeastcott willeastcott deleted the refactor-particle-system-component branch June 11, 2026 16: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