refactor: Clean up vestigial Model plumbing on SpriteComponent#8674
Merged
Conversation
SpriteComponent declared a _model = new Model() field and performed bookkeeping on it, but nothing ever consumed it. The sprite's single mesh instance is created against _node directly, _showModel/_hideModel use [_meshInstance] directly (not _model.meshInstances), and BatchManager's sprite path reads _meshInstance too. No references to sprite._model or sprite.model exist anywhere. Co-authored-by: Cursor <cursoragent@cursor.com>
…mponent Rename private legacy-named state and methods in SpriteComponent to reflect what they actually do (managing mesh instance membership in scene layers): _addedModel -> _inLayers, _showModel -> _addToLayers, _hideModel -> _removeFromLayers. The names were holdovers from when SpriteComponent owned a Model container (removed in #8674). Co-authored-by: Cursor <cursoragent@cursor.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors SpriteComponent to remove legacy Model usage that was no longer functionally used, and renames internal layer-membership helpers to reflect their actual responsibility (adding/removing the sprite mesh instance to/from scene layers).
Changes:
- Remove the unused
Modelinstance and associated bookkeeping fromSpriteComponent. - Rename internal layer membership state / helpers (
_addedModel→_inLayers,_showModel/_hideModel→_addToLayers/_removeFromLayers). - Update
SpriteAnimationClipcall sites to use the renamed helpers.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/framework/components/sprite/sprite-animation-clip.js | Updates internal calls to the renamed SpriteComponent layer-membership helpers. |
| src/framework/components/sprite/component.js | Removes vestigial Model plumbing, renames private state/methods around layer membership, and updates all internal call sites accordingly. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
removeModelFromLayers (called by BatchManager when a sprite is absorbed into a batch) was removing the mesh instance from layers without clearing the new _inLayers flag. That left two stale-state hazards: - When batchGroupId was set back to -1, _addToLayers early-outed because _inLayers was still true, so the sprite never re-appeared. - _onLayerAdded would re-add the unbatched mesh instance to any newly added matching layer behind the batch's back. Delegate removeModelFromLayers to _removeFromLayers so the flag (and the _meshInstance null guard) stay consistent. Also reword a stale "hide the model" comment. Co-authored-by: Cursor <cursoragent@cursor.com>
…nent Renames the internal `_addToLayers` / `_removeFromLayers` to public-but- private `addToLayers` / `removeFromLayers` (with `@private` JSDoc), and removes the `removeModelFromLayers` wrapper. BatchManager now calls `node.sprite.removeFromLayers()` directly, matching the shape it already uses for `node.render.removeFromLayers()`. This drops the last vestigial `Model` reference from SpriteComponent and unifies the convention with RenderComponent, leaving ModelComponent and ElementComponent (which still use `addModelToLayers` / `removeModelFromLayers`) as the only outliers - those can be aligned in a follow-up. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Fix _onLayersChanged binding the wrong handler names. The off/on calls referenced `this.onLayerAdded` / `this.onLayerRemoved` but the methods are `_onLayerAdded` / `_onLayerRemoved` (matching how onEnable wires them up). Replacing the scene's layers component would silently fail to rebind the listeners (and assert in debug builds). - Re-add `removeModelFromLayers()` as a deprecated alias that delegates to `removeFromLayers()`, per the engine's API stability rules. - Type `_autoPlayClip` as `string|null`. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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
Cleans up
SpriteComponent's vestigialModelplumbing, fixes a layer-membership desync againstBatchManager, fixes a pre-existing handler-name typo, and aligns the layer-membership API withRenderComponent.Details
1. Remove unused
_modelfieldSpriteComponentdeclared a_model = new Model()field and performed bookkeeping on it (settinggraph, pushing the mesh instance, nulling it out on destroy), but nothing ever consumed it:this._nodedirectly, not_model.graph._showModel/_hideModelhelpers add/remove[this._meshInstance]to/from layers directly, bypassing_model.meshInstances.BatchManager's sprite path readsnode.sprite._meshInstance, not_model.sprite._modelorsprite.modelexist anywhere (engine, tests, examples, editor, or any downstream repo).The
Modelclass is legacy; sprite was the one remaining component using it for no functional purpose.2. Fix
_inLayersdesync againstBatchManagerThe legacy
removeModelFromLayers()(called byBatchManagerwhen a sprite is absorbed into a batch) removed the mesh instance from layers without clearing the membership flag. This caused two stale-state hazards:batchGroupIdreturned to-1,_addToLayers()early-outed because the flag was stilltrue, leaving the sprite invisible._onLayerAddedwould re-add the unbatched mesh to any newly added matching layer, racing the batch.3. Fix
_onLayersChangedhandler-name typoPre-existing bug:
_onLayersChangedwas rebindingthis.onLayerAdded/this.onLayerRemoved(no underscore) but the actual methods are_onLayerAdded/_onLayerRemoved. Replacing the scene's layers component silently failed to rebind the listeners (and would assert in debug builds).4. Align layer-membership API with
RenderComponentWith the
Modelplumbing gone, the symbols are renamed and the BatchManager hook is unified with the existingRenderComponentpattern:_addedModel(private field)_inLayers(private field)_showModel()(private method)addToLayers()(@privateJSDoc)_hideModel()(private method)removeFromLayers()(@privateJSDoc)removeModelFromLayers()(undocumented, BatchManager-only)BatchManager._extractSpritesnow callsnode.sprite.removeFromLayers()directly — the same shape it already uses fornode.render.removeFromLayers(). Because that path goes throughremoveFromLayers(),_inLayersstays in sync, the_meshInstancenull guard is inherited, and the desync described in (2) is eliminated.removeModelFromLayers()was undocumented and only ever called byBatchManager(the engine's own internal infrastructure), so it's removed outright rather than carried as a deprecated alias.ModelComponentandElementComponentstill expose the legacyaddModelToLayers/removeModelFromLayerspair; aligning those is left for a follow-up.5. Minor type annotations
_meshInstanceand_autoPlayClipgot proper@typeannotations (MeshInstance|nullandstring|null).Test plan
npm run lint— clean.