Replace refId with url for asset reference resolution#2913
Replace refId with url for asset reference resolution#2913GuoLei1990 merged 12 commits intogalacean:dev/2.0from
Conversation
Migrate asset references from opaque refId to url-based (virtualPath) lookup. Remove _idResourceMap in favor of direct _virtualPathResourceMap resolution. Cache loaded assets in _objectPool by url and add type fallback in getResourceByRef.
…ceByRef Let _loadSingleItem handle virtualPath → CDN path resolution internally via _virtualPathResourceMap, avoiding double ?q= parsing that caused request timeouts. Remove redundant params forwarding.
…, add null check for _addDependenceAsset - Rename objectId to url in TextureDecoder to align with refId→url refactor - Restore _objectPool[url] cache write in TextureDecoder (editor will write url) - Add null guard before _addDependenceAsset in ReflectionParser - Remove extra _objectPool write logic from getResourceByRef
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## dev/2.0 #2913 +/- ##
===========================================
- Coverage 78.31% 78.12% -0.19%
===========================================
Files 883 881 -2
Lines 96714 96701 -13
Branches 9697 9703 +6
===========================================
- Hits 75739 75548 -191
- Misses 20812 20990 +178
Partials 163 163
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:
|
zhuxudong
changed the title
|
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:
WalkthroughRefactors asset references from ID-based ( Changes
Sequence Diagram(s)sequenceDiagram
participant SceneParser
participant ReflectionParser
participant ResourceManager
participant Loader
participant ObjectPool
SceneParser->>ReflectionParser: parse entity (contains assetUrl)
ReflectionParser->>ResourceManager: getResourceByRef({url, key?, isClone?})
ResourceManager->>ObjectPool: lookup by url
alt cached
ObjectPool-->>ResourceManager: Resource
ResourceManager-->>ReflectionParser: return (clone if isClone)
else not cached
ResourceManager->>Loader: load by mapped type + url (params)
Loader-->>ResourceManager: loaded Resource
ResourceManager->>ObjectPool: store under url
ResourceManager-->>ReflectionParser: return loaded Resource
end
ReflectionParser->>SceneParser: resolved entity (add dependency only if resource exists)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
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: 2
🧹 Nitpick comments (2)
tests/src/core/resource/ResourceManager.test.ts (1)
81-89: Clarify test intent: URL doesn't appear to have an "invalid q value".The test is named "invalid q case" and the comment states "contains invalid q value cdn url", but the new URL
https://mdn.alipayobjects.com/rms/afts/file/A*2onGSK7cXY4AAAAAQJAAAAgAehQnAQ/project.jsondoesn't contain a?q=parameter.If this test is meant to verify behavior when loading returns undefined, the test name and comment should be updated to reflect the actual scenario being tested.
📝 Suggested clarification
describe("gltf subAsset load", () => { - it("invalid q case", async () => { + it("project load returns undefined", async () => { const loadRes = await engine.resourceManager.load({ - // contains invalid q value cdn url. + // Test URL that results in undefined load result url: "https://mdn.alipayobjects.com/rms/afts/file/A*2onGSK7cXY4AAAAAQJAAAAgAehQnAQ/project.json", type: AssetType.Project }); expect(loadRes).to.equal(undefined); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/src/core/resource/ResourceManager.test.ts` around lines 81 - 89, The test name and comment are misleading: update the "gltf subAsset load" test case (the it block calling engine.resourceManager.load) to reflect the actual scenario — the provided CDN URL does not include a ?q= parameter and the test asserts loadRes is undefined; rename the it title from "invalid q case" to something like "returns undefined for CDN URL without q param" and update the inline comment to explain that the URL lacks a q parameter (or otherwise describe the real condition being tested).packages/core/src/asset/ResourceManager.ts (1)
594-602: Potential issue: Type lookup may fail for assets not in_virtualPathResourceMap.When
_virtualPathResourceMap[url]is undefined (e.g., for direct CDN URLs not registered viainitVirtualResources), the code falls back toref.type. However, many callers pass objects that may not have atypeproperty (as seen in context snippets with@ts-ignore).If both
mapped?.typeandref.typeare undefined,_assignDefaultOptionswill attempt to infer type from URL extension (line 343), which should work for standard assets. This is acceptable but worth noting for debugging.📝 Optional: Add debug logging for type resolution
const mapped = this._virtualPathResourceMap[url]; + const resolvedType = mapped?.type ?? ref.type; + if (!resolvedType) { + Logger.debug(`ResourceManager.getResourceByRef: No explicit type for "${url}", will infer from extension.`); + } const promise = this.load<T>({ url: loadUrl, - type: mapped?.type ?? ref.type + type: resolvedType });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/asset/ResourceManager.ts` around lines 594 - 602, The type lookup can be undefined when _virtualPathResourceMap[url] is missing and ref.type is absent, causing downstream ambiguity; update the ResourceManager code around loadUrl and the call to this.load<T> (referencing _virtualPathResourceMap, loadUrl, load<T>, and _assignDefaultOptions) to explicitly resolve type by: 1) preferring mapped?.type, 2) falling back to ref?.type, and 3) if still undefined, explicitly compute a type from the URL extension (reuse the same logic as _assignDefaultOptions or call a small helper) before calling this.load; optionally emit a debug log when the resolved type was inferred to aid debugging.
🤖 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/HierarchyParser.ts`:
- Around line 313-316: The current code uses componentConfig.url as the class
key which is invalid; change the lookup to use componentConfig.class (i.e. const
key = componentConfig.class) when calling Loader.getClass, then call
entity.addComponent(Loader.getClass(key)); if Loader.getClass(key) returns
undefined, handle it (throw or log and skip) before calling entity.addComponent
to avoid "undefined is not a constructor"; keep the
componentMap.set(componentConfig.id, component) once a valid component instance
is created.
In `@packages/loader/src/resource-deserialize/resources/scene/SceneParser.ts`:
- Around line 30-37: The patch can pass falsy URLs into
context._addDependentAsset which may add "undefined" to tracking; update the
SceneParser logic so you only call context._addDependentAsset(...) when the URL
is truthy: check customAmbientLight.url before calling
context._addDependentAsset for the customAmbientLight branch and check
ambientLight.url before calling context._addDependentAsset for the ambientLight
branch (preserving the existing guard conditions useCustomAmbient/useSH); use
the same null/empty-string defensive pattern used elsewhere and still call
resourceManager.getResourceByRef(...) only when the URL is valid.
---
Nitpick comments:
In `@packages/core/src/asset/ResourceManager.ts`:
- Around line 594-602: The type lookup can be undefined when
_virtualPathResourceMap[url] is missing and ref.type is absent, causing
downstream ambiguity; update the ResourceManager code around loadUrl and the
call to this.load<T> (referencing _virtualPathResourceMap, loadUrl, load<T>, and
_assignDefaultOptions) to explicitly resolve type by: 1) preferring
mapped?.type, 2) falling back to ref?.type, and 3) if still undefined,
explicitly compute a type from the URL extension (reuse the same logic as
_assignDefaultOptions or call a small helper) before calling this.load;
optionally emit a debug log when the resolved type was inferred to aid
debugging.
In `@tests/src/core/resource/ResourceManager.test.ts`:
- Around line 81-89: The test name and comment are misleading: update the "gltf
subAsset load" test case (the it block calling engine.resourceManager.load) to
reflect the actual scenario — the provided CDN URL does not include a ?q=
parameter and the test asserts loadRes is undefined; rename the it title from
"invalid q case" to something like "returns undefined for CDN URL without q
param" and update the inline comment to explain that the URL lacks a q parameter
(or otherwise describe the real condition being tested).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d5c02d5d-36f8-450a-b110-5e95ddc13f16
📒 Files selected for processing (12)
e2e/case/project-loader.tspackages/core/src/asset/ResourceManager.tspackages/loader/src/AnimationClipLoader.tspackages/loader/src/gltf/extensions/GLTFExtensionSchema.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/texture2D/TextureDecoder.tstests/src/core/resource/ResourceManager.test.tstests/src/loader/SceneParser.test.ts
| const key = !componentConfig.url ? componentConfig.class : componentConfig.url; | ||
| const componentId = componentConfig.id; | ||
| const component = entity.addComponent(Loader.getClass(key)); | ||
| componentMap.set(componentId, component); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check the Loader.getClass implementation
cat -n packages/core/src/asset/Loader.ts | head -50Repository: galacean/engine
Length of output: 1761
🏁 Script executed:
# Check the HierarchyParser context around lines 313-316
cat -n packages/loader/src/resource-deserialize/resources/parser/HierarchyParser.ts | sed -n '300,330p'Repository: galacean/engine
Length of output: 1410
🏁 Script executed:
# Search for componentConfig structure and initialization
rg -n 'componentConfig\s*=' packages/loader/src/resource-deserialize/resources/parser/HierarchyParser.ts -A 3 -B 1 | head -40Repository: galacean/engine
Length of output: 661
🏁 Script executed:
# Check how Loader.getClass is used elsewhere in the codebase
rg -n 'Loader\.getClass\(' --type=ts -C 2 | head -50Repository: galacean/engine
Length of output: 4638
🏁 Script executed:
# Check component registration patterns
rg -n 'registerClass|_engineObjects' packages/core/src/asset/Loader.ts -C 3Repository: galacean/engine
Length of output: 783
🏁 Script executed:
# Check addComponent implementation to see what happens with undefined/null
rg -n 'addComponent\(' packages/core/src/entity/Entity.ts -A 5 | head -30Repository: galacean/engine
Length of output: 133
🏁 Script executed:
# Check what componentConfig structure looks like - search for IEntity interface
rg -n 'interface IEntity|class IEntity' --type=ts -A 15 | head -50Repository: galacean/engine
Length of output: 41
🏁 Script executed:
# Check if there's any registration of URL values as class keys
rg -n 'componentConfig\.url|\.url\s*=' packages/loader/src/resource-deserialize/resources/parser/HierarchyParser.ts -B 2 -A 2Repository: galacean/engine
Length of output: 379
🏁 Script executed:
# Check for any transformations or registrations of componentConfig.url before being used as key
rg -n 'parseClassObject|_parseEntity|_addComponents' packages/loader/src/resource-deserialize/resources/parser/HierarchyParser.ts -A 3 -B 1 | head -60Repository: galacean/engine
Length of output: 1176
🏁 Script executed:
# Find Entity class location
fd -n 'Entity.ts' --type=f | head -20Repository: galacean/engine
Length of output: 289
🏁 Script executed:
# Search for addComponent method definition
rg -n 'addComponent\s*\(' --type=ts -A 3 | grep -A 3 'addComponent' | head -30Repository: galacean/engine
Length of output: 1870
🏁 Script executed:
# Check the type definitions for componentConfig - look for IEntity or component interface
fd -n '\.d\.ts' --type=f | xargs grep -l 'interface IEntity\|type IEntity' | head -5Repository: galacean/engine
Length of output: 289
🏁 Script executed:
# Search for where componentConfig.url is set or populated
rg -n 'url\s*:' --type=ts packages/loader/src/resource-deserialize -B 2 -A 1 | head -60Repository: galacean/engine
Length of output: 2212
🏁 Script executed:
# Find Entity class and addComponent method
fd Entity.ts --type=f | head -10Repository: galacean/engine
Length of output: 86
🏁 Script executed:
# Look for IEntity interface definition in schema files
rg -n 'interface IEntity\|type IEntity' --type=ts -A 10 | head -40Repository: galacean/engine
Length of output: 41
🏁 Script executed:
# Check what happens when addComponent receives undefined
rg -n 'addComponent\s*\(' packages/core --type=ts -A 5 | head -50Repository: galacean/engine
Length of output: 2019
🏁 Script executed:
# Look at the schema definitions to understand componentConfig structure
fd BasicSchema.ts --type=fRepository: galacean/engine
Length of output: 131
🏁 Script executed:
# Read the Entity.ts file around the addComponent method
cat packages/core/src/Entity.ts | head -350 | tail -100Repository: galacean/engine
Length of output: 3301
🏁 Script executed:
# Read BasicSchema.ts to see IEntity definition
cat packages/loader/src/resource-deserialize/resources/schema/BasicSchema.ts | head -150Repository: galacean/engine
Length of output: 2178
🏁 Script executed:
# Check if there's any null check or validation before using Loader.getClass result
rg -n 'Loader\.getClass' packages/loader/src/resource-deserialize/resources/parser/HierarchyParser.ts -B 3 -A 3Repository: galacean/engine
Length of output: 1226
🏁 Script executed:
# Check if componentConfig.url values are ever registered as class keys
rg -n 'registerClass.*url|_engineObjects\[.*url' --type=tsRepository: galacean/engine
Length of output: 41
🏁 Script executed:
# Look for any transformation of URL to class name before registration
rg -n 'registerClass' packages/core --type=ts -B 2 -A 2Repository: galacean/engine
Length of output: 475
🏁 Script executed:
# Check what happens in addComponent when passed undefined by looking at ComponentsDependencies
cat packages/core/src/ComponentsDependencies.ts | head -50Repository: galacean/engine
Length of output: 1789
🏁 Script executed:
# Verify if there are any tests that cover this scenario
fd test.ts --type=f | xargs rg -l 'addComponent.*Loader\.getClass' 2>/dev/nullRepository: galacean/engine
Length of output: 43
Line 313-315 will fail at runtime: componentConfig.url is not a registered class key.
Loader.getClass() only resolves class names registered via registerClass(). URLs are asset identifiers and are never registered as class keys. If componentConfig.url is present, Loader.getClass(url) returns undefined, causing entity.addComponent(undefined) to throw "undefined is not a constructor" at runtime.
The logic also inverts the intended fallback: it prefers url over the guaranteed class property, unlike the consistent pattern at line 240 (Loader.getClass(transform.class)).
Suggested fix
- const key = !componentConfig.url ? componentConfig.class : componentConfig.url;
+ const key = componentConfig.class;
const componentId = componentConfig.id;
- const component = entity.addComponent(Loader.getClass(key));
+ const ComponentClass = Loader.getClass(key);
+ if (!ComponentClass) {
+ throw new Error(`Component class not found: ${key}`);
+ }
+ const component = entity.addComponent(ComponentClass);📝 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.
| const key = !componentConfig.url ? componentConfig.class : componentConfig.url; | |
| const componentId = componentConfig.id; | |
| const component = entity.addComponent(Loader.getClass(key)); | |
| componentMap.set(componentId, component); | |
| const key = componentConfig.class; | |
| const componentId = componentConfig.id; | |
| const ComponentClass = Loader.getClass(key); | |
| if (!ComponentClass) { | |
| throw new Error(`Component class not found: ${key}`); | |
| } | |
| const component = entity.addComponent(ComponentClass); | |
| componentMap.set(componentId, component); |
🤖 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 313 - 316, The current code uses componentConfig.url as the class
key which is invalid; change the lookup to use componentConfig.class (i.e. const
key = componentConfig.class) when calling Loader.getClass, then call
entity.addComponent(Loader.getClass(key)); if Loader.getClass(key) returns
undefined, handle it (throw or log and skip) before calling entity.addComponent
to avoid "undefined is not a constructor"; keep the
componentMap.set(componentConfig.id, component) once a valid component instance
is created.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/core/src/asset/ResourceManager.ts`:
- Around line 579-583: The inline object parameter type on getResourceByRef
causes a Prettier lint failure—replace the one-line inline type with a properly
formatted type (either a small named type alias like ResourceRef { url: string;
key?: string; isClone?: boolean } and use getResourceByRef<T extends
EngineObject>(ref: ResourceRef): AssetPromise<T>, or expand the inline type into
a multiline shape) so the signature conforms to the formatter; update the
getResourceByRef declaration accordingly.
- Around line 589-603: The fast-path ignores the optional key and can return the
wrong cached sibling; fix by canonicalizing a cacheKey = key ? `${url}?q=${key}`
: url and use cacheKey for the _objectPool lookup, for constructing loadUrl, and
when accessing _virtualPathResourceMap and calling this.load; keep the clone
logic (IClone / AssetPromise.resolve and promise.then clone) but operate on the
cached value found via cacheKey so `{url, key}` are treated as distinct entries.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: a9e766d4-3b54-448f-8ca6-4a7a07291e43
📒 Files selected for processing (3)
packages/core/src/asset/ResourceManager.tspackages/loader/src/resource-deserialize/resources/parser/ReflectionParser.tspackages/loader/src/resource-deserialize/resources/schema/BasicSchema.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/loader/src/resource-deserialize/resources/parser/ReflectionParser.ts
| getResourceByRef<T extends EngineObject>(ref: { | ||
| url: string; | ||
| key?: string; | ||
| isClone?: boolean; | ||
| }): AssetPromise<T> { |
There was a problem hiding this comment.
Fix the Prettier violation on the new parameter type.
This signature is the current lint failure; collapsing the inline type to the formatter-preferred shape should clear the check.
💅 Proposed fix
- getResourceByRef<T extends EngineObject>(ref: {
- url: string;
- key?: string;
- isClone?: boolean;
- }): AssetPromise<T> {
+ getResourceByRef<T extends EngineObject>(ref: { url: string; key?: string; isClone?: boolean }): AssetPromise<T> {📝 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.
| getResourceByRef<T extends EngineObject>(ref: { | |
| url: string; | |
| key?: string; | |
| isClone?: boolean; | |
| }): AssetPromise<T> { | |
| getResourceByRef<T extends EngineObject>(ref: { url: string; key?: string; isClone?: boolean }): AssetPromise<T> { |
🧰 Tools
🪛 ESLint
[error] 579-583: Replace ⏎····url:·string;⏎····key?:·string;⏎····isClone?:·boolean;⏎· with ·url:·string;·key?:·string;·isClone?:·boolean
(prettier/prettier)
🪛 GitHub Check: lint
[failure] 579-579:
Replace ⏎····url:·string;⏎····key?:·string;⏎····isClone?:·boolean;⏎· with ·url:·string;·key?:·string;·isClone?:·boolean
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/src/asset/ResourceManager.ts` around lines 579 - 583, The
inline object parameter type on getResourceByRef causes a Prettier lint
failure—replace the one-line inline type with a properly formatted type (either
a small named type alias like ResourceRef { url: string; key?: string; isClone?:
boolean } and use getResourceByRef<T extends EngineObject>(ref: ResourceRef):
AssetPromise<T>, or expand the inline type into a multiline shape) so the
signature conforms to the formatter; update the getResourceByRef declaration
accordingly.
| const cached = this._objectPool[url]; | ||
| if (cached) { | ||
| return isClone ? AssetPromise.resolve(<T>(<IClone>(<unknown>cached)).clone()) : AssetPromise.resolve(cached); | ||
| } | ||
| return promise.then((item) => (isClone ? <T>(<IClone>(<unknown>item)).clone() : item)); | ||
| let loadUrl = url; | ||
| if (key) { | ||
| loadUrl += "?q=" + key; | ||
| } | ||
| const mapped = this._virtualPathResourceMap[url]; | ||
| const promise = this.load<T>({ | ||
| url: loadUrl, | ||
| type: mapped?.type, | ||
| params: mapped?.params | ||
| }); | ||
| return isClone ? promise.then((item) => <T>(<IClone>(<unknown>item)).clone()) : promise; |
There was a problem hiding this comment.
Treat key as part of the cached ref.
packages/loader/src/resource-deserialize/resources/schema/BasicSchema.ts:104 now defines refs as { url, key? }, so key is the only discriminator for sibling sub-assets. The fast path still returns _objectPool[url] directly, which can make { url: "foo", key: "textures[0]" } and { url: "foo", key: "textures[1]" } resolve to the same cached object once the pool is warm. Canonicalize the full keyed URL and use it consistently for cache lookup/loading, or resolve the keyed sub-resource from the cached root before returning.
🛠️ Proposed fix
- const cached = this._objectPool[url];
+ const fullUrl = key
+ ? `${url}${url.includes("?") ? "&" : "?"}q=${encodeURIComponent(key)}`
+ : url;
+ const cachedRoot = this._objectPool[url];
+ const cached =
+ this._objectPool[fullUrl] ??
+ (key && cachedRoot ? this._getResolveResource(cachedRoot, this._parseQueryPath(key)) : cachedRoot);
if (cached) {
return isClone ? AssetPromise.resolve(<T>(<IClone>(<unknown>cached)).clone()) : AssetPromise.resolve(cached);
}
- let loadUrl = url;
- if (key) {
- loadUrl += "?q=" + key;
- }
+ const loadUrl = fullUrl;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/src/asset/ResourceManager.ts` around lines 589 - 603, The
fast-path ignores the optional key and can return the wrong cached sibling; fix
by canonicalizing a cacheKey = key ? `${url}?q=${key}` : url and use cacheKey
for the _objectPool lookup, for constructing loadUrl, and when accessing
_virtualPathResourceMap and calling this.load; keep the clone logic (IClone /
AssetPromise.resolve and promise.then clone) but operate on the cached value
found via cacheKey so `{url, key}` are treated as distinct entries.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/core/src/asset/ResourceManager.ts (1)
586-599:⚠️ Potential issue | 🟠 MajorCache lookup ignores
key, causing incorrect sub-asset resolution.When a root asset is already in
_objectPool, requests for distinct sub-assets (e.g.,{ url: "model.glb", key: "textures[0]" }vs{ url: "model.glb", key: "textures[1]" }) will both return the cached root object instead of resolving to the appropriate sub-resource.The
keyshould either be incorporated into the cache key, or the cached root should be traversed using_parseQueryPathand_getResolveResourceto extract the correct sub-asset before returning.🛠️ Proposed fix
getResourceByRef<T extends EngineObject>(ref: { url: string; key?: string; isClone?: boolean }): AssetPromise<T> { const { url, key, isClone } = ref; if (!url) { Logger.warn("ResourceManager.getResourceByRef: url is empty."); return AssetPromise.resolve(null); } - const cached = this._objectPool[url]; - if (cached) { - return AssetPromise.resolve(isClone ? <T>(<IClone>(<unknown>cached)).clone() : cached); + const cachedRoot = this._objectPool[url]; + if (cachedRoot) { + const resolved = key ? this._getResolveResource(cachedRoot, this._parseQueryPath(key)) : cachedRoot; + return AssetPromise.resolve(isClone ? <T>(<IClone>(<unknown>resolved)).clone() : resolved); } const mapped = this._virtualPathResourceMap[url];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/asset/ResourceManager.ts` around lines 586 - 599, ResourceManager.getResourceByRef currently returns the cached root from _objectPool without considering the query key, causing sub-asset mixups; modify the cached branch so that if a cached object exists and a key/query is provided you parse the key with _parseQueryPath and resolve the specific sub-resource via _getResolveResource (then clone if isClone), otherwise fall back to returning/ cloning the cached root; use _objectPool, _parseQueryPath, _getResolveResource, and preserve the existing behavior for mapped/load when not cached.
🤖 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/core/src/asset/ResourceManager.ts`:
- Around line 586-599: ResourceManager.getResourceByRef currently returns the
cached root from _objectPool without considering the query key, causing
sub-asset mixups; modify the cached branch so that if a cached object exists and
a key/query is provided you parse the key with _parseQueryPath and resolve the
specific sub-resource via _getResolveResource (then clone if isClone), otherwise
fall back to returning/ cloning the cached root; use _objectPool,
_parseQueryPath, _getResolveResource, and preserve the existing behavior for
mapped/load when not cached.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 8554787c-6170-443c-a609-637708908291
📒 Files selected for processing (1)
packages/core/src/asset/ResourceManager.ts
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/Texture2DLoader.ts`:
- Around line 114-117: The branch in Texture2DLoader._createTexture currently
always calls texture.generateMipmaps() after setImageSource(img); instead,
restore the original mipmap policy by checking the same conditions used
elsewhere: only call texture.generateMipmaps() when the initial params indicate
mipmaps were desired and TextureUtils.supportGenerateMipmapsWithCorrection(...)
returns true (or whatever boolean you used originally to decide mip generation).
Update the branch to mirror the original logic so restored textures respect the
original mipmap flag and supported sizes/formats, using the same param name(s)
and TextureUtils.supportGenerateMipmapsWithCorrection(...) check as in the rest
of _createTexture.
- Around line 43-49: The current branch only calls decode<Texture2D> when
FileHeader.checkMagic(buffer) is true, which prevents serialized-but-no-MAGIC
textures from being decoded; change the condition to ask FileHeader's new
header-detection/parsing API (the function you added that recognizes headers
with or without MAGIC) and if that parser reports the buffer is a serialized
texture, call decode<Texture2D>(buffer, resourceManager.engine). After decoding,
continue to add the restorer via resourceManager.addContentRestorer(new
Texture2DContentRestorer(...)) and resolve, otherwise fall back to
loadImageFromBuffer(buffer) as before.
- Around line 19-29: loadImageFromBuffer leaks the created object URL on image
decode failures because URL.revokeObjectURL is only called in img.onload; modify
loadImageFromBuffer so that the blob URL created by URL.createObjectURL(blob) is
stored in a local variable and revoked both in img.onload and in img.onerror
(and ensure img.onerror calls reject after revoking). Update references to
img.src to use that local url variable when calling revoke in both handlers to
guarantee the URL is always released.
- Around line 73-75: Reflow the call to
TextureUtils.supportGenerateMipmapsWithCorrection so it satisfies the formatter:
put each argument on its own line and move the closing parenthesis and semicolon
to their own line, e.g. assign to generateMipmap =
TextureUtils.supportGenerateMipmapsWithCorrection( then list engine, width,
height, format, mipmap, isSRGBColorSpace each on separate indented lines, then
close with ); to unblock Prettier in the Texture2DLoader.ts call site.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: c4b84029-89e1-409f-878a-b88080abf65c
📒 Files selected for processing (7)
packages/loader/src/Texture2DContentRestorer.tspackages/loader/src/Texture2DLoader.tspackages/loader/src/resource-deserialize/index.tspackages/loader/src/resource-deserialize/resources/scene/EditorTextureLoader.tspackages/loader/src/resource-deserialize/resources/texture2D/TextureDecoder.tspackages/loader/src/resource-deserialize/utils/FileHeader.tstests/src/core/DeviceLost.test.ts
💤 Files with no reviewable changes (3)
- packages/loader/src/resource-deserialize/resources/scene/EditorTextureLoader.ts
- packages/loader/src/Texture2DContentRestorer.ts
- packages/loader/src/resource-deserialize/index.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/loader/src/resource-deserialize/resources/texture2D/TextureDecoder.ts
| function loadImageFromBuffer(buffer: ArrayBuffer): AssetPromise<HTMLImageElement> { | ||
| return new AssetPromise((resolve, reject) => { | ||
| const blob = new Blob([buffer]); | ||
| const img = new Image(); | ||
| img.onload = () => { | ||
| URL.revokeObjectURL(img.src); | ||
| resolve(img); | ||
| }; | ||
| img.onerror = reject; | ||
| img.src = URL.createObjectURL(blob); | ||
| }); |
There was a problem hiding this comment.
Revoke the blob URL on onerror too.
The helper only releases the object URL in onload. Failed image decodes leak the blob URL, and this helper is shared by both load and restore paths.
🧹 Suggested fix
function loadImageFromBuffer(buffer: ArrayBuffer): AssetPromise<HTMLImageElement> {
return new AssetPromise((resolve, reject) => {
const blob = new Blob([buffer]);
const img = new Image();
+ const objectURL = URL.createObjectURL(blob);
img.onload = () => {
- URL.revokeObjectURL(img.src);
+ URL.revokeObjectURL(objectURL);
resolve(img);
};
- img.onerror = reject;
- img.src = URL.createObjectURL(blob);
+ img.onerror = (event) => {
+ URL.revokeObjectURL(objectURL);
+ reject(event);
+ };
+ img.src = objectURL;
});
}📝 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.
| function loadImageFromBuffer(buffer: ArrayBuffer): AssetPromise<HTMLImageElement> { | |
| return new AssetPromise((resolve, reject) => { | |
| const blob = new Blob([buffer]); | |
| const img = new Image(); | |
| img.onload = () => { | |
| URL.revokeObjectURL(img.src); | |
| resolve(img); | |
| }; | |
| img.onerror = reject; | |
| img.src = URL.createObjectURL(blob); | |
| }); | |
| function loadImageFromBuffer(buffer: ArrayBuffer): AssetPromise<HTMLImageElement> { | |
| return new AssetPromise((resolve, reject) => { | |
| const blob = new Blob([buffer]); | |
| const img = new Image(); | |
| const objectURL = URL.createObjectURL(blob); | |
| img.onload = () => { | |
| URL.revokeObjectURL(objectURL); | |
| resolve(img); | |
| }; | |
| img.onerror = (event) => { | |
| URL.revokeObjectURL(objectURL); | |
| reject(event); | |
| }; | |
| img.src = objectURL; | |
| }); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/loader/src/Texture2DLoader.ts` around lines 19 - 29,
loadImageFromBuffer leaks the created object URL on image decode failures
because URL.revokeObjectURL is only called in img.onload; modify
loadImageFromBuffer so that the blob URL created by URL.createObjectURL(blob) is
stored in a local variable and revoked both in img.onload and in img.onerror
(and ensure img.onerror calls reject after revoking). Update references to
img.src to use that local url variable when calling revoke in both handlers to
guarantee the URL is always released.
| if (FileHeader.checkMagic(buffer)) { | ||
| decode<Texture2D>(buffer, resourceManager.engine).then((texture) => { | ||
| resourceManager.addContentRestorer(new Texture2DContentRestorer(texture, url, requestConfig)); | ||
| resolve(texture); | ||
| }, reject); | ||
| } else { | ||
| loadImageFromBuffer(buffer).then((img) => { |
There was a problem hiding this comment.
Keep the no-magic serialized-texture path reachable.
packages/loader/src/resource-deserialize/utils/FileHeader.ts now decodes headers with or without MAGIC, but these branches only call decode() when checkMagic() is true. That makes the no-magic path unreachable here, so serialized texture buffers without the prefix fall through to loadImageFromBuffer() instead of the binary decoder.
💡 One straightforward fix
- if (FileHeader.checkMagic(buffer)) {
+ const isSerializedTexture = FileHeader.checkMagic(buffer) || /\.tex(?:$|[?#])/i.test(url);
+ if (isSerializedTexture) {- if (FileHeader.checkMagic(buffer)) {
+ const isSerializedTexture = FileHeader.checkMagic(buffer) || /\.tex(?:$|[?#])/i.test(this.url);
+ if (isSerializedTexture) {Also applies to: 111-114
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/loader/src/Texture2DLoader.ts` around lines 43 - 49, The current
branch only calls decode<Texture2D> when FileHeader.checkMagic(buffer) is true,
which prevents serialized-but-no-MAGIC textures from being decoded; change the
condition to ask FileHeader's new header-detection/parsing API (the function you
added that recognizes headers with or without MAGIC) and if that parser reports
the buffer is a serialized texture, call decode<Texture2D>(buffer,
resourceManager.engine). After decoding, continue to add the restorer via
resourceManager.addContentRestorer(new Texture2DContentRestorer(...)) and
resolve, otherwise fall back to loadImageFromBuffer(buffer) as before.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
2 participants
Summary
This PR contains three related refactors to simplify and improve the asset loading pipeline:
1. Replace refId with url for asset reference resolution
url(virtualPath) instead of opaquerefId(UUID) for asset references, making them readable and debuggablegetResourceByRefto resolve directly via_virtualPathResourceMap, removing the redundant_idResourceMaplayerIAssetRef,IRefEntity,IComponent,IGalaceanMaterialRemap) fromrefIdtourl2. Unify Texture2DLoader and EditorTextureLoader
EditorTextureLoaderintoTexture2DLoader— one loader handles both raw images (png/jpg/webp) and Galacean binary textures (.tex)FileHeader.MAGIC(0x4E434C47= "GLCN") for binary format detection, following industry conventions (GLB usesglTF, PNG uses0x89504E47)arraybuffertype, thencheckMagic()branches to binary decode or image decode pathContentRestorerlogic using the same magic-based detection3. Standardize resource file extensions
png,jpg,webp,jpeg,textextexCube""→texCubematjson→matanimani→animanimCtrljson→animCtrlmeshphysMatmesh→physMatsceneprefabprojectproj→projectfontspriteatlasenvgs,gslgltf,glbktx,ktx2hdrttf,otf,woffmp3,ogg,wavglsljson,txt,binr3binDeleted files
EditorTextureLoader.ts— merged intoTexture2DLoaderTexture2DContentRestorer.ts— inlined intoTexture2DLoaderBreaking Changes (Editor Side)
0x4E434C47) and use.texextensiontype: "EditorTexture2D"→type: AssetType.Texture2D.json→.mat.ani→.anim.json→.animCtrl.mesh→.physMat.texCube.proj→.projectTest Plan
DeviceLost.test.ts,ResourceManager.test.ts,SceneParser.test.ts)