Fix PrefabResource refCount was incorrect
#2864
Conversation
cptbtptpbcptdtptp
added
bug
WalkthroughThis PR changes HierarchyParser._applyEntityData from private to protected, adds a protected override in PrefabParser that marks entities as templates using the parser context resource, adjusts PrefabParser imports, and adds a unit test verifying PrefabResource refCount across instantiation/destruction of a two-entity prefab. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)packages/loader/src/resource-deserialize/resources/parser/HierarchyParser.ts (3)
🔇 Additional comments (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2864 +/- ##
==========================================
- Coverage 79.65% 78.94% -0.71%
==========================================
Files 857 857
Lines 93436 93517 +81
Branches 9372 9378 +6
==========================================
- Hits 74422 73825 -597
- Misses 18863 19543 +680
+ Partials 151 149 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/loader/src/prefab/PrefabParser.ts (1)
2-2: Verify_applyEntityDataoverride doesn’t mis-mark prefab modification targets as templatesOverriding
_applyEntityDatato call_markAsTemplate(this.context.resource)looks like the right hook to ensure prefab template entities are registered againstPrefabResource, which matches the new refCount test.However,
_applyEntityDatais also invoked fromHierarchyParser._parsePrefabModificationwhen applyingpropstotargetEntityinside instantiated prefabs. With this override in place, those modification targets will also be passed through_markAsTemplate, even though they’re instance entities coming from another prefab/GLTF resource.Can you confirm this is intentional? If not, consider either:
- Introducing a dedicated hook used only from
_getEntityByConfigfor initial template construction, or- Guarding the template-marking with a condition (e.g., only when applying the original
entityConfig, not modificationprops).Also applies to: 23-28
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/loader/src/prefab/PrefabParser.ts(2 hunks)packages/loader/src/resource-deserialize/resources/parser/HierarchyParser.ts(2 hunks)tests/src/loader/PrefabResource.test.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
tests/src/loader/PrefabResource.test.ts (2)
packages/core/src/base/EngineObject.ts (1)
engine(21-23)packages/loader/src/prefab/PrefabParser.ts (1)
PrefabParser(6-38)
packages/loader/src/resource-deserialize/resources/parser/HierarchyParser.ts (3)
packages/core/src/Component.ts (1)
entity(71-73)packages/loader/src/resource-deserialize/resources/schema/BasicSchema.ts (1)
IEntity(52-52)packages/core/src/physics/shape/ColliderShape.ts (4)
position(103-105)position(107-111)rotation(90-92)rotation(94-98)
packages/loader/src/prefab/PrefabParser.ts (2)
packages/core/src/Component.ts (1)
entity(71-73)packages/loader/src/resource-deserialize/resources/schema/BasicSchema.ts (1)
IEntity(52-52)
🪛 ESLint
packages/loader/src/resource-deserialize/resources/parser/HierarchyParser.ts
[error] 361-361: Delete ·
(prettier/prettier)
[error] 363-363: Delete ·
(prettier/prettier)
🪛 GitHub Check: lint
packages/loader/src/resource-deserialize/resources/parser/HierarchyParser.ts
[failure] 363-363:
Delete ·
[failure] 361-361:
Delete ·
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: e2e (22.x, 3/4)
- GitHub Check: e2e (22.x, 4/4)
- GitHub Check: e2e (22.x, 1/4)
- GitHub Check: build (22.x, macos-latest)
- GitHub Check: codecov
🔇 Additional comments (1)
tests/src/loader/PrefabResource.test.ts (1)
1-54: Prefab refCount regression test looks solidThe test cleanly exercises the instantiate/destroy lifecycle and asserts the expected refCount transitions; setup/teardown for
WebGLEngineis reasonable for this scope.
| protected _applyEntityData(entity: Entity, entityConfig: IEntity = {}): Entity { | ||
| entity.isActive = entityConfig.isActive ?? entity.isActive; | ||
| entity.name = entityConfig.name ?? entity.name; | ||
| const transform = entity.transform; | ||
| const transformConfig = entityConfig.transform; | ||
| if (transformConfig) { | ||
| this._reflectionParser.parsePropsAndMethods(transform, transformConfig); | ||
| } else { | ||
| const { position, rotation, scale } = entityConfig; | ||
| if (position) transform.position.copyFrom(position); | ||
| if (rotation) transform.rotation.copyFrom(rotation); | ||
| if (scale) transform.scale.copyFrom(scale); | ||
| } | ||
| if (entityConfig.layer) entity.layer = entityConfig.layer; | ||
| return entity; | ||
| } |
There was a problem hiding this comment.
Layer assignment should allow 0 as a valid value
if (entityConfig.layer) entity.layer = entityConfig.layer; will not apply layer = 0, which makes it impossible to reset a previously non-zero layer back to 0 via config or prefab modifications.
Consider checking explicitly for undefined instead:
- if (entityConfig.layer) entity.layer = entityConfig.layer;
+ if (entityConfig.layer !== undefined) {
+ entity.layer = entityConfig.layer;
+ }This preserves the ability to set 0 while still ignoring absent layer config.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| protected _applyEntityData(entity: Entity, entityConfig: IEntity = {}): Entity { | |
| entity.isActive = entityConfig.isActive ?? entity.isActive; | |
| entity.name = entityConfig.name ?? entity.name; | |
| const transform = entity.transform; | |
| const transformConfig = entityConfig.transform; | |
| if (transformConfig) { | |
| this._reflectionParser.parsePropsAndMethods(transform, transformConfig); | |
| } else { | |
| const { position, rotation, scale } = entityConfig; | |
| if (position) transform.position.copyFrom(position); | |
| if (rotation) transform.rotation.copyFrom(rotation); | |
| if (scale) transform.scale.copyFrom(scale); | |
| } | |
| if (entityConfig.layer) entity.layer = entityConfig.layer; | |
| return entity; | |
| } | |
| protected _applyEntityData(entity: Entity, entityConfig: IEntity = {}): Entity { | |
| entity.isActive = entityConfig.isActive ?? entity.isActive; | |
| entity.name = entityConfig.name ?? entity.name; | |
| const transform = entity.transform; | |
| const transformConfig = entityConfig.transform; | |
| if (transformConfig) { | |
| this._reflectionParser.parsePropsAndMethods(transform, transformConfig); | |
| } else { | |
| const { position, rotation, scale } = entityConfig; | |
| if (position) transform.position.copyFrom(position); | |
| if (rotation) transform.rotation.copyFrom(rotation); | |
| if (scale) transform.scale.copyFrom(scale); | |
| } | |
| if (entityConfig.layer !== undefined) { | |
| entity.layer = entityConfig.layer; | |
| } | |
| return entity; | |
| } |
🤖 Prompt for AI Agents
In packages/loader/src/resource-deserialize/resources/parser/HierarchyParser.ts
around lines 65 to 80, the current layer assignment uses a truthy check which
skips applying layer when entityConfig.layer is 0; change the conditional to
explicitly check for undefined (e.g., if (entityConfig.layer !== undefined) ...)
so that zero is accepted while still ignoring absent layer config, then assign
entity.layer = entityConfig.layer when present.
packages/loader/src/resource-deserialize/resources/parser/HierarchyParser.ts
Outdated
Show resolved
Hide resolved
* fix: prefab refcount error
* fix: prefab refcount error
3 participants
Please check if the PR fulfills these requirements
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
What is the current behavior? (You can also link to an open issue here)
What is the new behavior (if this is a feature change)?
Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)
Other information:
Summary by CodeRabbit
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.