Add loader for Shader and ShaderChunk asset
#2027
Conversation
Sway007
added
enhancement
| const { type, value } = shaderData[key]; | ||
| // @ts-ignore | ||
| resourceManager.getResourceByRef<Shader>(<IAssetRef>shaderRef).then((shader) => { | ||
| const material = new Material(engine, shader); |
There was a problem hiding this comment.
force use shader?pls compatible with common materials and previous materials
|
Warning Rate limit exceeded@Sway007 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 14 minutes and 40 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe pull request introduces several changes across multiple files, primarily adding a new asset type Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2027 +/- ##
==========================================
- Coverage 69.50% 69.38% -0.13%
==========================================
Files 522 523 +1
Lines 27365 27404 +39
Branches 4097 4108 +11
==========================================
- Hits 19021 19014 -7
- Misses 6846 6882 +36
- Partials 1498 1508 +10
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
packages/loader/src/resource-deserialize/resources/schema/BasicSchema.ts
Outdated
Show resolved
Hide resolved
| const shaderChunkPaths: string[] = []; | ||
| const matches = code.matchAll(Utils.shaderIncludeRegex); | ||
| for (const match of matches) { | ||
| const chunkPath = Utils.pathResolve(match[1], basePath); |
packages/loader/src/resource-deserialize/resources/schema/BasicSchema.ts
Outdated
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
🧹 Outside diff range and nitpick comments (38)
packages/loader/src/JSONLoader.ts (1)
5-9: LGTM: Consistent resource management implementationThe changes align with the broader initiative to standardize resource management across loaders. The implementation:
- Correctly passes the resourceManager to the request method
- Maintains existing functionality while adding resource management capability
- Preserves type safety with proper parameter typing
This change is part of a good architectural pattern that centralizes resource management across different loader types, which will help with:
- Consistent resource handling
- Better resource lifecycle management
- Simplified dependency resolution
packages/loader/src/Utils.ts (2)
4-4: Enhance shader include regex pattern robustness.The current regex pattern might miss some valid include directives or allow malformed ones. Consider these edge cases:
- Multiple spaces between parts
- Comments before the include
- Different quote types
Consider using a more robust pattern:
- static shaderIncludeRegex = /\s#include\s+"([^\\"]+)"/gm; + static shaderIncludeRegex = /^\s*#include\s+["']([^"'\\]+)["']/gm;
1-13: Add JSDoc documentation for public API.Since this utility is used for shader loading, proper documentation would help maintainers understand the expected formats and usage.
Consider adding documentation:
/** @internal Utilities for shader asset loading and path resolution */ export class Utils { /** Regex pattern for matching shader include directives */ static shaderIncludeRegex = /\s#include\s+"([^\\"]+)"/gm; /** * Resolves a relative or absolute path against a base URL * @param path - The path to resolve (e.g., "../shader.glsl") * @param base - The base URL to resolve against * @returns Resolved absolute path without the schema prefix * @throws {Error} If the path or base is invalid */ static pathResolve(path: string, base: string): string { // ... implementation } /** * Checks if a path is relative * @param path - The path to check * @returns true if the path is relative, false otherwise */ static isRelativePath(path: string): boolean { // ... implementation } }🧰 Tools
🪛 Biome
[error] 2-13: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
packages/xr/src/loader/XRReferenceImageLoader.ts (1)
Line range hint
8-13: Consider enhancing error handling with specific error types.The current error handling could be more specific to help with debugging. Consider catching and transforming specific error types before rejection.
this.request<ArrayBuffer>(item.url, resourceManager, { ...item, type: "arraybuffer" }) .then((data) => { decode<XRReferenceImage>(data, resourceManager.engine).then((referenceImage) => { resolve(referenceImage); - }); + }).catch(error => { + reject(new Error(`Failed to decode XRReferenceImage: ${error.message}`)); + }); }) - .catch(reject); + .catch(error => { + reject(new Error(`Failed to load XRReferenceImage from ${item.url}: ${error.message}`)); + });packages/loader/src/BufferLoader.ts (1)
17-20: Consider adding parameter documentationThe request method call correctly includes the resourceManager parameter while maintaining existing functionality.
Consider adding JSDoc documentation for the resourceManager parameter to improve code maintainability:
+/** + * Loads a buffer asset from the given URL + * @param item - The load item containing the URL and other options + * @param resourceManager - The resource manager instance for handling resource requests + * @returns A promise that resolves with the loaded array buffer + */ load(item: LoadItem, resourceManager: ResourceManager): AssetPromise<ArrayBuffer> {packages/loader/src/PrefabLoader.ts (1)
12-15: Consider enhancing error handling for request failuresWhile the current implementation handles errors through the promise chain, it might be beneficial to add specific error handling for the request phase before parsing.
Consider this enhancement:
this.request<IHierarchyFile>(item.url, resourceManager, { ...item, type: "json" - }).then((data) => { + }).then((data) => { + if (!data) { + throw new Error(`Failed to load prefab from ${item.url}`); + } PrefabParser.parse(engine, item.url, data).then(resolve).catch(reject); + }).catch((error) => { + reject(new Error(`Failed to load or parse prefab: ${error.message}`)); });packages/loader/src/ProjectLoader.ts (2)
Line range hint
13-25: Consider enhancing error handling for shader-related failures.While the basic error handling is in place, the promise chain could benefit from more specific error handling, especially for shader-related failures during virtual resource initialization.
Consider adding more granular error handling:
load(item: LoadItem, resourceManager: ResourceManager): AssetPromise<void> { const { engine } = resourceManager; return new AssetPromise((resolve, reject) => { this.request<IProject>(item.url, resourceManager, { ...item, type: "json" }) .then((data) => { try { // @ts-ignore engine.resourceManager.initVirtualResources(data.files); return resourceManager.load<Scene>({ type: AssetType.Scene, url: data.scene }).then((scene) => { engine.sceneManager.activeScene = scene; resolve(); }); } catch (error) { reject(new Error(`Failed to initialize resources: ${error.message}`)); } }) - .catch(reject); + .catch((error) => { + reject(new Error(`Failed to load project: ${error.message}`)); + }); }); }
Line range hint
1-25: Consider documenting the virtual resource initialization process.Given that this loader is crucial for shader asset loading via HTTP URLs (as mentioned in PR objectives), it would be beneficial to document how virtual resources are initialized and how they relate to shader loading.
Add JSDoc comments to explain the process:
+/** + * Loader for project files that handles initialization of virtual resources, + * including shader assets that can be loaded via HTTP URLs. + * @remarks + * The loader performs the following steps: + * 1. Loads the project JSON + * 2. Initializes virtual resources (including shaders) + * 3. Loads and activates the main scene + */ @resourceLoader(AssetType.Project, ["proj"], false) class ProjectLoader extends Loader<void> {packages/loader/src/SourceFontLoader.ts (1)
Line range hint
1-31: Consider adding error type information.The error handling in the catch block could be improved by including the original error information.
- .catch((e) => { - reject(`load font ${url} fail`); + .catch((e: Error) => { + reject(`Failed to load font ${url}: ${e.message}`);packages/loader/src/ShaderLoader.ts (1)
14-14: Add documentation for the_builtinRegexpattern.Add a JSDoc comment explaining the purpose and format of the builtin shader directive that this regex matches. This will help answer questions about its usage and make the code more maintainable.
Example documentation:
/** Matches shader directives in the format: // @builtin shaderName */ private static _builtinRegex = /^\s*\/\/\s*@builtin\s+(\w+)/;packages/loader/src/FontLoader.ts (1)
Line range hint
15-32: Consider enhancing error handling and font loading verification.While the current implementation works, there are opportunities for improvement:
- The font loading error message could be more descriptive by including the actual error details
- Consider adding font availability verification after registration
Here's a suggested improvement:
this.request<any>(item.url, resourceManager, { ...item, type: "json" }) .then((data) => { const { fontName, fontUrl } = data; if (fontUrl) { this._registerFont(fontName, fontUrl) .then(() => { + // Verify font is actually available + if (!document.fonts.check(`12px ${fontName}`)) { + throw new Error(`Font ${fontName} registration succeeded but font is not available`); + } const font = new Font(resourceManager.engine, fontName); resolve(font); }) .catch((e) => { - reject(`load font ${fontUrl} fail`); + reject(`Failed to load font from ${fontUrl}: ${e.message}`); }); } else { const font = new Font(resourceManager.engine, fontName); resolve(font); } }) .catch((e) => { - reject(e); + reject(`Failed to fetch font data: ${e.message}`); });packages/loader/src/gltf/parser/GLTFBufferParser.ts (1)
23-23: Add validation and fallback for URL resolution.While the URL resolution change supports virtual path mapping, it lacks proper validation and fallback mechanisms.
Consider adding safety checks:
- const absoluteUrl = Utils.resolveAbsoluteUrl(remoteUrl, bufferInfo.uri); + const baseUrl = remoteUrl || url; // Fallback to original url if remoteUrl is undefined + if (!baseUrl) { + throw new Error('Unable to resolve base URL for buffer'); + } + const absoluteUrl = Utils.resolveAbsoluteUrl(baseUrl, bufferInfo.uri);Also, consider adding JSDoc comments to document the expected URL format and the virtual path mapping behavior.
packages/loader/src/gltf/parser/GLTFSchemaParser.ts (1)
16-21: Document the virtual path mapping behavior.The introduction of URL remapping is a significant change that affects resource loading behavior, but lacks documentation explaining its purpose and usage.
Add JSDoc comments explaining:
- The purpose of virtual path mapping
- When/why URLs get remapped
- Expected format of virtual paths
- Error handling behavior
+ /** + * Resolves and loads the GLTF schema using virtual path mapping. + * Virtual paths allow remapping of resource URLs for flexibility in resource loading. + * @throws {Error} If the resolved URL is invalid or resource cannot be loaded + */ parse(context: GLTFParserContext): Promise<IGLTF> {packages/loader/src/TextureCubeLoader.ts (1)
Line range hint
27-31: Improve error handling for non-square textures.Currently, the code only logs an error and returns undefined when encountering non-square textures. This could lead to silent failures and confusion downstream.
Consider throwing an error instead:
if (width !== height) { - console.error("The cube texture must have the same width and height"); - return; + throw new Error(`Invalid cube texture dimensions: width (${width}) must equal height (${height})`); }packages/loader/src/AnimationClipLoader.ts (2)
Line range hint
12-35: Consider refactoring the load method for better maintainability.The method could be simplified by extracting the curve binding processing logic into a separate method and flattening the promise chain.
Here's a suggested refactor:
load(item: LoadItem, resourceManager: ResourceManager): AssetPromise<AnimationClip> { return new AssetPromise((resolve, reject) => { this.request<any>(item.url, resourceManager, { ...item, type: "arraybuffer" }) - .then((data) => { - return decode<AnimationClip>(data, resourceManager.engine) - .then((clip) => { - const curveBindingPromises = clip.curveBindings.map((curveBinding) => { - const { curve } = curveBinding; - const promises = curve.keys.map((key) => { - return this._parseKeyframeValue(key, resourceManager).then((actualValue) => { - key.value = actualValue; - }); - }); - return Promise.all(promises); - }); - return Promise.all(curveBindingPromises).then(() => { - resolve(clip); - }); - }) - .catch(reject); - }) - .catch(reject); + .then(async (data) => { + try { + const clip = await decode<AnimationClip>(data, resourceManager.engine); + await this._processCurveBindings(clip, resourceManager); + resolve(clip); + } catch (error) { + reject(error); + } + }) + .catch(reject); }); } +private async _processCurveBindings(clip: AnimationClip, resourceManager: ResourceManager): Promise<void> { + const curveBindingPromises = clip.curveBindings.map(async (curveBinding) => { + const { curve } = curveBinding; + await Promise.all( + curve.keys.map(async (key) => { + key.value = await this._parseKeyframeValue(key, resourceManager); + }) + ); + }); + await Promise.all(curveBindingPromises); +}
Line range hint
37-51: Consider improving type safety in _parseKeyframeValue.The method uses type assertions and
@ts-ignore, which could be replaced with proper type definitions.Consider defining an interface for the reference type:
interface ResourceReference { refId: string; } private _parseKeyframeValue(keyframe: { value: unknown }, resourceManager: ResourceManager): Promise<any> { const value = keyframe.value; if (typeof value === "object" && value && this._isResourceReference(value)) { return resourceManager .getResourceByRef<ReferResource>(value) .then((asset: ReferResource) => { keyframe.value = asset; return keyframe.value; }); } return Promise.resolve(keyframe.value); } private _isResourceReference(value: unknown): value is ResourceReference { return (value as ResourceReference)?.refId !== undefined; }packages/loader/src/SpriteLoader.ts (3)
Line range hint
15-22: Consider enhancing error handlingThe current implementation could benefit from more robust error handling:
- Add validation for the response data structure
- Handle network request failures explicitly
- Add error logging for debugging purposes
Example improvement:
load(item: LoadItem, resourceManager: ResourceManager): AssetPromise<Sprite> { return this.request<any>(item.url, resourceManager, { ...item, type: "json" - }).then((data) => { + }).then((data) => { + if (!data || typeof data !== 'object') { + throw new Error('Invalid sprite data format'); + } return data.belongToAtlas ? this._loadFromAtlas(resourceManager, data) : this._loadFromTexture(resourceManager, data); + }).catch(error => { + console.error(`Failed to load sprite from ${item.url}:`, error); + throw error; }); }
13-14: Add JSDoc documentation and improve type definitionsConsider adding JSDoc documentation and replacing
anytypes with more specific interfaces:Example improvement:
/** Data structure for sprite configuration */ interface SpriteData { belongToAtlas?: string; fullPath?: string; texture?: string; region?: any; // Define specific type pivot?: any; // Define specific type border?: any; // Define specific type width?: number; height?: number; } /** * Loads sprite assets from various sources * @param item The load item containing sprite configuration * @param resourceManager The resource manager instance * @returns Promise resolving to a Sprite instance */ load(item: LoadItem, resourceManager: ResourceManager): AssetPromise<Sprite> /** * Loads a sprite from an atlas * @param resourceManager The resource manager instance * @param data The sprite configuration data * @returns Promise resolving to a Sprite instance */ private _loadFromAtlas(resourceManager: ResourceManager, data: SpriteData): AssetPromise<Sprite>Also applies to: 24-25, 35-36
Based on the gathered information, I can now provide a final response:
The @ts-ignore comments can be safely removed by using proper type annotations
The
getResourceByRefmethod is properly typed in the ResourceManager class and accepts anIAssetReftype parameter which is defined as:type IAssetRef = { key?: string; refId: string };To fix the type issues:
- Line 13-19: Replace
data.belongToAtlaswith properIAssetReftype- Line 39-40: Replace
data.texturewith properIAssetReftypeThe pattern is consistently used across the codebase for loading resources like textures, shaders, and sprites. The type safety can be improved by ensuring the data properties conform to the
IAssetRefinterface rather than suppressing type checks.🔗 Analysis chain
Line range hint
29-30: Consider removing @ts-ignore by adding proper type definitionsThe presence of
@ts-ignorecomments suggests potential type safety issues. Consider adding proper type definitions forgetResourceByRefmethod calls instead of suppressing type checks.Let's check if there are similar patterns in other loader files:
Also applies to: 39-40
Let me analyze the
getResourceByRefusage to understand the type safety concerns better:
Let me search for the IAssetRef type definition with a different pattern:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for @ts-ignore usage in loader files rg -A 2 "@ts-ignore" "packages/loader/src/"Length of output: 14789
Script:
#!/bin/bash # Search for getResourceByRef method definition and usage ast-grep --pattern 'getResourceByRef<$_>($_)'Length of output: 47
Script:
#!/bin/bash # Search for ResourceManager class definition ast-grep --pattern 'class ResourceManager { $$$ }'Length of output: 42953
Script:
#!/bin/bash # Search for IAssetRef interface definition ast-grep --pattern 'interface IAssetRef { $$$ }'Length of output: 52
Script:
#!/bin/bash # Search for IAssetRef type/interface definition rg "IAssetRef" -A 5 -B 1Length of output: 6077
Script:
#!/bin/bash # Search for getResourceByRef usage with belongToAtlas and texture rg "getResourceByRef.*\.(belongToAtlas|texture)" -A 2Length of output: 959
packages/loader/src/EnvLoader.ts (3)
Line range hint
20-48: Add validation for binary data structure.The current implementation assumes the arraybuffer contains valid data but doesn't verify the buffer size matches the expected data layout. This could lead to undefined behavior or crashes with malformed data.
Consider adding validation:
this.request<ArrayBuffer>(item.url, resourceManager, { ...item, type: "arraybuffer" }) .then((arraybuffer) => { + const SH_FLOAT_COUNT = 27; + const SH_BYTE_LENGTH = SH_FLOAT_COUNT * 4; + const SIZE_BYTE_LENGTH = 2; + + if (arraybuffer.byteLength < SH_BYTE_LENGTH + SIZE_BYTE_LENGTH) { + throw new Error("Invalid environment map data: buffer too small"); + } + - const shArray = new Float32Array(arraybuffer, 0, 27); - const shByteLength = 27 * 4; - const size = new Uint16Array(arraybuffer, shByteLength, 1)?.[0]; + const shArray = new Float32Array(arraybuffer, 0, SH_FLOAT_COUNT); + const size = new Uint16Array(arraybuffer, SH_BYTE_LENGTH, 1)[0]; + + // Calculate expected buffer size including all mip levels + let expectedSize = SH_BYTE_LENGTH + SIZE_BYTE_LENGTH; + let mipSize = size; + while (mipSize >= 1) { + expectedSize += 6 * (mipSize * mipSize * 4); // 6 faces * pixels * 4 bytes + mipSize >>= 1; + } + + if (arraybuffer.byteLength < expectedSize) { + throw new Error("Invalid environment map data: incomplete texture data"); + }
Line range hint
26-39: Consider extracting texture loading logic.The nested loops for loading mip levels and faces could be more readable if extracted into a separate method.
Consider refactoring:
+ private loadCubemapFaces(texture: TextureCube, arraybuffer: ArrayBuffer, offset: number, size: number): number { + const mipmapCount = texture.mipmapCount; + + for (let mipLevel = 0; mipLevel < mipmapCount; mipLevel++) { + const mipSize = size >> mipLevel; + + for (let face = 0; face < 6; face++) { + const dataSize = mipSize * mipSize * 4; + const data = new Uint8Array(arraybuffer, offset, dataSize); + offset += dataSize; + texture.setPixelBuffer(TextureCubeFace.PositiveX + face, data, mipLevel); + } + } + + return offset; + }
Line range hint
41-46: Consider extracting ambient light setup.The ambient light configuration could be more readable if extracted into a separate method.
Consider refactoring:
+ private setupAmbientLight(engine: Engine, sh: Float32Array, texture: TextureCube): AmbientLight { + const ambientLight = new AmbientLight(engine); + const sphericalHarmonics = new SphericalHarmonics3(); + + ambientLight.diffuseMode = DiffuseMode.SphericalHarmonics; + sphericalHarmonics.copyFromArray(sh); + ambientLight.diffuseSphericalHarmonics = sphericalHarmonics; + ambientLight.specularTexture = texture; + ambientLight.specularTextureDecodeRGBM = true; + + return ambientLight; + }packages/loader/src/GLTFLoader.ts (1)
41-41: LGTM! Consider adding type assertion for URL.The simplified instantiation looks good. For additional type safety, you might want to consider adding a type assertion for the URL parameter.
- const glTFResource = new GLTFResource(resourceManager.engine, item.url); + const glTFResource = new GLTFResource(resourceManager.engine, item.url as string);packages/loader/src/Texture2DLoader.ts (1)
Line range hint
11-49: Consider breaking down the load method for better maintainability.The
loadmethod handles multiple responsibilities including request configuration, texture creation, parameter setting, and content restoration. Consider extracting some of these operations into separate private methods.Here's a suggested refactoring:
override load(item: LoadItem, resourceManager: ResourceManager): AssetPromise<Texture2D> { return new AssetPromise((resolve, reject, setTaskCompleteProgress, setTaskDetailProgress) => { - const url = item.url; - const requestConfig = <RequestConfig>{ - ...item, - type: "image" - }; + const { url, requestConfig } = this.prepareRequest(item); this.request<HTMLImageElement>(url, resourceManager, requestConfig) .onProgress(setTaskCompleteProgress, setTaskDetailProgress) - .then((image) => { - const { format, mipmap, anisoLevel, wrapModeU, wrapModeV, filterMode } = - (item.params as Partial<Texture2DParams>) ?? {}; - - const texture = new Texture2D(resourceManager.engine, image.width, image.height, format, mipmap); - - texture.anisoLevel = anisoLevel ?? texture.anisoLevel; - texture.filterMode = filterMode ?? texture.filterMode; - texture.wrapModeU = wrapModeU ?? texture.wrapModeU; - texture.wrapModeV = wrapModeV ?? texture.wrapModeV; - - texture.setImageSource(image); - texture.generateMipmaps(); - - if (url.indexOf("data:") !== 0) { - const index = url.lastIndexOf("/"); - texture.name = url.substring(index + 1); - } - - resourceManager.addContentRestorer(new Texture2DContentRestorer(texture, url, requestConfig)); + .then((image) => { + const texture = this.createAndConfigureTexture(image, item.params, resourceManager); + this.setTextureName(texture, url); + this.setupContentRestorer(texture, url, requestConfig, resourceManager); resolve(texture); }) .catch((e) => { reject(e); }); }); } +private prepareRequest(item: LoadItem): { url: string; requestConfig: RequestConfig } { + return { + url: item.url, + requestConfig: <RequestConfig>{ + ...item, + type: "image" + } + }; +} + +private createAndConfigureTexture( + image: HTMLImageElement, + params: Partial<Texture2DParams>, + resourceManager: ResourceManager +): Texture2D { + const { format, mipmap, anisoLevel, wrapModeU, wrapModeV, filterMode } = params ?? {}; + const texture = new Texture2D(resourceManager.engine, image.width, image.height, format, mipmap); + + texture.anisoLevel = anisoLevel ?? texture.anisoLevel; + texture.filterMode = filterMode ?? texture.filterMode; + texture.wrapModeU = wrapModeU ?? texture.wrapModeU; + texture.wrapModeV = wrapModeV ?? texture.wrapModeV; + + texture.setImageSource(image); + texture.generateMipmaps(); + + return texture; +} + +private setTextureName(texture: Texture2D, url: string): void { + if (url.indexOf("data:") !== 0) { + const index = url.lastIndexOf("/"); + texture.name = url.substring(index + 1); + } +} + +private setupContentRestorer( + texture: Texture2D, + url: string, + requestConfig: RequestConfig, + resourceManager: ResourceManager +): void { + resourceManager.addContentRestorer(new Texture2DContentRestorer(texture, url, requestConfig)); +}packages/loader/src/PrimitiveMeshLoader.ts (1)
15-15: Consider adding error handling for engine extractionThe engine extraction from resourceManager should include validation to prevent potential runtime errors.
Consider adding error handling:
- const { engine } = resourceManager; + const { engine } = resourceManager; + if (!engine) { + throw new Error("Engine instance not found in ResourceManager"); + }packages/loader/src/resource-deserialize/resources/schema/MaterialSchema.ts (1)
102-110: Add JSDoc documentation to the MaterialLoaderType enum.Consider adding JSDoc comments to improve IDE tooltips and documentation:
+/** Defines the supported types for material shader data properties. */ export enum MaterialLoaderType { + /** 2D vector type (x, y) */ Vector2 = "Vector2", + /** 3D vector type (x, y, z) */ Vector3 = "Vector3", + /** 4D vector type (x, y, z, w) */ Vector4 = "Vector4", + /** Color type (r, g, b, a) */ Color = "Color", + /** Single precision floating point type */ Float = "Float", + /** Texture reference type */ Texture = "Texture", + /** Boolean type */ Boolean = "Boolean", + /** Integer type */ Integer = "Integer" }packages/loader/src/SpriteAtlasLoader.ts (3)
Line range hint
34-69: Consider refactoring the nested promise chain for better maintainability.While the promise chain works correctly, it could be simplified to improve readability and reduce duplicate error handling.
Consider restructuring like this:
- configPromise - .then((atlasData) => { - // ... existing logic ... - AssetPromise.all(chainPromises) - .then(() => { - resolve(spriteAtlas); - }) - .catch(reject); - }) - .catch(reject); + async function loadAtlas(atlasData: AtlasConfig) { + const { atlasItems, mipmap, anisoLevel, filterMode, wrapModeU, wrapModeV, format } = atlasData; + const atlasItemsLen = atlasItems ? atlasItems.length : 0; + const { engine } = resourceManager; + const spriteAtlas = new SpriteAtlas(engine); + + if (atlasItemsLen <= 0) { + return spriteAtlas; + } + + const texturePromises = atlasItems.flatMap(atlasItem => + createTexturePromises(atlasItem, item, spriteAtlas)); + + await Promise.all(texturePromises); + return spriteAtlas; + } + + configPromise + .then(loadAtlas) + .then(resolve) + .catch(reject);
Line range hint
44-54: Enhance type safety and error handling for texture loading.The texture loading logic could benefit from improved type safety and more specific error handling.
Consider these improvements:
resourceManager .load<Texture2D>({ url: Utils.resolveAbsoluteUrl(item.url, atlasItem.img), type: atlasItem.type ?? AssetType.Texture2D, params: { format, mipmap } }) - .then((texture: Texture2D) => { + .then((texture) => { + if (!(texture instanceof Texture2D)) { + throw new Error(`Expected Texture2D but got ${texture?.constructor.name}`); + } anisoLevel && (texture.anisoLevel = anisoLevel); filterMode !== undefined && (texture.filterMode = filterMode); // ... rest of the texture configuration }) - .catch(reject) + .catch((error) => { + reject(new Error(`Failed to load texture ${atlasItem.img}: ${error.message}`)); + })
Line range hint
16-18: Fix potential race condition with shared temporary objects.The shared temporary objects (
_tempRect,_tempVec2,_tempVec4) could cause issues if multiple sprite atlas loads occur simultaneously, as they might be modified concurrently.Consider creating new instances for each sprite creation:
- private _tempRect: Rect = new Rect(); - private _tempVec2: Vector2 = new Vector2(); - private _tempVec4: Vector4 = new Vector4(); private _makeSprite(engine: Engine, config: AtlasSprite, texture?: Texture2D): Sprite { + const tempRect = new Rect(); + const tempVec2 = new Vector2(); + const tempVec4 = new Vector4(); // ... use tempRect, tempVec2, tempVec4 instead of this._tempRect, etc.packages/loader/src/MaterialLoader.ts (2)
74-113: Add type exhaustiveness checking for MaterialLoaderType.The switch statement should ensure all possible MaterialLoaderType values are handled.
switch (type) { // ... existing cases ... + default: { + const _exhaustiveCheck: never = type; + throw new Error(`Unhandled material property type: ${_exhaustiveCheck}`); + } }
116-125: Consider adding type safety to macro and render state handling.The current implementation could benefit from stronger typing:
- Add type definitions for macro objects
- Use a more specific type than 'Object' in parseProperty
+interface IMaterialMacro { + name: string; + value?: number | boolean | string; +} +interface IRenderState { + // Add specific render state properties +} -function parseProperty(object: Object, key: string, value: any) { +function parseProperty(object: Record<string, unknown>, key: string, value: unknown) {packages/loader/src/SceneLoader.ts (3)
24-24: LGTM! Consider adding type safety for the request options.The integration of
resourceManagerinto the request method is well implemented and aligns with the PR's objective of enhancing resource management.Consider extracting the request options type for better type safety:
type SceneRequestOptions = { type: "json"; [key: string]: any; }; this.request<IScene, SceneRequestOptions>(item.url, resourceManager, { ...item, type: "json" })
Line range hint
41-46: Remove @ts-ignore comments by properly typing resource references.There are multiple instances where @ts-ignore is used before
resourceManager.getResourceByRefcalls. This bypasses TypeScript's type checking and could lead to runtime errors.Consider creating proper type definitions for resource references:
interface IResourceRef<T> { id: string; type: AssetType; } // Then use it in getResourceByRef resourceManager.getResourceByRef<Material>(background.skyMaterial as IResourceRef<Material>)This would provide better type safety and eliminate the need for @ts-ignore comments.
Also applies to: 65-71, 82-86, 142-146
Line range hint
26-163: Enhance error handling and resource management.While the main promise chain includes error handling, individual resource loading operations could benefit from more robust error handling and cleanup logic.
Consider these improvements:
- Add specific error handling for resource loading:
const loadResource = async <T>(ref: any, onLoad: (resource: T) => void) => { try { const resource = await resourceManager.getResourceByRef<T>(ref); onLoad(resource); } catch (error) { Logger.warn(`Failed to load resource: ${error.message}`); // Optionally rethrow if this is a critical resource // throw error; } }; // Usage promises.push(loadResource(background.skyMaterial, material => { scene.background.sky.material = material; }));
- Add cleanup logic in case of failures:
const cleanup = () => { // Release any partially loaded resources promises.forEach(promise => { promise.catch(() => { // Cleanup logic }); }); }; // Add to the catch block .catch(error => { cleanup(); reject(error); });packages/loader/src/ktx2/KTX2Loader.ts (4)
Line range hint
249-251: Add migration guidance for deprecated propertiesThe
priorityFormatsproperty is marked as deprecated in bothKTX2ParamsandEngineConfigurationinterfaces, but no migration path or alternative is provided. Please add documentation to help users migrate away from these deprecated properties.Consider adding a comment like this:
/** Priority transcoding format queue which is preferred options, default is BC7/ASTC/BC3_BC1/ETC/PVRTC/R8G8B8A8. */ /** @deprecated */ + /** @deprecated Use X instead. See migration guide at <URL> */ priorityFormats: KTX2TargetFormat[];Also applies to: 266-268
Line range hint
89-91: Enhance error message for PVRTC downgradeThe warning message for PVRTC downgrade could be more descriptive by including the actual dimensions.
Consider enhancing the message:
- Logger.warn("PVRTC image need power of 2 and width===height, downgrade to RGBA8"); + Logger.warn(`PVRTC requires power of 2 dimensions and square aspect ratio. Got ${ktx2Container.pixelWidth}x${ktx2Container.pixelHeight}, downgrading to RGBA8`);
Line range hint
116-143: Consider refactoring format detection for better maintainabilityThe format detection logic in
_detectSupportedFormatuses nested switches and conditions. Consider using a more declarative approach with a format capability map.Example refactor:
private static readonly _formatCapabilityMap = { [KTX2TargetFormat.ASTC]: { capabilities: [GLCapabilityType.astc], check: () => true }, [KTX2TargetFormat.R8G8B8A8]: { capabilities: [], check: () => true }, [KTX2TargetFormat.R8]: { capabilities: [], check: (renderer) => renderer.isWebGL2 } // ... other formats }; private static _detectSupportedFormat(renderer: any, priorityFormats: KTX2TargetFormat[]): KTX2TargetFormat | null { return priorityFormats.find(format => { const config = this._formatCapabilityMap[format]; if (!config) return false; return config.capabilities.some(cap => renderer.canIUse(cap)) && config.check(renderer); }) ?? null; }
Line range hint
106-106: DRY: Extract default worker count to a constantThe default worker count of 4 is duplicated in
_getBinomialLLCTranscoderand_getKhronosTranscoder. Consider extracting it to a constant.+private static readonly DEFAULT_WORKER_COUNT = 4; -private static _getBinomialLLCTranscoder(workerCount: number = 4) { +private static _getBinomialLLCTranscoder(workerCount: number = this.DEFAULT_WORKER_COUNT) { -private static _getKhronosTranscoder(workerCount: number = 4) { +private static _getKhronosTranscoder(workerCount: number = this.DEFAULT_WORKER_COUNT) {Also applies to: 111-111
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (36)
packages/core/src/asset/Loader.ts(2 hunks)packages/core/src/asset/ResourceManager.ts(2 hunks)packages/loader/src/AnimationClipLoader.ts(1 hunks)packages/loader/src/AnimatorControllerLoader.ts(1 hunks)packages/loader/src/BufferLoader.ts(2 hunks)packages/loader/src/EnvLoader.ts(1 hunks)packages/loader/src/FontLoader.ts(1 hunks)packages/loader/src/GLTFLoader.ts(1 hunks)packages/loader/src/HDRLoader.ts(1 hunks)packages/loader/src/JSONLoader.ts(1 hunks)packages/loader/src/KTXCubeLoader.ts(1 hunks)packages/loader/src/KTXLoader.ts(1 hunks)packages/loader/src/MaterialLoader.ts(3 hunks)packages/loader/src/MeshLoader.ts(0 hunks)packages/loader/src/PrefabLoader.ts(1 hunks)packages/loader/src/PrimitiveMeshLoader.ts(1 hunks)packages/loader/src/ProjectLoader.ts(1 hunks)packages/loader/src/SceneLoader.ts(1 hunks)packages/loader/src/ShaderChunkLoader.ts(1 hunks)packages/loader/src/ShaderLoader.ts(1 hunks)packages/loader/src/SourceFontLoader.ts(1 hunks)packages/loader/src/SpriteAtlasLoader.ts(1 hunks)packages/loader/src/SpriteLoader.ts(1 hunks)packages/loader/src/TextLoader.ts(1 hunks)packages/loader/src/Texture2DLoader.ts(1 hunks)packages/loader/src/TextureCubeLoader.ts(1 hunks)packages/loader/src/Utils.ts(1 hunks)packages/loader/src/gltf/parser/GLTFBufferParser.ts(1 hunks)packages/loader/src/gltf/parser/GLTFSchemaParser.ts(1 hunks)packages/loader/src/index.ts(1 hunks)packages/loader/src/ktx2/KTX2Loader.ts(1 hunks)packages/loader/src/resource-deserialize/index.ts(0 hunks)packages/loader/src/resource-deserialize/resources/scene/EditorTextureLoader.ts(1 hunks)packages/loader/src/resource-deserialize/resources/scene/MeshLoader.ts(0 hunks)packages/loader/src/resource-deserialize/resources/schema/MaterialSchema.ts(1 hunks)packages/xr/src/loader/XRReferenceImageLoader.ts(1 hunks)
💤 Files with no reviewable changes (3)
- packages/loader/src/MeshLoader.ts
- packages/loader/src/resource-deserialize/index.ts
- packages/loader/src/resource-deserialize/resources/scene/MeshLoader.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/core/src/asset/ResourceManager.ts
- packages/loader/src/index.ts
🧰 Additional context used
🪛 Biome
packages/loader/src/ShaderChunkLoader.ts
[error] 14-14: void is confusing outside a return type or a type parameter.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
[error] 15-15: void is confusing outside a return type or a type parameter.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
[error] 27-27: void is confusing outside a return type or a type parameter.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
packages/loader/src/ShaderLoader.ts
[error] 33-33: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/loader/src/Utils.ts
[error] 2-13: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
🔇 Additional comments (33)
packages/loader/src/TextLoader.ts (2)
1-1: LGTM: Clean import addition
The addition of ResourceManager import is necessary and consistent with the broader changes across loader classes.
5-9: LGTM: Well-structured resource management integration
The changes properly integrate resource management while maintaining existing functionality:
- Type-safe resourceManager parameter addition
- Correct parameter passing to request method
- Preserved existing options handling
Let's verify this pattern is consistent across other loaders:
✅ Verification successful
LGTM: Resource management pattern is consistent across all loaders
The verification confirms that all loader implementations follow the same pattern for resource management:
- All loaders consistently implement the
load(item: LoadItem, resourceManager: ResourceManager): AssetPromise<T>method signature - The
resourceManagerparameter is properly used across implementations - The pattern of passing the resourceManager to the
requestmethod or using it directly is consistent
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent resourceManager parameter usage across loaders
# Expected: All loader classes should follow the same pattern
# Search for loader class definitions and their load methods
ast-grep --pattern 'class $LOADER extends Loader<$_> {
$$$
load($PARAMS) {
$$$
}
$$$
}'
Length of output: 101
Script:
#!/bin/bash
# Let's try a different approach to find loader implementations
# First, find all TypeScript files that might contain loaders
rg -t ts "class.*extends.*Loader" -l
# Then let's examine the load method implementations
rg -t ts "load\s*\(.*\)\s*:\s*AssetPromise" -A 3
Length of output: 11684
packages/loader/src/JSONLoader.ts (1)
1-1: LGTM: Clean import addition
The ResourceManager import is correctly added to support the new parameter.
packages/loader/src/resource-deserialize/resources/scene/EditorTextureLoader.ts (1)
8-8: LGTM! The change aligns with the broader resource management enhancement pattern.
The addition of resourceManager parameter to this.request is consistent with the project-wide refactoring to improve resource management capabilities.
Let's verify this pattern is consistently applied across other loaders:
✅ Verification successful
The resourceManager parameter usage is consistently implemented across all loaders
The verification confirms that all loader implementations follow the same pattern of passing the resourceManager parameter to this.request() calls, with only one exception in Texture2DContentRestorer.ts which uses a different request function. The change in EditorTextureLoader.ts is correctly aligned with the codebase-wide implementation pattern.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the consistent usage of resourceManager in request calls across loaders
# Expected: All loaders should follow the same pattern of passing resourceManager to request
# Search for request method calls in loader files
rg -t typescript 'this\.request.*\(' packages/loader/src/
Length of output: 94
Script:
#!/bin/bash
# Search for request method calls in loader files without file type restriction
rg 'this\.request.*\(' packages/loader/src/
Length of output: 2777
packages/xr/src/loader/XRReferenceImageLoader.ts (1)
8-8: LGTM! Verify consistent pattern across loaders.
The addition of resourceManager parameter to the request call aligns with the broader changes across other loaders.
Let's verify this pattern is consistent across other loaders:
✅ Verification successful
Based on the search results from our verification attempts, I can now generate the final response:
Pattern verified: resourceManager parameter is consistently used across loaders
From the extensive search results of loader files, we can confirm that:
- All loader implementations consistently pass
resourceManageras the second parameter tothis.request() - The pattern
this.request(item.url, resourceManager, { ...item, type: ... })is uniformly followed - Various loaders (TextLoader, SpriteLoader, etc.) all implement this same pattern
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if other loaders follow the same pattern of passing resourceManager to request
# Expected: All loaders should show similar pattern with resourceManager as second argument
# Search for loader request calls
rg -t typescript "this\.request.*resourceManager" packages/
Length of output: 96
Script:
#!/bin/bash
# Let's try a different approach to find loader patterns
# Search for files containing "request" method calls in loader classes
# First, let's find all loader files
fd "Loader\.ts$" packages/ --exec rg -l "this\.request.*" {}
Length of output: 1020
Script:
#!/bin/bash
# Let's also check the actual request calls in these files
fd "Loader\.ts$" packages/ --exec rg -A 2 "this\.request.*" {}
Length of output: 3348
Script:
#!/bin/bash
# Additionally, let's look for the base loader class to understand the expected pattern
rg -l "abstract.*Loader" packages/
Length of output: 70
Script:
#!/bin/bash
# Let's examine the base Loader class to understand the request method signature
rg -A 5 "protected.*request.*" packages/core/src/asset/Loader.ts
Length of output: 66
Script:
#!/bin/bash
# Let's try a different approach to find the request method in the base Loader class
ast-grep --pattern 'abstract class Loader {
$$$
protected request$$$
$$$
}'
Length of output: 83
Script:
#!/bin/bash
# Also check for any interfaces defining the request method
rg -A 5 "interface.*Loader" packages/core/src/asset/
Length of output: 54
packages/loader/src/BufferLoader.ts (2)
1-1: LGTM: Import changes are correct
The addition of ResourceManager to the imports is appropriate for the new functionality.
Line range hint 8-16: LGTM: Method signature update is consistent
The addition of the resourceManager parameter aligns with the standardization effort across loaders while maintaining the existing base64 handling functionality.
Let's verify that all buffer loader usages have been updated to provide the resourceManager parameter:
✅ Verification successful
Let me try a different pattern to catch method calls:
Based on the gathered information, I can now generate the final response:
LGTM: BufferLoader signature change is safe
The BufferLoader is registered as a resource loader for AssetType.Buffer and is only used through the resource manager system. The signature change is safe as:
- The loader is auto-registered via decorator and only instantiated by the resource manager
- All resource loading is done through
engine.resourceManager.load()which internally provides the requiredresourceManagerparameter - No direct instantiation of BufferLoader was found in the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for BufferLoader usage to ensure all calls include resourceManager
ast-grep --pattern 'new BufferLoader().load($$$)'
Length of output: 51
Script:
#!/bin/bash
# Search for BufferLoader instantiation and usage patterns with broader context
rg "BufferLoader" -A 3
# Also search for potential variable declarations and usage
ast-grep --pattern 'const $_ = new BufferLoader()'
Length of output: 617
Script:
#!/bin/bash
# Search for load method calls with broader pattern
ast-grep --pattern '$_.load($$$)'
Length of output: 24164
Script:
#!/bin/bash
# Let's search for direct instantiation and usage of BufferLoader
rg "new BufferLoader\(\)" -A 3
# Also search for any potential variable assignments of BufferLoader instances
ast-grep --pattern 'const $_ = new BufferLoader'
Length of output: 83
Script:
#!/bin/bash
# Let's check how BufferLoader is registered and used in the system
rg "BufferLoader" -B 2 -A 2
# Also check for any resource type registration related to BufferLoader
rg "AssetType.*Buffer" -B 2 -A 2
Length of output: 1169
packages/loader/src/PrefabLoader.ts (1)
Line range hint 8-16: LGTM! Clean integration of resourceManager
The changes properly integrate the resourceManager parameter into the request chain while maintaining the existing promise structure. This aligns well with the broader resource management improvements across the codebase.
packages/loader/src/ProjectLoader.ts (1)
17-17: LGTM: resourceManager parameter addition is consistent with the loader architecture.
The addition of resourceManager parameter to the request method aligns with the broader changes across loader classes and supports the new shader loading functionality.
Let's verify the consistency of this change across other loaders:
✅ Verification successful
LGTM: resourceManager parameter is consistently used across all loaders
The verification confirms that the addition of resourceManager parameter to the request method in ProjectLoader follows the established pattern across the entire codebase. All loader classes:
- Receive
resourceManagerin theirloadmethod - Pass it correctly to the
requestmethod - Use consistent parameter ordering:
url,resourceManager,config
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all loader classes follow the same pattern
# of passing resourceManager to request method
# Search for loader classes and their request method usage
ast-grep --pattern 'class $_ extends Loader {
$$$
request<$_>($url, $resourceManager, $_) {
$$$
}
}'
Length of output: 112
Script:
#!/bin/bash
# Let's try a different approach to verify loader implementations
# First, find all loader classes
rg "class.*extends.*Loader" -A 5
# Also search for request method implementations
rg "request.*resourceManager.*\{" -A 3
Length of output: 23138
packages/loader/src/KTXLoader.ts (1)
16-19: LGTM! Verify error handling with resourceManager.
The addition of resourceManager parameter to the request method aligns with the standardization of resource management across loaders. The implementation maintains the existing functionality while integrating with the resource management system.
Let's verify the error handling implementation in the base Loader class:
✅ Verification successful
Error handling for resourceManager is properly implemented
The implementation has robust error handling:
- The base
requestmethod inLoader.tssafely handles virtual path mapping with the resourceManager using optional chaining - The underlying request implementation includes comprehensive error handling:
- HTTP status code validation (200-299 range)
- Network errors via
xhr.onerror - Timeout handling via
xhr.ontimeout - Retry mechanism with configurable count and interval
- Progress tracking and cancellation support via
AssetPromise
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check error handling in the base Loader class's request method
# Expected: Should find error handling code that properly handles resourceManager
# Search for the request method implementation
ast-grep --pattern 'class Loader {
$$$
request<T>($url: string, $resourceManager: any, $options: any) {
$$$
}
$$$
}'
# Search for error handling patterns
rg -A 5 'catch|throw|reject' "$(fd -t f -e ts loader)"
Length of output: 2639
Script:
#!/bin/bash
# Let's try a different approach to find the base Loader class implementation
# First, locate the base Loader class file
fd -t f Loader.ts packages/core/src
# Then examine its content to verify error handling
cat packages/core/src/asset/Loader.ts
Length of output: 1828
Script:
#!/bin/bash
# Let's check the implementation of the request function that's imported
cat packages/core/src/asset/request.ts
# Also check for any error handling in AssetPromise
cat packages/core/src/asset/AssetPromise.ts
Length of output: 12343
packages/loader/src/FontLoader.ts (2)
15-15: LGTM! Proper integration of resourceManager.
The addition of resourceManager parameter to this.request is correctly implemented, maintaining consistency with other loaders in the system.
Line range hint 1-7: Verify type consistency across the engine.
The changes are part of a broader update to include resourceManager in loaders. Let's verify type consistency across other loaders.
✅ Verification successful
Type consistency verified across all loaders in the codebase.
All loader implementations consistently follow the same pattern for the load method signature:
- They extend the
Loader<T>base class - They implement
load(item: LoadItem, resourceManager: ResourceManager): AssetPromise<T> - They properly utilize the
resourceManagerparameter
The changes in FontLoader.ts align with the consistent pattern used across all other loaders in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if all loaders consistently use ResourceManager parameter
# Expected: All loader classes should have similar load method signatures
# Search for loader class definitions and their load methods
ast-grep --pattern 'class $_ extends Loader<$_> {
$$$
load($_, $_) {
$$$
}
}'
Length of output: 89
Script:
#!/bin/bash
# Let's try a different approach to find loader implementations
# First, find all loader files
fd "Loader.ts" --type f --exec echo "=== {} ===" \; cat {}
Length of output: 201
Script:
#!/bin/bash
# Let's try a simpler approach with ripgrep to find loader implementations
# Search for class definitions that extend Loader
rg "class.*extends.*Loader.*" -A 10
Length of output: 26634
packages/loader/src/ShaderChunkLoader.ts (1)
14-14: 🛠️ Refactor suggestion
Replace void[] with a more specific type.
Using void[] as a generic type parameter is confusing and potentially incorrect.
-class ShaderChunkLoader extends Loader<void[]> {
+class ShaderChunkLoader extends Loader<undefined[]> {Likely invalid or redundant comment.
🧰 Tools
🪛 Biome
[error] 14-14: void is confusing outside a return type or a type parameter.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
packages/loader/src/KTXCubeLoader.ts (1)
19-22: LGTM! Verify resource cleanup and consistency.
The addition of resourceManager to the request method aligns with the broader refactoring across loaders. However, let's verify the implementation consistency and resource cleanup.
Let's check the consistency across other loaders and resource cleanup patterns:
✅ Verification successful
Based on the results, I can now provide a conclusive response about the consistency of the resourceManager parameter usage.
The resourceManager parameter usage is consistent across all loaders
The verification shows that all loaders, including KTXCubeLoader, consistently use the resourceManager parameter in their request calls with the same pattern. The implementation in KTXCubeLoader perfectly aligns with the codebase-wide approach:
- All loaders pass
resourceManageras the second parameter - The request configuration (including type) is consistently passed as the third parameter
- Type parameters and request options follow the same pattern across loaders
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent resourceManager usage across loaders and resource cleanup patterns
# Test 1: Check if all loaders consistently use resourceManager in their request calls
echo "Checking loader request patterns..."
ast-grep --pattern 'this.request<$_>($_, resourceManager, $_)'
# Test 2: Look for resource cleanup patterns in loaders
echo "Checking resource cleanup patterns..."
rg -l "dispose|cleanup|destroy" "packages/loader/src/*Loader.ts"
Length of output: 4297
packages/loader/src/gltf/parser/GLTFSchemaParser.ts (1)
17-18:
Verify URL resolution security and error handling.
The code assumes _virtualPathMap will always contain a valid mapping for the URL. This could lead to runtime errors if the mapping is missing.
- Add fallback to original URL if no mapping exists
- Validate resolved URLs to prevent potential security issues
- Add error handling for invalid URL mappings
Suggested implementation:
- const remoteUrl = context.resourceManager._virtualPathMap[url];
+ const remoteUrl = context.resourceManager._virtualPathMap[url] || url;
+ if (!GLTFUtils.isValidResourceUrl(remoteUrl)) {
+ throw new Error(`Invalid resource URL after resolution: ${remoteUrl}`);
+ }Also applies to: 21-21
packages/loader/src/TextureCubeLoader.ts (1)
24-24: LGTM: Resource manager integration looks good.
The addition of resourceManager parameter to this.request aligns with the broader changes across loaders and improves resource management consistency.
packages/loader/src/AnimationClipLoader.ts (2)
Line range hint 1-11: LGTM: Imports and class declaration are well-structured.
The imports are comprehensive, and the class is properly decorated to handle AnimationClip assets with the .ani extension.
17-17: LGTM: Proper integration of resourceManager.
The resourceManager parameter is correctly passed to the request method, aligning with the PR's objective to enhance resource loading capabilities.
packages/loader/src/SpriteLoader.ts (1)
16-16: LGTM: Consistent resource management enhancement
The addition of resourceManager to the request call aligns with the broader changes across other loaders, improving resource management consistency.
packages/loader/src/EnvLoader.ts (1)
20-20: LGTM: ResourceManager integration looks correct.
The addition of the resourceManager parameter to the request method aligns with the broader changes across other loaders.
packages/loader/src/Texture2DLoader.ts (1)
25-25: LGTM: Proper integration of resourceManager.
The addition of resourceManager to the request method aligns with the PR objectives and maintains consistency with other loaders.
packages/loader/src/PrimitiveMeshLoader.ts (3)
8-8: LGTM: ResourceManager import added correctly
The addition of ResourceManager import aligns with the new parameter requirements.
14-16: LGTM: Load method signature updated correctly
The changes align with the broader refactoring to use ResourceManager across loaders, maintaining consistency in the codebase.
14-16: Verify consistent implementation across loaders
Let's ensure this implementation follows the same pattern as other loaders in the codebase.
✅ Verification successful
Let me try a different search pattern since the previous one might have been too specific.
This broader search should help us verify the consistency of the loader implementations across the codebase.
Implementation is consistent with other loaders
The search results confirm that the implementation in PrimitiveMeshLoader follows the same pattern as other loaders in the codebase. Common patterns observed:
- All loaders implement the
loadmethod withLoadItemandResourceManagerparameters - Many loaders extract
enginefromresourceManagerat the start - The
requestmethod is consistently used with appropriate type parameters - Return type is always
AssetPromise<T>
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other loader implementations to verify consistency
echo "Checking other loader implementations..."
ast-grep --pattern 'class $_ extends Loader {
$$$
load($_, resourceManager: ResourceManager) {
const { engine } = resourceManager;
$$$
}
}'
Length of output: 246
Script:
#!/bin/bash
# Search for loader classes and their load methods with broader pattern
echo "Searching for loader implementations..."
ast-grep --pattern 'class $_ extends Loader {
$$$
load($_, $_) {
$$$
}
}'
# Also search for direct load method implementations
echo -e "\nSearching for load method implementations..."
rg "load\(.*\).*\{" --type ts -A 3
Length of output: 9810
packages/loader/src/resource-deserialize/resources/schema/MaterialSchema.ts (2)
99-99: Document the relationship between shader and shaderRef properties.
Referring to the previous review comment about documenting the relationship between shader and shaderRef properties and marking shader as deprecated.
93-93: Verify the impact of changing shaderData.type to enum.
The change from string literals to MaterialLoaderType enum improves type safety, but could break existing code.
✅ Verification successful
Change to MaterialLoaderType enum is safe and properly implemented
The change from string literals to MaterialLoaderType enum is well-implemented and doesn't introduce breaking changes because:
- The enum values match the previous string literals exactly (e.g.,
MaterialLoaderType.Vector2 = "Vector2") - The enum is properly used in the MaterialLoader.ts switch statement for all cases
- No existing code was found using string literals directly, suggesting this is either a new feature or a coordinated change
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for existing string literal usage of shaderData.type
echo "Checking for potential breaking changes in shaderData.type usage:"
rg -B 2 -A 2 'type:\s*["\x27](Vector2|Vector3|Vector4|Color|Float|Texture|Boolean|Integer)["\x27]' --type ts
Length of output: 251
Script:
#!/bin/bash
# Let's try a broader search for MaterialLoaderType usage and related patterns
echo "Searching for MaterialLoaderType enum usage:"
rg -B 2 -A 2 "MaterialLoaderType" --type ts
echo -e "\nSearching for type property in material-related files:"
fd "material" -e ts -i --exec rg -B 2 -A 2 '\.type\s*=' {}
echo -e "\nSearching for shader data type assignments:"
rg -B 2 -A 2 'shaderData.*type' --type ts
Length of output: 4137
packages/loader/src/SpriteAtlasLoader.ts (1)
30-33: LGTM! Resource manager integration looks good.
The addition of resourceManager to the request method is well-implemented and aligns with the broader resource management improvements across loaders.
packages/loader/src/MaterialLoader.ts (3)
4-4: LGTM: Import statements are well-organized and necessary.
The added imports support the new functionality and type safety in the loader implementation.
Also applies to: 14-22
69-105:
Add error handling for texture loading.
The texture loading promises should handle potential failures to prevent the entire material loading from failing.
This issue was previously identified in the past review comments. The suggested fix remains valid:
texturePromises.push(
engine.resourceManager
.getResourceByRef<Texture2D>(value as IAssetRef)
- .then((texture) => {
+ .then(
+ (texture) => {
materialShaderData.setTexture(key, texture);
- })
+ },
+ (error) => {
+ console.warn(`Failed to load texture for key '${key}':`, error);
+ }
+ )
);53-56: 🛠️ Refactor suggestion
Consider enhancing backward compatibility handling.
The current implementation for 1.2-pre version compatibility might need additional error handling for cases where the shader is not found.
} else {
// compatible with 1.2-pre version material schema
const shaderObject = Shader.find(shader);
+ if (!shaderObject) {
+ reject(new Error(`Shader '${shader}' not found`));
+ return;
+ }
resolve(this.getMaterialByShader(materialSchema, shaderObject, engine));
}✅ Verification successful
Based on the search results, I can see that Shader.find() is widely used across the codebase, but there's no explicit error handling when the shader is not found. However, looking at the usage patterns, it's important to note that:
- Most usages are with built-in shaders that are guaranteed to exist (as documented in both English and Chinese docs)
- The code in question is specifically handling backward compatibility for "1.2-pre version" materials
- The error case would be a legitimate scenario that should be handled
Add error handling for shader lookup in backward compatibility code.
The original review comment is correct. When dealing with legacy material schemas, we should handle the case where the shader is not found to prevent potential runtime errors.
} else {
// compatible with 1.2-pre version material schema
const shaderObject = Shader.find(shader);
+ if (!shaderObject) {
+ reject(new Error(`Shader '${shader}' not found while loading 1.2-pre material`));
+ return;
+ }
resolve(this.getMaterialByShader(materialSchema, shaderObject, engine));
}🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for existing usages of Shader.find to understand common patterns
rg "Shader\.find\(" -A 3
Length of output: 17848
packages/loader/src/AnimatorControllerLoader.ts (1)
22-22: LGTM! Consistent with resource management updates.
The addition of resourceManager to the request call aligns with the standardization of resource management across loaders. The change is properly typed and maintains consistency with other loader implementations.
Let's verify that this pattern is consistently applied across other loaders:
✅ Verification successful
Resource management pattern consistently implemented across loaders
The verification shows that the resourceManager parameter is consistently passed to the request method across all loader implementations in the codebase, including AnimatorControllerLoader. This confirms that the change aligns with the established pattern and maintains consistency in resource management.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent usage of resourceManager in loader request calls
# Expected: All loaders should follow the same pattern of passing resourceManager to request
# Search for request method calls in loader files
rg -U "this\.request.*resourceManager" "packages/loader/src/*Loader.ts"
Length of output: 144
Script:
#!/bin/bash
# Let's first find all loader files in the repository
fd -t f "Loader\.ts$" packages/loader/src
# Then search for request method calls in those files
rg -U "this\.request.*\(" packages/loader/src
Length of output: 3804
packages/loader/src/ktx2/KTX2Loader.ts (1)
224-224: LGTM: Resource manager integration looks correct
The addition of resourceManager parameter to the request method aligns with the PR's objective to enhance resource management capabilities.
packages/loader/src/HDRLoader.ts (1)
384-384: LGTM: Resource management enhancement
The addition of resourceManager parameter to the request method aligns with the PR's objective of improving resource management across loaders. The change is consistent with similar modifications in other loader classes.
Let's verify the consistency of this change across other loaders:
✅ Verification successful
Resource management parameter consistently implemented
The verification confirms that the resourceManager parameter is consistently used across all loader implementations in their request method calls. The base Loader class in packages/core/src/asset/Loader.ts defines the contract, and all loader classes, including HDRLoader, follow this pattern with the same parameter ordering and usage. The change aligns with the codebase's resource management architecture.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all loader classes use resourceManager in their request calls
# Expected: All loaders should follow the same pattern
# Search for request method calls in loader classes
rg -U "request<.*>\(.*\)" "packages/loader/src/*Loader.ts"
Length of output: 131
Script:
#!/bin/bash
# Let's first find all loader files in the packages directory
fd "Loader\.ts$" packages/
# Then search for request method calls in those files
echo "=== Searching for request method calls ==="
rg -U "request<.*>\(.*resourceManager.*\)" packages/
Length of output: 2957
packages/loader/src/Utils.ts
Outdated
| static isRelativePath(path: string): boolean { | ||
| return path[0] === "."; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance relative path detection.
The current implementation only checks for paths starting with ".", but misses other relative path patterns.
Consider a more comprehensive check:
static isRelativePath(path: string): boolean {
- return path[0] === ".";
+ if (!path) return false;
+ return path.startsWith('./') || path.startsWith('../') || path === '.' || path === '..';
}📝 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.
| static isRelativePath(path: string): boolean { | |
| return path[0] === "."; | |
| } | |
| static isRelativePath(path: string): boolean { | |
| if (!path) return false; | |
| return path.startsWith('./') || path.startsWith('../') || path === '.' || path === '..'; | |
| } |
packages/loader/src/Utils.ts
Outdated
| /** @internal */ | ||
| export class Utils { | ||
| private static _urlSchema = "files://"; | ||
| static shaderIncludeRegex = /\s#include\s+"([^\\"]+)"/gm; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider refactoring to pure functions instead of static class members.
The class currently only contains static members, which suggests it's being used as a namespace. Modern TypeScript/JavaScript practices favor pure functions in a module.
Consider refactoring to:
- /** @internal */
- export class Utils {
- private static _urlSchema = "files://";
- static shaderIncludeRegex = /\s#include\s+"([^\\"]+)"/gm;
+ /** @internal */
+ const URL_SCHEMA = "files://";
+ export const shaderIncludeRegex = /\s#include\s+"([^\\"]+)"/gm;Committable suggestion skipped: line range outside the PR's diff.
| // @ts-ignore | ||
| const url = resourceManager._virtualPathMap[item.url] ?? item.url; |
There was a problem hiding this comment.
Avoid accessing private properties with @ts-ignore.
The use of @ts-ignore to access the private _virtualPathMap property is concerning. This approach bypasses TypeScript's type safety and could break if the internal implementation of ResourceManager changes.
Consider these improvements:
- Add a public method in
ResourceManagerfor URL resolution:
// In ResourceManager
public resolveVirtualPath(url: string): string {
return this._virtualPathMap[url] ?? url;
}- Then update this loader to use the public API:
- // @ts-ignore
- const url = resourceManager._virtualPathMap[item.url] ?? item.url;
+ const url = resourceManager.resolveVirtualPath(item.url);This change would:
- Maintain type safety
- Provide a stable public API
- Allow for future implementation changes without breaking loaders
- Keep the virtual path resolution logic centralized
packages/loader/src/ShaderLoader.ts
Outdated
| load(item: LoadItem, resourceManager: ResourceManager): AssetPromise<Shader> { | ||
| const { url } = item; | ||
|
|
||
| return this.request<string>(url, resourceManager, { ...item, type: "text" }).then((code: string) => { | ||
| const builtinShader = this.getBuiltinShader(code); | ||
| if (builtinShader) { | ||
| return Shader.find(builtinShader); | ||
| } | ||
|
|
||
| return _loadChunksInCode(code, url, resourceManager).then(() => { | ||
| return Shader.create(code); | ||
| }); | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling in the load method.
The current implementation could benefit from better error handling to provide more meaningful error messages and ensure proper cleanup.
Consider applying these improvements:
load(item: LoadItem, resourceManager: ResourceManager): AssetPromise<Shader> {
const { url } = item;
- return this.request<string>(url, resourceManager, { ...item, type: "text" }).then((code: string) => {
+ return this.request<string>(url, resourceManager, { ...item, type: "text" })
+ .then((code: string) => {
+ if (!code) {
+ throw new Error(`Failed to load shader source from ${url}`);
+ }
const builtinShader = this.getBuiltinShader(code);
if (builtinShader) {
return Shader.find(builtinShader);
}
return _loadChunksInCode(code, url, resourceManager).then(() => {
return Shader.create(code);
- });
+ }).catch(error => {
+ throw new Error(`Failed to process shader chunks: ${error.message}`);
+ });
- });
+ })
+ .catch(error => {
+ throw new Error(`Shader loading failed for ${url}: ${error.message}`);
+ });
}📝 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.
| load(item: LoadItem, resourceManager: ResourceManager): AssetPromise<Shader> { | |
| const { url } = item; | |
| return this.request<string>(url, resourceManager, { ...item, type: "text" }).then((code: string) => { | |
| const builtinShader = this.getBuiltinShader(code); | |
| if (builtinShader) { | |
| return Shader.find(builtinShader); | |
| } | |
| return _loadChunksInCode(code, url, resourceManager).then(() => { | |
| return Shader.create(code); | |
| }); | |
| }); | |
| } | |
| load(item: LoadItem, resourceManager: ResourceManager): AssetPromise<Shader> { | |
| const { url } = item; | |
| return this.request<string>(url, resourceManager, { ...item, type: "text" }) | |
| .then((code: string) => { | |
| if (!code) { | |
| throw new Error(`Failed to load shader source from ${url}`); | |
| } | |
| const builtinShader = this.getBuiltinShader(code); | |
| if (builtinShader) { | |
| return Shader.find(builtinShader); | |
| } | |
| return _loadChunksInCode(code, url, resourceManager).then(() => { | |
| return Shader.create(code); | |
| }).catch(error => { | |
| throw new Error(`Failed to process shader chunks: ${error.message}`); | |
| }); | |
| }) | |
| .catch(error => { | |
| throw new Error(`Shader loading failed for ${url}: ${error.message}`); | |
| }); | |
| } |
| // @ts-ignore | ||
| ShaderLib |
There was a problem hiding this comment.
Remove @ts-ignore by adding proper type definitions.
The @ts-ignore comment suggests underlying type issues that should be addressed properly rather than suppressed.
Consider adding proper type definitions for ShaderLib in the core package:
// In @galacean/engine-core
export interface ShaderLib {
[key: string]: string;
}| // @ts-ignore | ||
| const remoteUrl = context.resourceManager._virtualPathMap[url]; | ||
| const restoreBufferRequests = contentRestorer.bufferRequests; | ||
| const requestConfig = <RequestConfig>{ type: "arraybuffer" }; | ||
| const absoluteUrl = Utils.resolveAbsoluteUrl(url, bufferInfo.uri); | ||
| const absoluteUrl = Utils.resolveAbsoluteUrl(remoteUrl, bufferInfo.uri); |
There was a problem hiding this comment.
💡 Codebase verification
Based on the results, I can now provide a final response:
Direct access to private _virtualPathMap and inconsistent URL resolution patterns require attention
- Found 3 direct accesses to private
_virtualPathMapacross loaders:packages/loader/src/SourceFontLoader.tspackages/loader/src/gltf/parser/GLTFSchemaParser.tspackages/loader/src/gltf/parser/GLTFBufferParser.ts
- URL resolution with
Utils.resolveAbsoluteUrlis used consistently across 4 files:packages/loader/src/SpriteAtlasLoader.tspackages/loader/src/gltf/parser/GLTFBufferParser.tspackages/loader/src/gltf/parser/GLTFTextureParser.tspackages/loader/src/gltf/extensions/KHR_texture_basisu.ts
- High number of
@ts-ignoreusages (45 occurrences) indicates potential type safety issues
The direct access to the private _virtualPathMap property should be replaced with a proper public API method in the ResourceManager class to maintain encapsulation and prevent future breaking changes.
🔗 Analysis chain
Verify consistent URL resolution across loaders.
Let's ensure this URL resolution pattern is consistently implemented across other loaders.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent URL resolution patterns across loaders
# Look for similar patterns of URL resolution in other loader files
echo "Checking URL resolution patterns in loaders..."
rg -A 2 "resolveAbsoluteUrl" "packages/loader/src"
# Check for other direct accesses to _virtualPathMap
echo "Checking for direct _virtualPathMap access..."
rg "_virtualPathMap" "packages/loader/src"
# Look for similar @ts-ignore usage
echo "Checking for @ts-ignore usage..."
rg "@ts-ignore" "packages/loader/src"
Length of output: 5869
| // @ts-ignore | ||
| const remoteUrl = context.resourceManager._virtualPathMap[url]; |
There was a problem hiding this comment.
Address type safety concerns with _virtualPathMap access.
The @ts-ignore comment and accessing _virtualPathMap (with underscore prefix) suggests we're bypassing TypeScript's type checking and potentially accessing internal/private APIs. This could lead to maintainability issues.
Consider:
- Properly typing the
resourceManagerinterface - Using a public API method for virtual path resolution if available
- If this is intentional, document why we need to access the private property
- // @ts-ignore
- const remoteUrl = context.resourceManager._virtualPathMap[url];
+ // TODO: Use proper typing and public API
+ const remoteUrl = context.resourceManager.resolveVirtualPath?.(url) ?? url;Committable suggestion skipped: line range outside the PR's diff.
packages/core/src/asset/Loader.ts
Outdated
| request<U>(url: string, resourceManager: ResourceManager, config: RequestConfig): AssetPromise<U> { | ||
| // @ts-ignore | ||
| const remoteUrl = resourceManager._virtualPathMap[url] ?? url; | ||
| return request(remoteUrl, config); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve type safety and encapsulation.
While the implementation aligns with the PR objectives for HTTP URL loading, there are a few concerns:
- Using
@ts-ignoremasks potential type safety issues - Directly accessing the private
_virtualPathMapproperty breaks encapsulation
Consider this safer implementation:
request<U>(url: string, resourceManager: ResourceManager, config: RequestConfig): AssetPromise<U> {
- // @ts-ignore
- const remoteUrl = resourceManager._virtualPathMap[url] ?? url;
+ const remoteUrl = resourceManager.resolveVirtualPath?.(url) ?? url;
return request(remoteUrl, config);
}This assumes adding a public method to ResourceManager:
// In ResourceManager class
public resolveVirtualPath(path: string): string | undefined {
return this._virtualPathMap[path];
}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/core/src/asset/ResourceManager.ts(2 hunks)
🔇 Additional comments (1)
packages/core/src/asset/ResourceManager.ts (1)
551-554: 🛠️ Refactor suggestion
Add type validation and improve test coverage.
The load configuration lacks type validation and test coverage:
- Missing validation of
resourceConfig.type - Method needs comprehensive test coverage
Add type validation:
- promise = this.load<any>({
- url,
- type: resourceConfig.type
- });
+ if (!resourceConfig.type || !ResourceManager._loaders[resourceConfig.type]) {
+ Logger.warn(`Invalid resource type: ${resourceConfig.type}`);
+ return Promise.resolve(null);
+ }
+
+ promise = this.load<any>({
+ url,
+ type: resourceConfig.type
+ });Let's verify the usage of this method:
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (3)
packages/loader/src/gltf/parser/GLTFSchemaParser.ts (1)
21-21: Consider adding URL validation before creating BufferRequestInfo.The BufferRequestInfo is created with the remoteUrl without validation. While this matches the PR's objective of supporting HTTP URLs, we should ensure the URL is valid.
Consider adding URL validation:
-restoreBufferRequests.push(new BufferRequestInfo(remoteUrl, requestConfig)); +try { + new URL(remoteUrl); // Validate URL format + restoreBufferRequests.push(new BufferRequestInfo(remoteUrl, requestConfig)); +} catch (error) { + throw new Error(`Invalid remote URL format: ${remoteUrl}`); +}packages/core/src/asset/Loader.ts (1)
46-46: Remove debug logging statement.Production code should not contain console.log statements. Consider using a proper logging system if logging is required.
- console.log("request: ", url);packages/core/src/asset/ResourceManager.ts (1)
570-579: Enhance getRemoteUrlByPath with validation and documentation.The method could be more robust with:
- Input validation for the path parameter
- Comprehensive JSDoc documentation
- Test coverage
Consider these improvements:
/** * @internal * Get resource remote url by path in editor. + * @param path - The virtual path with optional query parameters + * @returns The resolved remote URL or the original path if not found */ getRemoteUrlByPath(path: string): string { + if (!path) { + Logger.warn('Empty path provided to getRemoteUrlByPath'); + return path; + } const [key, query] = path.split("?"); const url = this._virtualPathMap[key]; if (!url) return path; return url + (query ? `?${query}` : ''); }🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 578-578: packages/core/src/asset/ResourceManager.ts#L578
Added line #L578 was not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
packages/core/src/asset/Loader.ts(2 hunks)packages/core/src/asset/ResourceManager.ts(3 hunks)packages/loader/src/gltf/parser/GLTFBufferParser.ts(1 hunks)packages/loader/src/gltf/parser/GLTFSchemaParser.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/loader/src/gltf/parser/GLTFBufferParser.ts
🧰 Additional context used
🪛 GitHub Check: codecov/patch
packages/core/src/asset/ResourceManager.ts
[warning] 539-539: packages/core/src/asset/ResourceManager.ts#L539
Added line #L539 was not covered by tests
[warning] 544-546: packages/core/src/asset/ResourceManager.ts#L544-L546
Added lines #L544 - L546 were not covered by tests
[warning] 578-578: packages/core/src/asset/ResourceManager.ts#L578
Added line #L578 was not covered by tests
🔇 Additional comments (5)
packages/loader/src/gltf/parser/GLTFSchemaParser.ts (1)
16-21: Verify integration with other resource loaders.
The changes align with the PR's objective of enhancing asset loading, but we should verify integration with other loaders.
Let's check for consistent URL resolution patterns across loaders:
✅ Verification successful
Let me generate additional scripts to gather more comprehensive information about URL handling patterns across the codebase.
This will help us understand:
- All URL handling patterns across loaders
- Different ways ResourceManager is being used
- Resource loading patterns to ensure consistency
Based on the analysis of the shell script results, I can now provide a final response:
URL resolution is consistently implemented across loaders
The codebase shows a consistent pattern for URL resolution:
- GLTFSchemaParser and GLTFBufferParser use
getRemoteUrlByPathfor remote URL resolution - Other loaders either use direct URLs or handle virtual paths through
_virtualPathMap - All loaders follow the same pattern of using the ResourceManager for URL resolution and resource loading
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent URL resolution patterns across loaders
# Expected: All loaders should use getRemoteUrlByPath consistently
# Search for URL resolution patterns in loader files
echo "Checking URL resolution patterns in loaders:"
rg -A 2 "getRemoteUrlByPath|_virtualPathMap" "packages/loader/src/"
# Search for other potential URL resolution methods
echo "Checking for other URL resolution methods:"
rg -A 2 "resolveUrl|resolveUri|getUrl" "packages/loader/src/"
Length of output: 1275
Script:
#!/bin/bash
# Search for all URL-related patterns and resource loading in the loader package
echo "Checking all URL and resource loading patterns:"
# Look for URL handling patterns
rg -A 3 "url|URL|Uri|URI" "packages/loader/src/"
# Look for ResourceManager usage patterns
ast-grep --pattern 'resourceManager.$_($$$)'
# Check for request/fetch patterns
rg -A 3 "request\(|fetch\(|load\(" "packages/loader/src/"
Length of output: 42465
packages/core/src/asset/Loader.ts (2)
1-1: Remove unused import.
47-48: Improve type safety and encapsulation.
packages/core/src/asset/ResourceManager.ts (2)
332-332: LGTM! Clean virtual path mapping implementation.
The nullish coalescing operator is appropriately used for the fallback to itemURL.
539-553: Add test coverage for getResourceByRef changes.
The static analysis indicates that the changes to getResourceByRef lack test coverage. This is a critical path for shader loading and should be thoroughly tested.
Let's verify the current test coverage:
Would you like me to help generate comprehensive test cases for the getResourceByRef method?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 539-539: packages/core/src/asset/ResourceManager.ts#L539
Added line #L539 was not covered by tests
[warning] 544-546: packages/core/src/asset/ResourceManager.ts#L544-L546
Added lines #L544 - L546 were not covered by tests
Sway007
force-pushed
the
feat/shaderloader
branch
from
461d6df to
44598d8
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
packages/core/src/asset/ResourceManager.ts (3)
193-194: Improve readability by avoiding assignment in expression.The current code uses assignment within an expression, which can be confusing. Consider restructuring for better readability.
- (this._subAssetPromiseCallbacks[remoteAssetBaseURL] ||= {})[assetSubPath] = { - resolvedValue: value - }; + if (!this._subAssetPromiseCallbacks[remoteAssetBaseURL]) { + this._subAssetPromiseCallbacks[remoteAssetBaseURL] = {}; + } + this._subAssetPromiseCallbacks[remoteAssetBaseURL][assetSubPath] = { + resolvedValue: value + };🧰 Tools
🪛 Biome
[error] 193-193: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
Line range hint
331-342: Add error handling for URL parsing and resolution.The URL resolution logic could fail if the URLs are malformed. Consider adding try-catch blocks and validation.
let { url } = item; // Not absolute and base url is set - if (!Utils.isAbsoluteUrl(url) && this.baseUrl) url = Utils.resolveAbsoluteUrl(this.baseUrl, url); + if (!Utils.isAbsoluteUrl(url) && this.baseUrl) { + try { + url = Utils.resolveAbsoluteUrl(this.baseUrl, url); + } catch (e) { + Logger.warn(`Failed to resolve URL: ${url} with base: ${this.baseUrl}`); + return Promise.reject(e); + } + }🧰 Tools
🪛 Biome
[error] 356-356: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
398-419: Add cleanup handling for failed loads.The error handling could be improved to ensure proper cleanup of resources in case of failures.
promise.then( (resource: T) => { if (loader.useCache) { this._addAsset(remoteAssetBaseURL, resource as EngineObject); } delete loadingPromises[remoteAssetBaseURL]; this._releaseSubAssetPromiseCallback(remoteAssetBaseURL); }, - () => { + (error) => { + Logger.warn(`Failed to load asset: ${remoteAssetBaseURL}`, error); delete loadingPromises[remoteAssetBaseURL]; this._releaseSubAssetPromiseCallback(remoteAssetBaseURL); + this._cleanupFailedLoad(remoteAssetBaseURL); } );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/core/src/asset/ResourceManager.ts(6 hunks)
🧰 Additional context used
🪛 Biome
packages/core/src/asset/ResourceManager.ts
[error] 193-193: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 356-356: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 449-449: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
🔇 Additional comments (2)
packages/core/src/asset/ResourceManager.ts (2)
548-562: LGTM with previous comments.
The implementation looks correct, but please address the security concerns raised in the previous review comments about URL parsing and input validation.
186-187: Add validation for virtual path mapping.
The fallback to assetBaseURL is good, but consider validating the mapped URL format to prevent invalid redirections.
- const remoteAssetBaseURL = this._virtualPathMap[assetBaseURL] ?? assetBaseURL;
+ let remoteAssetBaseURL = this._virtualPathMap[assetBaseURL];
+ try {
+ if (remoteAssetBaseURL) {
+ new URL(remoteAssetBaseURL); // Validate URL format
+ } else {
+ remoteAssetBaseURL = assetBaseURL;
+ }
+ } catch (e) {
+ Logger.warn(`Invalid URL in virtual path mapping: ${remoteAssetBaseURL}`);
+ remoteAssetBaseURL = assetBaseURL;
+ }
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (4)
packages/loader/src/Utils.ts (1)
1-13: Add tests and security documentation.Given that this utility class handles critical path resolution and shader includes:
- Add comprehensive unit tests covering:
- Path resolution edge cases
- Security scenarios (path traversal attempts, protocol attacks)
- Various shader include patterns
- Add documentation about:
- Security considerations
- Usage guidelines
- Supported path formats
Would you like me to help generate:
- A comprehensive test suite for the Utils class?
- Security documentation with examples?
🧰 Tools
🪛 Biome
[error] 2-13: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
packages/loader/src/ShaderLoader.ts (2)
13-15: Add documentation for the builtin shader regex pattern.The
_builtinRegexpattern is used to detect builtin shaders from comments, but its purpose and format should be documented for better maintainability.Add JSDoc comment explaining the pattern:
+ /** + * Regex pattern to detect builtin shader directives. + * Matches comments in the format: // @builtin shaderName + * Example: // @builtin pbr + */ private static _builtinRegex = /^\s*\/\/\s*@builtin\s+(\w+)/;
34-37: Simplify conditional check using optional chaining.The conditional check can be simplified using the optional chaining operator.
Apply this improvement:
private getBuiltinShader(code: string) { const match = code.match(ShaderLoader._builtinRegex); - if (match && match[1]) return match[1]; + return match?.[1]; }🧰 Tools
🪛 Biome
[error] 36-36: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/core/src/asset/Loader.ts (1)
44-47: Add JSDoc documentation for the request method.The new
requestmethod lacks documentation explaining its purpose, parameters, and return value. This is particularly important as it's a core method that will be used by various loader implementations.Add JSDoc documentation:
+ /** + * Resolves and requests a resource URL through the resource manager. + * @param url - The resource URL to request + * @param resourceManager - The resource manager instance for URL resolution + * @param config - Request configuration options + * @returns A promise that resolves with the requested resource + */ request<U>(url: string, resourceManager: ResourceManager, config: RequestConfig): AssetPromise<U> {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
packages/core/src/asset/Loader.ts(1 hunks)packages/loader/src/ShaderLoader.ts(1 hunks)packages/loader/src/Utils.ts(1 hunks)
🧰 Additional context used
🪛 Biome
packages/loader/src/ShaderLoader.ts
[error] 36-36: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/loader/src/Utils.ts
[error] 2-13: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
🔇 Additional comments (2)
packages/loader/src/ShaderLoader.ts (1)
1-12: LGTM! Imports and decorator configuration are well structured.
The imports are properly organized and the decorator correctly configures the loader for shader file extensions.
packages/core/src/asset/Loader.ts (1)
44-47: Verify integration with shader loading requirements.
The implementation aligns with the PR objective of enabling HTTP URL loading for Shader assets. However, let's verify the integration points.
✅ Verification successful
Integration with shader loading is properly implemented and verified
The verification confirms that both shader loaders (ShaderLoader and ShaderChunkLoader) correctly utilize the request method from the base Loader class:
- Both loaders use the request method with proper URL resolution through ResourceManager
- They handle text content appropriately with
type: "text"in the request config - The implementation supports both direct shader loading and shader chunk includes
- The virtual path mapping (
resourceManager._virtualPathMap) will work correctly for both shader types
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify shader loader implementations using this request method
# Test 1: Find shader loader implementations
echo "Searching for shader loader implementations..."
ast-grep --pattern 'class $_ extends Loader {
$$$
load($_, $_) {
$$$
request($_, $_, $_)
$$$
}
}'
# Test 2: Find shader-related asset type definitions
echo "Searching for shader asset type definitions..."
rg -l "ShaderAsset|ShaderChunkAsset"
Length of output: 362
Script:
#!/bin/bash
# Let's try a different approach to find shader-related implementations
# Search for shader-related files
echo "=== Searching for shader-related files ==="
fd -e ts -e js shader -t f
# Search for Loader implementations related to shaders
echo -e "\n=== Searching for shader loader classes ==="
rg -l "class.*Loader.*shader" -g "*.ts"
# Search for request usage in shader context
echo -e "\n=== Searching for request usage with shaders ==="
rg "request.*shader" -g "*.ts" -A 2
# Look for shader asset type imports and usage
echo -e "\n=== Searching for shader asset type references ==="
rg "import.*Shader.*Asset" -g "*.ts"
Length of output: 2587
Script:
#!/bin/bash
# Let's examine the shader loaders specifically
# Check ShaderLoader implementation
echo "=== ShaderLoader Implementation ==="
cat packages/loader/src/ShaderLoader.ts
# Check ShaderChunkLoader implementation
echo -e "\n=== ShaderChunkLoader Implementation ==="
cat packages/loader/src/ShaderChunkLoader.ts
# Look for asset types in these files
echo -e "\n=== Asset Types in Shader Loaders ==="
rg "type.*Asset" packages/loader/src/ShaderLoader.ts packages/loader/src/ShaderChunkLoader.ts
# Check for request usage in these loaders
echo -e "\n=== Request Usage in Shader Loaders ==="
rg "request" -A 3 -B 3 packages/loader/src/ShaderLoader.ts packages/loader/src/ShaderChunkLoader.ts
Length of output: 4170
packages/loader/src/Utils.ts
Outdated
| /** @internal */ | ||
| export class Utils { | ||
| private static _urlSchema = "files://"; | ||
| static shaderIncludeRegex = /\s#include\s+"([./]([^\\"]+))"/gm; |
There was a problem hiding this comment.
Strengthen the shader include regex pattern security.
The current regex pattern could potentially allow path traversal attacks. Consider making it more restrictive by:
- Only allowing specific file extensions
- Explicitly preventing directory traversal sequences
- static shaderIncludeRegex = /\s#include\s+"([./]([^\\"]+))"/gm;
+ static shaderIncludeRegex = /\s#include\s+"([a-zA-Z0-9_\-./]+\.(?:glsl|shader))"/gm;Committable suggestion skipped: line range outside the PR's diff.
packages/loader/src/Utils.ts
Outdated
| static pathResolve(path: string, base: string): string { | ||
| return new URL(path, Utils._urlSchema + base).href.substring(Utils._urlSchema.length); | ||
| } |
There was a problem hiding this comment.
Add protocol validation to prevent URL-based attacks.
The current implementation doesn't validate the URL protocol, which could lead to security vulnerabilities. Consider adding protocol validation:
static pathResolve(path: string, base: string): string {
+ const url = new URL(path, Utils._urlSchema + base);
+ if (url.protocol !== 'files:') {
+ throw new Error('Invalid URL protocol. Only files:// is allowed');
+ }
- return new URL(path, Utils._urlSchema + base).href.substring(Utils._urlSchema.length);
+ return url.href.substring(Utils._urlSchema.length);
}📝 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.
| static pathResolve(path: string, base: string): string { | |
| return new URL(path, Utils._urlSchema + base).href.substring(Utils._urlSchema.length); | |
| } | |
| static pathResolve(path: string, base: string): string { | |
| const url = new URL(path, Utils._urlSchema + base); | |
| if (url.protocol !== 'files:') { | |
| throw new Error('Invalid URL protocol. Only files:// is allowed'); | |
| } | |
| return url.href.substring(Utils._urlSchema.length); | |
| } |
| const shader = Shader.create(code); | ||
| // @ts-ignore | ||
| shader._registerPath(url); | ||
| return shader; |
There was a problem hiding this comment.
Improve type safety and error handling for shader creation.
The code has two issues:
- Using @ts-ignore suggests a type definition problem
- No error handling for shader creation
Consider this improvement:
- const shader = Shader.create(code);
- // @ts-ignore
- shader._registerPath(url);
- return shader;
+ try {
+ const shader = Shader.create(code);
+ if (!shader) {
+ throw new Error('Failed to create shader');
+ }
+ // TODO: Add proper type definition for _registerPath
+ (shader as any)._registerPath(url);
+ return shader;
+ } catch (error) {
+ throw new Error(`Failed to create shader: ${error.message}`);
+ }📝 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 shader = Shader.create(code); | |
| // @ts-ignore | |
| shader._registerPath(url); | |
| return shader; | |
| try { | |
| const shader = Shader.create(code); | |
| if (!shader) { | |
| throw new Error('Failed to create shader'); | |
| } | |
| // TODO: Add proper type definition for _registerPath | |
| (shader as any)._registerPath(url); | |
| return shader; | |
| } catch (error) { | |
| throw new Error(`Failed to create shader: ${error.message}`); | |
| } |
Please check if the PR fulfills these requirements
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Add loader for Shader and ShaderChunk asset
Summary by CodeRabbit
Release Notes
New Features
Shader, to enhance asset categorization.ShaderLoaderandShaderChunkLoaderclasses for improved shader asset management.Utilsfor URL manipulation and path validation.Improvements
resourceManagerinto request methods.MaterialLoader,AnimationClipLoader, and other loaders for better shader handling and resource management.GLTFBufferParserandGLTFSchemaParserfor more robust resource requests.Bug Fixes
Deprecations
KTX2ParamsandEngineConfigurationas deprecated to streamline configurations.Removals
MeshLoaderclass to simplify resource management.