feat(loader): v2 scene/prefab format parsing#2959
feat(loader): v2 scene/prefab format parsing#2959luzhuang wants to merge 43 commits intogalacean:dev/2.0from
Conversation
Rewrite the loader parsers to directly consume v2 scene/prefab format with $ prefix value types, eliminating the v1 conversion layer. - ReflectionParser: handle $ref, $type, $entity, $component, $signal - HierarchyParser: flat entity array with integer index references - ParserContext: entityMap as Map<number, Entity> for flat index lookup - SceneParser/PrefabParser: v2 $ref asset references - BasicSchema: strip v1-only types, keep IAssetRef for material loaders - SceneSchema: IScene extends IHierarchyFile with Vec4Tuple support
- Fix getResourceByRef param: refId → url across all call sites - Fix $signal resolution: call Signal.on() instead of replacing instance - Restore missing Font import in SceneLoader - Change customParseComponentHandles from Map to Record (bracket access) - Remove dead _prefabPromiseMap code in HierarchyParser - Extract assetRefToEngine() to types.ts, reuse everywhere - Remove unused entityIndex param from _loadPrefabInstance
- Revert IAssetRef type from { refId } back to { url } to match v1 runtime data
- Remove unused generic parameter I from ParserContext
- Fix constructor.name to Loader.getClassName for minification safety
- Add component pipeline tests (Stage 3-4) and $type resolution tests
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughReworks loader/parser stack from v1 to v2 scene/prefab format: introduces flat numeric entity indices, AssetRef Changes
Sequence Diagram(s)sequenceDiagram
participant Client as SceneLoader
participant Parser as HierarchyParser
participant Reflect as ReflectionParser
participant Prefab as PrefabParser
participant Resource as ResourceManager
participant Engine as Engine
Client->>Resource: _request(url) -> SceneFile (with $ref, entities[])
Client->>Parser: new ParserContext(data)
Parser->>Parser: _parseEntities() (iterate numeric indices)
Parser->>Prefab: detect instance entity -> load prefab via Resource
Prefab->>Resource: _request(prefabUrl) -> PrefabFile
Prefab-->>Parser: instantiated entities mapped by index
Parser->>Parser: _organizeEntities() (link children by index)
Parser->>Parser: _parseComponents() (create components)
Parser->>Parser: _parseComponentsProps() (queue props)
loop apply props / overrides
Parser->>Reflect: parseProps(target, props)
Reflect->>Resource: resolve `{ $ref }` -> asset
Reflect->>Parser: resolve `{ $entity }` / `{ $component }` by index
Reflect->>Engine: set resolved values on instances
end
Parser->>Parser: _parsePrefabOverrides() (apply instance overrides)
Parser->>Client: _clearAndResolve() -> built Scene / PrefabResource
Estimated Code Review Effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
tests/src/loader/SceneFormatV2.test.ts (1)
39-131: Good coverage forReflectionParserprop resolution.Tests cover the key v2 resolution paths: primitives, nested objects, in-place mutation, arrays,
$entity,$component, and$refdetection.Consider adding tests for:
$signalbinding resolution- Error cases (e.g., invalid
$entityindex returnsundefined, unregistered$typethrows)Would you like me to draft additional test cases for
$signalresolution and error handling?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/src/loader/SceneFormatV2.test.ts` around lines 39 - 131, Add tests exercising ReflectionParser.parseProps for $signal resolution and error cases: create a scene/context and entities, register a signal (via the same context structure used for entity/component resolution) then call parseProps with a property like { sig: { $signal: { entity: 0, name: "signalName" } } } and assert the resolved value is the expected bound signal; add a test where an invalid $entity index (no entry in context.entityMap) is used and assert the result is undefined; add a test where a $component refers to a non-registered type (e.g., { $component: { entity: 0, type: "NonExistent", index: 0 } }) and assert parseProps throws/rejects. Reference ReflectionParser, parseProps, and context.entityMap/context handling for locating where to add these cases.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/loader/src/resource-deserialize/resources/scene/SceneParser.ts`:
- Around line 41-56: The code assumes scene.background is always present and
reads background.mode unguarded; update SceneParser.ts to first check that
scene.background is defined (e.g., if (!background) return/skip dependency
collection for background) before accessing background.mode, then retain the
existing branches for BackgroundMode.Texture and BackgroundMode.Sky that call
context._addDependentAsset with
resourceManager.getResourceByRef(assetRefToEngine(...)); ensure both the Texture
and Sky branches are only executed when background is non-null and their
respective fields (texture, skyMesh, skyMaterial) are present to avoid runtime
errors.
- Around line 78-95: The loop in SceneParser that handles entity.instance only
enqueues instance.asset and skips nested refs in instance.overrides; update the
logic inside the entities loop (the block that checks entity.instance) to also
traverse instance.overrides and any nested data for asset refs the same way
components do: call this._searchDependentAssets on instance.overrides (or
otherwise walk its structure) and continue using context._addDependentAsset with
assetRefToEngine for any discovered $ref values so override-added
scripts/components are preloaded before Loader.getClass is called; keep existing
usage of context._addDependentAsset, assetRefToEngine, and
this._searchDependentAssets to locate and register those refs.
In `@packages/loader/src/SceneLoader.ts`:
- Line 52: Remove the invalid property assignment to
scene.ambientLight.specularTextureDecodeRGBM — the AmbientLight class does not
have that property; instead remove the entire line and, if RGBM decode is
required, configure it on the TextureCube referenced by
AmbientLight.specularTexture (e.g., adjust decode settings on the texture
object, not on AmbientLight). Ensure no other references to
specularTextureDecodeRGBM remain and keep AmbientLight.specularTexture usage
unchanged.
---
Nitpick comments:
In `@tests/src/loader/SceneFormatV2.test.ts`:
- Around line 39-131: Add tests exercising ReflectionParser.parseProps for
$signal resolution and error cases: create a scene/context and entities,
register a signal (via the same context structure used for entity/component
resolution) then call parseProps with a property like { sig: { $signal: {
entity: 0, name: "signalName" } } } and assert the resolved value is the
expected bound signal; add a test where an invalid $entity index (no entry in
context.entityMap) is used and assert the result is undefined; add a test where
a $component refers to a non-registered type (e.g., { $component: { entity: 0,
type: "NonExistent", index: 0 } }) and assert parseProps throws/rejects.
Reference ReflectionParser, parseProps, and context.entityMap/context handling
for locating where to add these cases.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 03a0c311-9bab-416b-8cfe-2f7a56c9cc32
📒 Files selected for processing (12)
packages/loader/src/PrefabLoader.tspackages/loader/src/SceneLoader.tspackages/loader/src/prefab/PrefabParser.tspackages/loader/src/resource-deserialize/resources/parser/HierarchyParser.tspackages/loader/src/resource-deserialize/resources/parser/ParserContext.tspackages/loader/src/resource-deserialize/resources/parser/ReflectionParser.tspackages/loader/src/resource-deserialize/resources/scene/SceneParser.tspackages/loader/src/resource-deserialize/resources/schema/BasicSchema.tspackages/loader/src/resource-deserialize/resources/schema/SceneSchema.tspackages/loader/src/resource-deserialize/resources/schema/index.tspackages/loader/src/scene-format/types.tstests/src/loader/SceneFormatV2.test.ts
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
HierarchyParser: - 合并 _applyEntityData/_applyEntityOverrides/_createInlineEntity 中重复的 entity props 赋值为统一的 _applyEntityProps 静态方法 - _overrideKeyToPath 用 slice+replace 替代 JSON.parse ReflectionParser: - customParseComponentHandles 类型从 Function 收窄为具体签名 - $ref 解析添加 resource null 守卫，防止 null 加入 dependence assets SceneParser: - _parseDependentAssets 补充扫描 prefab instance overrides 中的 $ref - 移除 _isAssetRef 调用前多余的 @ts-ignore PrefabParser: - 移除 parse() 中多余的 .then(() => prefabResource) types.ts: - _entityIds/_componentIds 添加 JSDoc 说明编辑器专用字段 测试: - PrefabResource 测试迁移到 v2 格式，新增 $ref null 守卫和 instance overrides 测试 - SceneFormatV2 新增 $signal 绑定和 $type 多态解析测试
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/loader/src/resource-deserialize/resources/scene/SceneParser.ts (1)
41-57:⚠️ Potential issue | 🟠 MajorGuard the optional
backgroundblock.The code still accesses
background.modewithout checking ifbackgroundexists. Ifscene.backgroundis undefined, this will throw.🛡️ Proposed fix
const background = scene.background; - const backgroundMode = background.mode; - if (backgroundMode === BackgroundMode.Texture) { + if (background) { + const backgroundMode = background.mode; + if (backgroundMode === BackgroundMode.Texture) { const texture = background.texture; if (texture) { // `@ts-ignore` context._addDependentAsset(texture.$ref, resourceManager.getResourceByRef(assetRefToEngine(texture))); } - } else if (backgroundMode === BackgroundMode.Sky) { + } else if (backgroundMode === BackgroundMode.Sky) { const { skyMesh, skyMaterial } = background; if (skyMesh && skyMaterial) { // `@ts-ignore` context._addDependentAsset(skyMesh.$ref, resourceManager.getResourceByRef(assetRefToEngine(skyMesh))); // `@ts-ignore` context._addDependentAsset(skyMaterial.$ref, resourceManager.getResourceByRef(assetRefToEngine(skyMaterial))); } + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/loader/src/resource-deserialize/resources/scene/SceneParser.ts` around lines 41 - 57, The code reads background.mode without ensuring scene.background exists, so add a guard for the optional background before accessing its properties: check that scene.background (the local const background) is defined (e.g., if (!background) { /* skip */ } or wrap the existing logic in if (background) { ... }) before using backgroundMode, background.texture, skyMesh or skyMaterial; keep using the same calls to context._addDependentAsset, resourceManager.getResourceByRef and assetRefToEngine for the dependent-asset handling.
🧹 Nitpick comments (4)
packages/loader/src/resource-deserialize/resources/parser/ReflectionParser.ts (2)
85-94: Guard against unregistered component type in$componentresolution.Similar to
$type, ifLoader.getClass(comp.type)returnsundefined, thegetComponentscall will fail. Add a guard for robustness.🛡️ Proposed defensive check
// $component — component reference: { entity, type, index } if ("$component" in obj) { const comp = obj.$component as { entity: number; type: string; index: number }; const entity = this._context.entityMap.get(comp.entity); if (entity) { - const components = entity.getComponents(Loader.getClass(comp.type), []); + const ComponentClass = Loader.getClass(comp.type); + if (!ComponentClass) { + console.warn(`ReflectionParser: Unknown component type "${comp.type}"`); + return Promise.resolve(null); + } + const components = entity.getComponents(ComponentClass, []); return Promise.resolve(components[comp.index] ?? null); } return Promise.resolve(null); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/loader/src/resource-deserialize/resources/parser/ReflectionParser.ts` around lines 85 - 94, The $component branch in ReflectionParser.ts uses Loader.getClass(comp.type) without checking for undefined, which can cause entity.getComponents to throw; update the block that reads comp (the "$component" in obj branch) to first store the result of Loader.getClass(comp.type) (or call a variable like compClass) and if it's undefined return Promise.resolve(null) (or otherwise short-circuit) before calling entity.getComponents(compClass, []); ensure you reference comp, Loader.getClass, and entity.getComponents when applying the guard.
69-77: Guard against unregistered class in$typeresolution.If
Loader.getClass($type)returnsundefinedfor an unregistered type name,new Class()will throw a runtime error. Consider adding a guard with a meaningful error message.🛡️ Proposed defensive check
// $type — polymorphic type: construct instance and apply remaining props if ("$type" in obj) { const { $type, ...rest } = obj; const Class = Loader.getClass($type as string); + if (!Class) { + console.warn(`ReflectionParser: Unknown type "${$type}"`); + return Promise.resolve(null); + } const instance = new Class(); if (Object.keys(rest).length > 0) { return this.parseProps(instance, rest); } return Promise.resolve(instance); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/loader/src/resource-deserialize/resources/parser/ReflectionParser.ts` around lines 69 - 77, The code in ReflectionParser that handles objects with a "$type" needs to guard against Loader.getClass($type) returning undefined; update the block where you call Loader.getClass($type) and construct instance (in ReflectionParser) to check whether Class is truthy, and if not throw or return a rejected Promise with a clear error mentioning the unresolved type name (e.g., include $type and context like "unregistered class type in ReflectionParser"); ensure parsing flow (parseProps and Promise.resolve(instance)) remains unchanged when Class is valid.packages/loader/src/resource-deserialize/resources/parser/HierarchyParser.ts (2)
238-245: Consider order of operations for removedEntities.Destroying entities during iteration over
removedEntitiesshould be safe since each path is independent. However, if a parent entity is destroyed before its children are processed, attempting to destroy the child may fail silently or cause unexpected behavior. Consider processing in reverse depth order (children first) or validating that the entity still exists.💡 Optional: Add existence check before destroy
// removedEntities — destroy entities from prefab tree if (overrides.removedEntities) { for (let j = 0, m = overrides.removedEntities.length; j < m; j++) { const path = overrides.removedEntities[j].join("/"); const target = ctx.entityMap.get(path); - if (target) target.destroy(); + if (target && !target.destroyed) target.destroy(); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/loader/src/resource-deserialize/resources/parser/HierarchyParser.ts` around lines 238 - 245, When destroying entities from overrides.removedEntities in HierarchyParser (the loop that looks up paths via ctx.entityMap and calls target.destroy()), ensure children are destroyed before parents by sorting or iterating the paths in reverse-depth order (e.g., longest path first) so parent removals don't invalidate child destruction; additionally guard each destroy call with an existence check on ctx.entityMap.get(path) (or verify target.isDestroyed is false) before calling target.destroy() to avoid silent failures.
137-141: Guard against unregistered class when adding components.If
Loader.getClass(key)returnsundefined(e.g., script not loaded or type not registered),entity.addComponent(undefined)will fail at runtime. Consider adding validation.🛡️ Proposed defensive check
for (let j = 0, m = componentIndices.length; j < m; j++) { const config = allComponents[componentIndices[j]]; const key = config.script ? config.script.$ref : config.type; - const component = entity.addComponent(Loader.getClass(key)); + const ComponentClass = Loader.getClass(key); + if (!ComponentClass) { + console.warn(`HierarchyParser: Unknown component type "${key}"`); + continue; + } + const component = entity.addComponent(ComponentClass); componentPairs.push({ component, config }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/loader/src/resource-deserialize/resources/parser/HierarchyParser.ts` around lines 137 - 141, The code calls Loader.getClass(key) and immediately passes its result to entity.addComponent, which can throw if the class is undefined; update the loop in HierarchyParser to validate the returned class before adding: call Loader.getClass(key), check for falsy/undefined, log or throw a clear error (including key and config info) and skip pushing into componentPairs when undefined, otherwise proceed to create the component with entity.addComponent; reference Loader.getClass, entity.addComponent, componentPairs, config, and key when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/loader/src/resource-deserialize/resources/scene/SceneParser.ts`:
- Around line 41-57: The code reads background.mode without ensuring
scene.background exists, so add a guard for the optional background before
accessing its properties: check that scene.background (the local const
background) is defined (e.g., if (!background) { /* skip */ } or wrap the
existing logic in if (background) { ... }) before using backgroundMode,
background.texture, skyMesh or skyMaterial; keep using the same calls to
context._addDependentAsset, resourceManager.getResourceByRef and
assetRefToEngine for the dependent-asset handling.
---
Nitpick comments:
In
`@packages/loader/src/resource-deserialize/resources/parser/HierarchyParser.ts`:
- Around line 238-245: When destroying entities from overrides.removedEntities
in HierarchyParser (the loop that looks up paths via ctx.entityMap and calls
target.destroy()), ensure children are destroyed before parents by sorting or
iterating the paths in reverse-depth order (e.g., longest path first) so parent
removals don't invalidate child destruction; additionally guard each destroy
call with an existence check on ctx.entityMap.get(path) (or verify
target.isDestroyed is false) before calling target.destroy() to avoid silent
failures.
- Around line 137-141: The code calls Loader.getClass(key) and immediately
passes its result to entity.addComponent, which can throw if the class is
undefined; update the loop in HierarchyParser to validate the returned class
before adding: call Loader.getClass(key), check for falsy/undefined, log or
throw a clear error (including key and config info) and skip pushing into
componentPairs when undefined, otherwise proceed to create the component with
entity.addComponent; reference Loader.getClass, entity.addComponent,
componentPairs, config, and key when making the change.
In
`@packages/loader/src/resource-deserialize/resources/parser/ReflectionParser.ts`:
- Around line 85-94: The $component branch in ReflectionParser.ts uses
Loader.getClass(comp.type) without checking for undefined, which can cause
entity.getComponents to throw; update the block that reads comp (the
"$component" in obj branch) to first store the result of
Loader.getClass(comp.type) (or call a variable like compClass) and if it's
undefined return Promise.resolve(null) (or otherwise short-circuit) before
calling entity.getComponents(compClass, []); ensure you reference comp,
Loader.getClass, and entity.getComponents when applying the guard.
- Around line 69-77: The code in ReflectionParser that handles objects with a
"$type" needs to guard against Loader.getClass($type) returning undefined;
update the block where you call Loader.getClass($type) and construct instance
(in ReflectionParser) to check whether Class is truthy, and if not throw or
return a rejected Promise with a clear error mentioning the unresolved type name
(e.g., include $type and context like "unregistered class type in
ReflectionParser"); ensure parsing flow (parseProps and
Promise.resolve(instance)) remains unchanged when Class is valid.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 029afca3-d2dd-42c7-b05c-a04d13c1e20a
📒 Files selected for processing (9)
.gitignorepackages/loader/src/SceneLoader.tspackages/loader/src/prefab/PrefabParser.tspackages/loader/src/resource-deserialize/resources/parser/HierarchyParser.tspackages/loader/src/resource-deserialize/resources/parser/ReflectionParser.tspackages/loader/src/resource-deserialize/resources/scene/SceneParser.tspackages/loader/src/scene-format/types.tstests/src/loader/PrefabResource.test.tstests/src/loader/SceneFormatV2.test.ts
✅ Files skipped from review due to trivial changes (2)
- .gitignore
- packages/loader/src/scene-format/types.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/loader/src/prefab/PrefabParser.ts
prettier 拆行后 @ts-ignore 只覆盖了 resourceManager 而非下一行的 .getResourceByRef，将 @ts-ignore 移到 getResourceByRef 正上方。 同时格式化所有 changed 文件通过 prettier 检查。
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## dev/2.0 #2959 +/- ##
===========================================
+ Coverage 77.41% 77.64% +0.23%
===========================================
Files 898 905 +7
Lines 98314 98714 +400
Branches 9775 9959 +184
===========================================
+ Hits 76105 76644 +539
+ Misses 22043 21901 -142
- Partials 166 169 +3
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:
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/loader/src/resource-deserialize/resources/scene/SceneParser.ts (1)
44-60:⚠️ Potential issue | 🟠 MajorGuard the optional
backgroundblock.The code reads
background.modewithout checking ifbackgroundis defined. If the scene file lacks abackgroundsection, this will throw a runtime error during dependency collection.🛡️ Suggested fix
const background = scene.background; - const backgroundMode = background.mode; - if (backgroundMode === BackgroundMode.Texture) { + if (background) { + const backgroundMode = background.mode; + if (backgroundMode === BackgroundMode.Texture) { + const texture = background.texture; + if (texture) { + // `@ts-ignore` + context._addDependentAsset(texture.$ref, resourceManager.getResourceByRef(assetRefToEngine(texture))); + } + } else if (backgroundMode === BackgroundMode.Sky) { + // ...existing sky handling... + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/loader/src/resource-deserialize/resources/scene/SceneParser.ts` around lines 44 - 60, SceneParser accesses background.mode without checking background; wrap the existing background handling in a guard that checks background is defined (e.g., if (background) { ... }) before reading background.mode and its fields so dependency collection (context._addDependentAsset, resourceManager.getResourceByRef, assetRefToEngine) never runs on undefined; ensure both Texture and Sky branches stay inside that guard and keep existing null checks for texture, skyMesh, and skyMaterial.
🧹 Nitpick comments (4)
packages/loader/src/SceneLoader.ts (2)
190-200: Consider simplifying the TextRenderer handler to a synchronous function.The handler is declared
asyncbut doesn't perform any asynchronous operations. SinceFont.createFromOSappears to be synchronous and noawaitis used, the handler could be a regular function returning the instance directly, improving clarity.♻️ Suggested simplification
ReflectionParser.registerCustomParseComponent( "TextRenderer", - async (instance: any, item: { props?: Record<string, unknown> }) => { + (instance: any, item: { props?: Record<string, unknown> }) => { const { props } = item; if (!props?.font) { // `@ts-ignore` instance.font = Font.createFromOS(instance.engine, (props?.fontFamily as string) || "Arial"); } return instance; } );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/loader/src/SceneLoader.ts` around lines 190 - 200, The TextRenderer custom parse handler is declared async but performs no asynchronous work; change the handler passed to ReflectionParser.registerCustomParseComponent for "TextRenderer" to be a synchronous function (remove the async keyword) that sets instance.font via Font.createFromOS when props.font is absent and then returns the instance directly; keep the same parameter names (instance, item) and logic around props?.font and props?.fontFamily so only the function signature changes.
44-51: Consider extracting the array/object color handling pattern to a helper.The same pattern for handling colors as either arrays or objects is repeated three times (diffuseSolidColor, background.solidColor, fogColor). A small helper function would reduce duplication and improve maintainability.
♻️ Proposed helper function
function applyColor(target: { set: (r: number, g: number, b: number, a: number) => void; copyFrom: (c: any) => void }, color: any): void { if (Array.isArray(color)) { target.set(color[0], color[1], color[2], color[3]); } else { target.copyFrom(color); } } // Usage: if (solidColor) applyColor(scene.ambientLight.diffuseSolidColor, solidColor);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/loader/src/SceneLoader.ts` around lines 44 - 51, Extract the repeated array/object color handling into a helper (e.g., applyColor) and use it from SceneLoader wherever colors are handled: replace the inline checks around scene.ambientLight.diffuseSolidColor, background.solidColor, and fogColor with calls to applyColor(target, color). The helper should accept a target with set(r,g,b,a) and copyFrom(color) methods and perform Array.isArray(color) ? target.set(...) : target.copyFrom(color) so the three duplicated blocks are removed and the logic centralized.packages/loader/src/resource-deserialize/resources/parser/HierarchyParser.ts (2)
272-279: Misleading type annotation ongetResourceByRef.The generic
<Entity>suggests the resource is anEntity, but the actual return type isPrefabResource | GLTFResource. Consider using a more accurate type to improve code clarity.♻️ Suggested type fix
return ( engine.resourceManager // `@ts-ignore` - .getResourceByRef<Entity>(assetRefToEngine(instance.asset)) + .getResourceByRef<PrefabResource | GLTFResource>(assetRefToEngine(instance.asset)) .then((prefabResource: PrefabResource | GLTFResource) => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/loader/src/resource-deserialize/resources/parser/HierarchyParser.ts` around lines 272 - 279, The type parameter on engine.resourceManager.getResourceByRef is misleading (currently <Entity>) — update the call in HierarchyParser to use the correct resource union type (PrefabResource | GLTFResource) or remove the generic so TypeScript infers the return as PrefabResource | GLTFResource; replace the incorrect generic and remove the accompanying // `@ts-ignore` so the subsequent logic that calls prefabResource.instantiate() or prefabResource.instantiateSceneRoot() is typed accurately.
226-245: Consider override application order for edge cases.
addedEntitiesis processed beforeremovedEntities. If an override adds a child entity to a parent that is subsequently removed, the newly added entity becomes orphaned (destroyed along with its parent). This may be intentional behavior, but consider documenting the override application order or reordering to process removals first if orphaned entities are unexpected.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/loader/src/resource-deserialize/resources/parser/HierarchyParser.ts` around lines 226 - 245, Summary: addedEntities are applied before removedEntities which can orphan newly added children; either process removals first or document the order. In HierarchyParser, locate the block handling overrides.addedEntities and overrides.removedEntities and either swap the two loops so removedEntities are processed before addedEntities (use overrides.removedEntities loop first, call ctx.entityMap.get(path) and target.destroy() as currently done) or add a clear comment/docstring near HierarchyParser explaining that additions happening before removals may be intentionally orphaned; ensure references to _createInlineEntity, ctx.entityMap, overrides.addedEntities, overrides.removedEntities, and target.destroy are updated accordingly so future readers understand the chosen behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/loader/src/resource-deserialize/resources/parser/ReflectionParser.ts`:
- Around line 71-80: When handling polymorphic objects in ReflectionParser (the
"$type" branch), guard the Loader.getClass($type) result before calling new
Class(): check if Class is falsy/undefined and throw or return a rejected
Promise with a clear error that includes the unresolved $type string and context
(e.g., "Unregistered type: { $type } in ReflectionParser"), otherwise proceed to
instantiate and call this.parseProps(instance, rest) as before; update uses of
Class, instance, parseProps to follow this new guard so the error is descriptive
instead of a cryptic runtime crash.
- Around line 99-109: In ReflectionParser, when handling the "$signal" branch
avoid calling _resolveSignal if the existing signal instance (originValue) is
undefined/null; add a guard before invoking _resolveSignal that checks
originValue (the resolved target instance) and either return early (e.g., skip
registration/return originValue) or throw a clear error/log message so
_resolveSignal is never called on null; update the "$signal" handling code to
validate originValue and reference the same parameter names (originValue and
_resolveSignal) to locate the fix.
---
Duplicate comments:
In `@packages/loader/src/resource-deserialize/resources/scene/SceneParser.ts`:
- Around line 44-60: SceneParser accesses background.mode without checking
background; wrap the existing background handling in a guard that checks
background is defined (e.g., if (background) { ... }) before reading
background.mode and its fields so dependency collection
(context._addDependentAsset, resourceManager.getResourceByRef, assetRefToEngine)
never runs on undefined; ensure both Texture and Sky branches stay inside that
guard and keep existing null checks for texture, skyMesh, and skyMaterial.
---
Nitpick comments:
In
`@packages/loader/src/resource-deserialize/resources/parser/HierarchyParser.ts`:
- Around line 272-279: The type parameter on
engine.resourceManager.getResourceByRef is misleading (currently <Entity>) —
update the call in HierarchyParser to use the correct resource union type
(PrefabResource | GLTFResource) or remove the generic so TypeScript infers the
return as PrefabResource | GLTFResource; replace the incorrect generic and
remove the accompanying // `@ts-ignore` so the subsequent logic that calls
prefabResource.instantiate() or prefabResource.instantiateSceneRoot() is typed
accurately.
- Around line 226-245: Summary: addedEntities are applied before removedEntities
which can orphan newly added children; either process removals first or document
the order. In HierarchyParser, locate the block handling overrides.addedEntities
and overrides.removedEntities and either swap the two loops so removedEntities
are processed before addedEntities (use overrides.removedEntities loop first,
call ctx.entityMap.get(path) and target.destroy() as currently done) or add a
clear comment/docstring near HierarchyParser explaining that additions happening
before removals may be intentionally orphaned; ensure references to
_createInlineEntity, ctx.entityMap, overrides.addedEntities,
overrides.removedEntities, and target.destroy are updated accordingly so future
readers understand the chosen behavior.
In `@packages/loader/src/SceneLoader.ts`:
- Around line 190-200: The TextRenderer custom parse handler is declared async
but performs no asynchronous work; change the handler passed to
ReflectionParser.registerCustomParseComponent for "TextRenderer" to be a
synchronous function (remove the async keyword) that sets instance.font via
Font.createFromOS when props.font is absent and then returns the instance
directly; keep the same parameter names (instance, item) and logic around
props?.font and props?.fontFamily so only the function signature changes.
- Around line 44-51: Extract the repeated array/object color handling into a
helper (e.g., applyColor) and use it from SceneLoader wherever colors are
handled: replace the inline checks around scene.ambientLight.diffuseSolidColor,
background.solidColor, and fogColor with calls to applyColor(target, color). The
helper should accept a target with set(r,g,b,a) and copyFrom(color) methods and
perform Array.isArray(color) ? target.set(...) : target.copyFrom(color) so the
three duplicated blocks are removed and the logic centralized.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: c4d49ffd-87a7-4d84-9ef2-7fd19720d90a
📒 Files selected for processing (5)
packages/loader/src/SceneLoader.tspackages/loader/src/resource-deserialize/resources/parser/HierarchyParser.tspackages/loader/src/resource-deserialize/resources/parser/ParserContext.tspackages/loader/src/resource-deserialize/resources/parser/ReflectionParser.tspackages/loader/src/resource-deserialize/resources/scene/SceneParser.ts
- SceneParser: 提取 _searchComponentDependentAssets/_searchInlineEntityDependentAssets， 补充 addedEntities 递归扫描 - ReflectionParser: parseProps 接受可选 props 参数，$type 分支加 getClass guard - HierarchyParser: 3 处 Loader.getClass 加未注册类型 guard，消除调用侧冗余 props 判空 - 测试: 补充 prefab override 全部 6 种类型测试，加 getClass 错误路径测试
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/loader/src/resource-deserialize/resources/parser/ReflectionParser.ts (1)
103-113:⚠️ Potential issue | 🟡 MinorGuard against missing signal instance.
If the target property doesn't have an existing
Signalinstance (originValueisundefined),_resolveSignalwill attempt to call methods onnull/undefined, causing a runtime error. Consider adding a null check before invoking_resolveSignal.🛡️ Proposed guard
// $signal — signal binding: register listeners on the existing Signal instance if ("$signal" in obj) { + if (!originValue) { + console.warn("ReflectionParser: $signal target does not have an existing Signal instance"); + return Promise.resolve(originValue); + } return this._resolveSignal( originValue, obj.$signal as Array<{🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/loader/src/resource-deserialize/resources/parser/ReflectionParser.ts` around lines 103 - 113, The $signal handling block in ReflectionParser.ts should guard against a missing Signal instance: before calling this._resolveSignal with originValue, check that originValue is not null/undefined (the resolved target Signal instance); if it is missing, either skip registration or log/throw a clear error. Update the code path that handles "$signal" to validate originValue and only call _resolveSignal(originValue, obj.$signal) when originValue is present, referencing the "$signal" branch and the _resolveSignal method to locate the change.
🧹 Nitpick comments (3)
packages/loader/src/resource-deserialize/resources/parser/ReflectionParser.ts (2)
92-101: Consider guarding against unregistered component types in$componentresolution.If
Loader.getClass(comp.type)returnsundefinedfor an unregistered type,entity.getComponents(undefined, [])may produce unexpected behavior. For consistency with the$typeguard at line 78, consider adding a similar check here.🛡️ Proposed guard
// $component — component reference: { entity, type, index } if ("$component" in obj) { const comp = obj.$component as { entity: number; type: string; index: number }; const entity = this._context.entityMap.get(comp.entity); if (entity) { - const components = entity.getComponents(Loader.getClass(comp.type), []); + const CompClass = Loader.getClass(comp.type); + if (!CompClass) { + console.warn(`ReflectionParser: $component type "${comp.type}" is not registered`); + return Promise.resolve(null); + } + const components = entity.getComponents(CompClass, []); return Promise.resolve(components[comp.index] ?? null); } return Promise.resolve(null); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/loader/src/resource-deserialize/resources/parser/ReflectionParser.ts` around lines 92 - 101, In the $component branch of ReflectionParser.ts guard against unregistered component types by calling Loader.getClass(comp.type) into a variable (e.g., compClass) and if it's undefined return Promise.resolve(null); otherwise pass compClass to entity.getComponents and return components[comp.index] ?? null — this mirrors the $type guard and prevents calling entity.getComponents(undefined, []) when comp.type is unregistered.
141-148: Same unguardedLoader.getClasspattern in signal resolution.Similar to the
$componentbranch,Loader.getClass(comp.type)at line 146 could returnundefinedfor unregistered types, leading toentity.getComponents(undefined, []).🛡️ Proposed guard
const promises = listeners.map((listener) => { const comp = listener.target.$component; const entity = this._context.entityMap.get(comp.entity); let targetComponent = null; if (entity) { - const components = entity.getComponents(Loader.getClass(comp.type), []); + const CompClass = Loader.getClass(comp.type); + if (!CompClass) { + console.warn(`ReflectionParser: $signal target type "${comp.type}" is not registered`); + return Promise.resolve(); + } + const components = entity.getComponents(CompClass, []); targetComponent = components[comp.index] ?? null; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/loader/src/resource-deserialize/resources/parser/ReflectionParser.ts` around lines 141 - 148, The code maps listeners and calls Loader.getClass(comp.type) without guarding for undefined which can pass undefined into entity.getComponents; update the listeners mapping in ReflectionParser (the block that sets comp = listener.target.$component and calls entity.getComponents(Loader.getClass(comp.type), [])) to first store the result of Loader.getClass(comp.type) in a variable (e.g., compClass), check if compClass is truthy, and only call entity.getComponents(compClass, []) when compClass is defined; if compClass is undefined, set targetComponent to null or skip that listener so you do not call getComponents(undefined, ...).packages/loader/src/resource-deserialize/resources/parser/HierarchyParser.ts (1)
106-110: Consider defensive check for missing child entities.If
children[j]references an index not inentityMap(malformed data),parent.addChild(undefined)could cause unexpected behavior. This is likely low risk if data is validated upstream, but a guard could improve debugging experience.🛡️ Proposed defensive check
const parent = entityMap.get(i); for (let j = 0, m = children.length; j < m; j++) { - parent.addChild(entityMap.get(children[j])); + const child = entityMap.get(children[j]); + if (child) { + parent.addChild(child); + } else { + console.warn(`HierarchyParser: child index ${children[j]} not found for entity ${i}`); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/loader/src/resource-deserialize/resources/parser/HierarchyParser.ts` around lines 106 - 110, Guard against missing child entries when iterating children in HierarchyParser: before calling parent.addChild(entityMap.get(children[j])), check that entityMap.has(children[j]) (or that the retrieved value is not undefined); if missing, either skip adding and emit a warning/error including the parent identifier and missing child index (or throw a descriptive error) to aid debugging. Update the loop that uses parent.addChild, referencing entityMap, children and parent.addChild to perform this defensive existence check and handle the malformed case consistently with project logging/error patterns.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@packages/loader/src/resource-deserialize/resources/parser/ReflectionParser.ts`:
- Around line 103-113: The $signal handling block in ReflectionParser.ts should
guard against a missing Signal instance: before calling this._resolveSignal with
originValue, check that originValue is not null/undefined (the resolved target
Signal instance); if it is missing, either skip registration or log/throw a
clear error. Update the code path that handles "$signal" to validate originValue
and only call _resolveSignal(originValue, obj.$signal) when originValue is
present, referencing the "$signal" branch and the _resolveSignal method to
locate the change.
---
Nitpick comments:
In
`@packages/loader/src/resource-deserialize/resources/parser/HierarchyParser.ts`:
- Around line 106-110: Guard against missing child entries when iterating
children in HierarchyParser: before calling
parent.addChild(entityMap.get(children[j])), check that
entityMap.has(children[j]) (or that the retrieved value is not undefined); if
missing, either skip adding and emit a warning/error including the parent
identifier and missing child index (or throw a descriptive error) to aid
debugging. Update the loop that uses parent.addChild, referencing entityMap,
children and parent.addChild to perform this defensive existence check and
handle the malformed case consistently with project logging/error patterns.
In
`@packages/loader/src/resource-deserialize/resources/parser/ReflectionParser.ts`:
- Around line 92-101: In the $component branch of ReflectionParser.ts guard
against unregistered component types by calling Loader.getClass(comp.type) into
a variable (e.g., compClass) and if it's undefined return Promise.resolve(null);
otherwise pass compClass to entity.getComponents and return
components[comp.index] ?? null — this mirrors the $type guard and prevents
calling entity.getComponents(undefined, []) when comp.type is unregistered.
- Around line 141-148: The code maps listeners and calls
Loader.getClass(comp.type) without guarding for undefined which can pass
undefined into entity.getComponents; update the listeners mapping in
ReflectionParser (the block that sets comp = listener.target.$component and
calls entity.getComponents(Loader.getClass(comp.type), [])) to first store the
result of Loader.getClass(comp.type) in a variable (e.g., compClass), check if
compClass is truthy, and only call entity.getComponents(compClass, []) when
compClass is defined; if compClass is undefined, set targetComponent to null or
skip that listener so you do not call getComponents(undefined, ...).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 06914411-9712-4f8e-9401-49d38798bee8
📒 Files selected for processing (5)
packages/loader/src/resource-deserialize/resources/parser/HierarchyParser.tspackages/loader/src/resource-deserialize/resources/parser/ReflectionParser.tspackages/loader/src/resource-deserialize/resources/scene/SceneParser.tstests/src/loader/PrefabResource.test.tstests/src/loader/SceneFormatV2.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/loader/src/resource-deserialize/resources/scene/SceneParser.ts
- ResourceManager.getResourceByRef 签名从 { url } 改为 { $ref }
- 删除 assetRefToEngine() 适配函数，所有调用方直传 AssetRef
- 删除旧类型 IAssetRef，统一使用 AssetRef { $ref, key? }
- GLTFExtensionSchema IGalaceanMaterialRemap.url → $ref
- AnimationClipLoader 资产引用检测从 .url 改为 .$ref
- 删除 GalaceanAssetInfo 接口，version 提升到 IHierarchyFile 顶层
- 删除三个接口中的 isLocked 编辑器专用字段
- 删除 IHierarchyFile 中的 _entityIds/_componentIds 编辑器专用字段
luzhuang
changed the title
- SceneSchema: color/solidColor/fogColor 类型从 Vec4Tuple | IColor 改为 Vec4Tuple - SceneSchema: shadowFourCascadeSplits 类型从 IVector3 改为 Vec3Tuple - SceneSchema: 删除对 BasicSchema 中 IColor/IVector3 的 import - SceneLoader: 删除 3 处 Array.isArray/copyFrom 双分支，只保留数组路径 - SceneLoader: shadowFourCascadeSplits 从 copyFrom(IVector3) 改为 set(tuple)
- types.ts: 去掉 Galacean* 前缀（EntitySchema/ComponentSchema/SceneFile 等）， IHierarchyFile → HierarchyFile，合并 SceneSchema 的 IScene 到 SceneFile， 删除死字段 customSpecularTexture 和 bakerResolution，SpecularMode 移入 types.ts - 删除 SceneSchema.ts（IScene/SpecularMode 已合并到 types.ts） - schema/index.ts: 删除对 scene-format/types 的 re-export， 改由 loader/index.ts 直接 export * from "./scene-format/types" - SceneLoader.ts: 提取 applySceneData 为独立导出函数，消除测试 false green - ParserContext.ts: 删除未使用的 phantom type T - GLTFExtensionSchema.ts: IGalaceanMaterialRemap extends AssetRef 复用类型 - 所有 parser/test 文件同步更新类型引用 - 测试: scene property 测试改为调用 applySceneData 生产代码， 新增 PrefabResource destroy-then-re-instantiate 生命周期测试
TextRenderer 构造时已通过 engine._textDefaultFont 设置默认字体， 不需要额外的 fontFamily 回退逻辑。该 handler 引入了不存在于 v2 格式规范中的 fontFamily 字段，属于超出需求范围的发明。
- types.ts: EntitySchema 和 InlineEntitySchema 改为 extends EntityOverrideProps， 消除 12 行重复字段声明 - ReflectionParser: 合并 _resolveValue 末尾 plain object 两个分支为一个循环， 提取 _resolveComponent helper 消除重复，简化 signal arguments 处理 - HierarchyParser: 删除构造函数 5 行 .bind(this)，start() 改用 inline arrow
- _resolveComponent: 恢复 v1 的 Loader.getClass null guard，未注册类型返回 null - _resolveSignal: 添加 signal 实例存在性检查，要求组件 Signal 字段必须已初始化 - 补充对应错误路径测试
|
@GuoLei1990 对照当前 PR head（ 先说结论：你这条 review 的整体方向是对的，尤其是从“这是一次结构性迁移，不只是局部修补”这个角度看问题很准确；但对照当前 head 来看，大部分代码层面的 P2/P3 已经在后续提交里处理掉了，现阶段我不打算继续为了这条 review 再改实现代码，剩下主要是 PR 表达和契约说明层面的事情。 已在后续提交中处理的点
你提到这里不应该用 private static _overrideKeyToPath(key: string): string {
return key.slice(1, -1).replace(/,/g, "/");
}也就是已经从“通用 JSON 解析”收敛到了“按已知格式做最小转换”。
你指出
你提这个点时是成立的，但这部分后续已经删掉了。当前
这部分后续已经从宽泛的 export type CustomParseComponentHandle = (
instance: any,
item: { props?: Record<string, unknown> }
) => Promise<any> | any;对应的静态字段也已经用
这两个字段后续已经去掉，当前 我保留但不继续改代码的点
你提到 我现在的理解是：
所以我倾向于把它视为“格式契约需要说清楚”，而不是继续在 parser 里增加隐式推断逻辑。也就是说，这条我认同它是一个需要让使用方知道的边界，但不认为还需要在当前实现上继续打补丁。
这个建议我理解，但我这边没有继续调整。原因不是它错，而是它更偏向风格/局部表达：这里的 我认同的问题，但它已经不再是“继续改 parser 代码”
不过对照当前实现，我认为它的落点已经不是“代码遗漏”，而是“PR 需要明确表达这次变更的真实性质”：
这也意味着这里更需要补的是 PR 标题/描述/兼容性说明，而不是再往 parser 里补一层回退逻辑。否则代码和 PR 叙述会继续不一致，reviewer 会天然按“新增 v2 支持但不该破坏 v1”来理解这次改动。 当前判断综合下来，我现在的处理思路是：
如果你觉得还有哪一条在当前 head 上仍然是“必须继续改代码”的，我们可以再基于最新 commit 具体看，不按旧 diff 状态讨论。 |
…romConfig helper - ReflectionParser: 删除 CustomParseComponentHandle 类型、静态字段、注册方法及 parseProps 中的 handle 调用 — TextRenderer handler 在 091649e 中已删除，整套机制无消费者 - ReflectionParser: $entity 空值返回 null 而非 undefined - HierarchyParser: 提取 _addComponentFromConfig 静态方法，消除 3 处重复的 getClass+guard+addComponent - 测试: 删除 customParseComponentHandles 测试，新增 $entity null、scene 属性解析覆盖
mock 对象缺少 _addReferCount 方法和 SphericalHarmonics3.coefficients， 导致引擎 setter 内部调用崩溃。
…核心职责 _isAssetRef 仅被 SceneParser._searchDependentAssets 一处调用，外层 else if 已保证 value 是非空 object，内联后只需 "$ref" in value 一个条件。
编辑器侧 shadow/fog/ambientOcclusion 字段全部 optional， 引擎类型定义同步改为 optional，applySceneData 统一使用 != undefined guard。 字段名保持与引擎 Scene 属性一致（fogMode 等），编辑器侧后续对齐。
|
|
removedEntities and removedComponents destroyed targets one-by-one via resolveEntity/resolveComponent, but destroy() synchronously mutates the parent children array (Entity.destroy -> _setParent(null) -> removeFromParent) and shifts subsequent sibling/component indices, so later paths in the same override batch either go out of range or delete the wrong target. Pre-resolve all targets before destroying any.
Extend $entity and $component.entity to accept a path array instead of a single flat index. First element is the flat index into top-level entities[]; subsequent elements descend into children arrays. This enables scene-level scripts to reference entities/components inside a prefab instance's subtree (e.g. CameraFollow.target -> Player prefab's Head bone), which the single-index form could not express. Also document the distinction vs override path fields (path/parent/target), which are instance-local and not affected.
|
@cptbtptpbcptdtptp @GuoLei1990 逐条回应： 已修复1. 确认是 bug。 已改为先预解析所有目标再批量 destroy：a66db9fef 覆盖测试：
2. 确认是能力收缩。已扩展
覆盖测试：
设计选择，不改3. 这是刻意的格式契约，不是 race。v2 约定：同一 MutationBlock 内 props 和 calls 独立——props 设置声明式属性，calls 执行命令式 setup 方法，依赖通过 call 自身参数传入（$ref / $entity / $component）而非从组件其他 prop 读。典型 setup 方法（ 若 builder 产出了"call 依赖同 block 内异步 prop"的组合，属于 builder 生成不当。真正需要顺序依赖的场景，格式层有 4. 这个机制论据不成立。 反之若按建议先 remove 后 add， 架构方向，本 PR 外5. 把 方向认可——全局 manifest 是业界共识（Unity Addressables / Cocos config.json）。但需要 runtime 从自描述 scene 切到强依赖全局 manifest，涉及依赖图构建、加载生命周期调整、增量切场景、按需卸载等，远超本 PR scope。当前 |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@cptbtptpbcptdtptp 认可。两处 |
|
回复一下最后这个 P2：我重新按当前实现核了一遍，这条在现状里应该不成立。
这次真正需要处理的是 destroy 过程中的索引漂移，这部分已经通过 pre-resolve targets 修掉了。 如果后续 |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
小建议：结构位置的 refs 索引统一用裸 number原则结构位置裸 number，值位置用
理由
小 fix协议里绝大多数结构位置的 refs 索引字段（ interface ComponentSchema extends MutationBlock {
type: string;
- script?: AssetRef;
+ script?: number;
}-"script": { "$ref": 2 }
+"script": 2解析侧同步改一行： const key = config.script
- ? resolveRefItem(refs, config.script.$ref, "HierarchyParser", "component.script").url
+ ? resolveRefItem(refs, config.script, "HierarchyParser", "component.script").url
: config.type; |
Schema: - ComponentSchema.script: AssetRef -> number, align with other structural refs index fields (instance.asset, background.texture) - Drop NormalEntitySchema.instance?: undefined, JSON never carries it - SceneSchema declares postProcess?: unknown, drops (as any) in loader Parser: - Extract _applyOverrides from _parsePrefabOverrides for clearer responsibility split - Cache refs = this.data.refs outside loops in _parseComponents and _parsePrefabOverrides - Replace 3x Promise.all(promises) as any with Promise<unknown> return type, zero runtime cost - Promise<any>[] -> Promise<unknown>[] for push-only buffers SceneLoader: - Introduce loadRef helper, removes 5 duplicated resolveRefItem + getResourceByRef + @ts-ignore blocks
- ResourceManager.getResourceByRef param field $ref -> url
- AssetRef type removed, unified to RefItem { url, key? }
- ParserContext.entityMap: Map -> Entity[] (array index access)
- entityMap -> entityInstances, componentPairs -> pendingComponents
- pendingComponents element field component -> instance
- All call sites and tests updated
_addDependentAsset is only called once per refs[] entry in SceneParser._collectDependentAssets. refs[] is deduplicated by the editor, so tasks.has(url) never returns true — it was dead code. Signature simplified: _addDependentAsset(promise) without url param.
…indices
Previously addedEntities.entity and addedComponents.component were inline
objects (InlineEntitySchema / ComponentSchema), preventing other objects
from referencing them via $entity / $component.
Now they are indices into the top-level entities[] / components[] arrays,
same as regular entities and components. This enables:
- Scripts can reference added entities via $entity: [index]
- Components on added entities can be referenced via $component
- Unified serialization path in the editor (no special inline case)
- Schema simplification: InlineEntitySchema type removed
- Runtime simplification: _createInlineEntity recursion removed
Added entities/components are still only attached to the prefab instance
subtree via addedEntities.parent / addedComponents.target paths. They are
created in Stage 1/3 like normal objects but not auto-attached as scene
roots (not in scene.entities) or child nodes (not referenced by any
entity.children), only by the addedEntities/addedComponents override.
Protocol change (breaking):
addedEntities: [{ parent, entity: InlineEntitySchema }]
-> addedEntities: [{ parent, entity: number }]
addedComponents: [{ target, component: ComponentSchema }]
-> addedComponents: [{ target, component: number }]
|
关于 refs[] 收归 project.json 的建议,这个架构选择值得展开谈一下。 cptbtptpbcptdtptp 列举的四个引擎（Unity Addressables / Unreal AssetRegistry / Cocos config.json / PlayCanvas）都是发布型引擎——交付物是完整应用,scene 文件永远在自己的 registry 体系里使用,用户下载整个项目才能玩。registry 模式对它们合理。 但 Galacean 走的是另一条路——生态型引擎。这两种引擎的本质区别: 发布型(Unity/Unreal/Cocos/PlayCanvas)
生态型(Galacean)
资产库场景:registry 路线的根本冲突对 Galacean 来说,资产库场景是核心——不像 Unity 需要加载 bundle,Galacean 应该能纯粹地加载单个 scene 或 prefab。运行时真正的跨项目复用,不依赖编辑器项目重新导入导出。 资产库的本质就是"每个资产独立可消费":设计师上传一个 prefab,消费方(任何项目)拿到 URL 就能用。registry 模式在这里存在根本冲突——如果每个资产都必须挂在某个 project.json 下才能加载,资产库就退化成了"预打包项目的分发渠道",而不是真正的资产库。这对资产库建设是致命的:
自描述不只是"生态",也是开发者体验的全面优势除了跨项目复用和生态友好,自描述对单个项目内的开发体验同样有决定性优势: 加载 API 简洁 // 自描述:一步
const prefab = await resourceManager.load("Player.prefab");
// registry:两步 + 全局初始化
await resourceManager.load("project.json");
const prefab = await resourceManager.load("Player.prefab");跨项目复用零接入成本
单文件可读可调试
CDN 缓存友好
嵌入到其他应用
Web 开发者心智模型
registry 被援引的 4 个收益分析cptbtptpbcptdtptp 列的四个收益在当前架构下都不成问题:
这是策略层选择,不是实现优化这不是"方向认可但以后再考虑"的问题——是 Galacean 的设计路线选择。自描述与"资产库独立使用 + Web 原生 URL 可寻址 + 跨项目复用 + 易用性"的定位一致,registry 模式和这个定位互斥。走 registry 路线会关闭资产库发展的可能性,不应走那条路。 基于这次 review 的讨论,我提了一个补充的优化 PR: luzhuang#41 在
87 个 loader tests 全部通过。PR #41 描述里详细展开了每条改动,包括自描述设计立场的完整论证。 |
… number Align with other structure-position ref fields (instance.asset, background.texture, etc.) which all use bare number indices into the top-level refs array.
|
回复 @cptbtptpbcptdtptp 的三条评论： 1.
|
|
回复 @GuoLei1990： ComponentSchema.script → 裸 number已改。 P2: Override 应用顺序确认过代码： 如果后续 |
Replace appendSuffix (order-dependent) with captureResolvedArgs (order-independent) in MutationBlock tests. Props and calls are verified separately, matching the parallel execution design.
…ization refactor(loader): protocol consistency + honest types + quality cleanup - ComponentSchema.script: AssetRef → number (aligned at merge) - addedEntities/addedComponents: inline → top-level indices - entityMap: Map → Entity[] - getResourceByRef param $ref → url (eliminate @ts-ignore) - rename componentPairs → pendingComponents, component → instance - delete AssetRef (unified to RefItem) - delete NormalEntitySchema.instance?: undefined - delete ParserContext._tasks dead code - Promise.all() as any → Promise<unknown> - SceneLoader loadRef helper - extract _applyOverrides method
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Only contained a single skipped empty test with a stale TODO. All v2 scene parsing coverage is in SceneFormatV2.test.ts.
The benchmark test creates 120 PrefabResource instances with unique URLs (Math.random()) across two test cases, and never destroys the outer prefab references. With v8 coverage instrumentation and the CI macos runner memory budget, this caused the Test step to hang indefinitely (38+ min vs 77s baseline). Performance observation tests should not gate CI; run locally via 'npx vitest run tests/src/loader/PrefabBenchmark.test.ts' instead.
Replace the previous --exclude workaround with outright deletion. The benchmark was a performance observation test (weak expect(avg > 0) assertions) with resource leaks (120 outer PrefabResource never destroyed) that hung CI coverage for 38+ minutes. Revert package.json coverage script to baseline.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
GuoLei1990
left a comment
GuoLei1990
left a comment
There was a problem hiding this comment.
总结
重写 loader/parser 子系统以原生消费 v2 scene/prefab 格式。v2 格式使用扁平 entity/component 数组 + 整数索引引用（替代字符串 ID）、$ 前缀值类型（$ref/$type/$entity/$component/$signal）、顶层 refs[] 集中资产引用。Prefab 实例支持完整 override 系统。
架构方向正确。ParserContext 从 6 个 Map 简化为 2 个数组，_collectDependentAssets 从递归树遍历简化为 refs[] 线性遍历，5 阶段管线（parseEntities → organize → addComponents → propsAndCalls → prefabOverrides）职责清晰。测试覆盖充分。
问题
-
[P1]
parseMutationBlock并行执行 props 和 calls — 违反调用顺序保证 —ReflectionParser.parseMutationBlock通过Promise.all同时执行parseProps和parseCalls。如果一个 call 的方法依赖某个 prop 已经被赋值（$ref类型的 prop 是异步的），就会出现竞态。虽然注释说"不强制 props/calls 顺序"，但这是反直觉的——序列化数据中，先设属性再调方法是自然预期。如果 call 的参数不依赖同 block 的 props（编辑器序列化保证），需要在注释中显式声明这个前提条件。否则建议改为顺序执行：先parseProps再parseCalls，性能代价可忽略。 -
[P2]
applySceneData标记@internal但测试通过路径绕过 barrel import —SceneLoader.ts导出applySceneData并标@internal，index.ts只做 side-effect import。测试直接 import"../../../packages/loader/src/SceneLoader"绕过 barrel。如果函数确实是 internal，测试应通过内部路径 import（加注释说明原因）。如果未来有 editor 消费需求，考虑export { applySceneData } from "./SceneLoader"并改为@beta。 -
[P2]
_componentBuffer静态共享数组跨 parser 耦合 —ReflectionParser._componentBuffer被HierarchyParser._resolveComponent共享使用，契约是"同步 length=0 → getComponents → 读取 → length=0"。当前安全（单线程同步调用），但隐式耦合。如果getComponents变为异步或并行加载多个场景，会静默数据污染。建议改为调用点局部数组或实例级 buffer。 -
[P3]
applySceneData中 try-catch 只包裹 ambient + background，shadow/fog/AO 在外面 — 结构不对称。要么统一包裹，要么移除 try-catch（外层 caller 已有.catch(reject)）。
简化建议
parseMutationBlock顺序化后更简单也更安全resolveRefItem（schema/refs.ts）只有 4 行逻辑、3 个调用点，可考虑内联为数组越界检查 helper，减少一个文件
整体是优质重构。LGTM，P1 修复后可合入。
3 participants
Summary
$ref,$type,$entity,$component,$signalvalue resolutionassetRefToEngine()utility for v2{ $ref }→ engine{ url }conversionTest plan
$typepolymorphic resolutionSummary by CodeRabbit
New Features
Improvements
Tests
Chores