Support WebP plugin #2310
Support WebP plugin #2310
Conversation
WalkthroughRecent modifications enhance the GLTF framework by introducing optional properties for texture definitions and adding support for WebP textures. The Changes
Sequence Diagram(s)sequenceDiagram
participant A as GLTFParser
participant B as GLTFTextureParser
participant C as EXT_texture_webp
participant D as TextureLoader
A->>B: parse(textureInfo)
B->>D: _parseTexture(imageIndex, textureIndex)
D-->>B: Return Texture
B-->>A: Return Parsed Texture
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 as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- packages/loader/src/gltf/GLTFSchema.ts (1 hunks)
- packages/loader/src/gltf/extensions/EXT_texture_webp.ts (1 hunks)
- packages/loader/src/gltf/extensions/index.ts (1 hunks)
- packages/loader/src/gltf/parser/GLTFTextureParser.ts (2 hunks)
Additional comments not posted (8)
packages/loader/src/gltf/extensions/index.ts (1)
18-18: Importing the WebP extension looks good.The import statement for the
EXT_texture_webpmodule is correctly added, enhancing the module's capabilities by including WebP texture handling functionality.packages/loader/src/gltf/extensions/EXT_texture_webp.ts (3)
1-10: Imports and interface definition look good.The imports from the relevant modules and the interface definition for
EXTWebPSchemaare appropriate and correctly implemented.
12-23: Constructor implementation looks good.The constructor correctly checks for WebP support using a canvas element and handles exceptions appropriately.
25-42:createAndParsemethod implementation looks good.The method correctly handles the creation and parsing of WebP textures, utilizing the
_parseTexturemethod fromGLTFTextureParser.packages/loader/src/gltf/parser/GLTFTextureParser.ts (3)
Line range hint
3-16:
Imports and static property definition look good.The imports from the relevant modules and the static property
_wrapMapare appropriate and correctly implemented.
18-77:_parseTexturemethod implementation looks good.The method correctly handles the parsing and loading of textures, improving modularity and maintainability.
79-91:parsemethod implementation looks good.The method correctly utilizes the
_parseTexturemethod, enhancing code organization and readability.packages/loader/src/gltf/GLTFSchema.ts (1)
766-766: LGTM! But verify the impact on the codebase.The change to make the
sourceproperty optional in theITextureinterface increases flexibility. However, ensure that all parts of the codebase that use this interface handle thesourceproperty being potentially undefined.
There was a problem hiding this comment.
Actionable comments posted: 7
Outside diff range, codebase verification and nitpick comments (1)
packages/loader/src/gltf/parser/GLTFTextureParser.ts (1)
Line range hint
90-95:
Ensure promise rejection is handled.Ensure that any errors during texture parsing are properly handled and rejected.
- return Promise.resolve(texture).then((texture) => { + return Promise.resolve(texture) + .then((texture) => { + GLTFParser.executeExtensionsAdditiveAndParse(extensions, context, texture, textureInfo); + // @ts-ignore + texture._associationSuperResource(glTFResource); + return texture; + }) + .catch((error) => { + console.error(`Failed to parse texture: ${textureName}`, error); + throw error; + });
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- packages/loader/src/gltf/parser/GLTFTextureParser.ts (2 hunks)
Additional comments not posted (2)
packages/loader/src/gltf/parser/GLTFTextureParser.ts (2)
18-24: Good modularization with_parseTexture.The introduction of
_parseTextureimproves code organization by encapsulating the texture loading logic.
79-79: Improved readability with_parseTexturedelegation.The delegation to
_parseTextureimproves readability and maintainability.
| const { glTFResource, glTF } = context; | ||
| const { engine, url } = glTFResource; | ||
| const { sampler, source = 0, name: textureName, extensions } = textureInfo; | ||
| const { uri, bufferView: bufferViewIndex, mimeType, name: imageName } = glTF.images[source]; | ||
| const { uri, bufferView: bufferViewIndex, mimeType, name: imageName } = glTF.images[imageIndex]; |
There was a problem hiding this comment.
Ensure imageIndex is within bounds.
Before accessing glTF.images[imageIndex], ensure imageIndex is within the bounds of glTF.images.
+ if (imageIndex < 0 || imageIndex >= glTF.images.length) {
+ throw new Error(`Invalid imageIndex: ${imageIndex}`);
+ }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 { uri, bufferView: bufferViewIndex, mimeType, name: imageName } = glTF.images[imageIndex]; | |
| if (imageIndex < 0 || imageIndex >= glTF.images.length) { | |
| throw new Error(`Invalid imageIndex: ${imageIndex}`); | |
| } | |
| const { uri, bufferView: bufferViewIndex, mimeType, name: imageName } = glTF.images[imageIndex]; |
| if (uri) { | ||
| const extIndex = uri.lastIndexOf("."); | ||
| const ext = uri.substring(extIndex + 1); | ||
| const type = ext.startsWith("ktx") ? AssetType.KTX : AssetType.Texture2D; |
There was a problem hiding this comment.
Consider supporting additional texture formats.
Currently, the code supports KTX and Texture2D. Consider extending support for other formats like WebP.
- const type = ext.startsWith("ktx") ? AssetType.KTX : AssetType.Texture2D;
+ let type;
+ if (ext.startsWith("ktx")) {
+ type = AssetType.KTX;
+ } else if (ext === "webp") {
+ type = AssetType.WebP;
+ } else {
+ type = AssetType.Texture2D;
+ }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.
| if (uri) { | |
| const extIndex = uri.lastIndexOf("."); | |
| const ext = uri.substring(extIndex + 1); | |
| const type = ext.startsWith("ktx") ? AssetType.KTX : AssetType.Texture2D; | |
| if (uri) { | |
| const extIndex = uri.lastIndexOf("."); | |
| const ext = uri.substring(extIndex + 1); | |
| let type; | |
| if (ext.startsWith("ktx")) { | |
| type = AssetType.KTX; | |
| } else if (ext === "webp") { | |
| type = AssetType.WebP; | |
| } else { | |
| type = AssetType.Texture2D; | |
| } |
| texture = context.get<ArrayBuffer>(GLTFParserType.Buffer).then((buffers) => { | ||
| const buffer = buffers[bufferView.buffer]; | ||
| const imageBuffer = new Uint8Array(buffer, bufferView.byteOffset, bufferView.byteLength); | ||
|
|
||
| return GLTFUtils.loadImageBuffer(imageBuffer, mimeType).then((image) => { | ||
| const texture = new Texture2D(engine, image.width, image.height, undefined, samplerInfo?.mipmap); | ||
| texture.setImageSource(image); | ||
| texture.generateMipmaps(); | ||
| return GLTFUtils.loadImageBuffer(imageBuffer, mimeType).then((image) => { | ||
| const texture = new Texture2D(engine, image.width, image.height, undefined, samplerInfo?.mipmap); |
There was a problem hiding this comment.
Handle potential errors in buffer view processing.
Ensure proper error handling when processing buffer views and loading image buffers.
+ if (!bufferView) {
+ throw new Error(`Buffer view not found for index: ${bufferViewIndex}`);
+ }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.
| texture = context.get<ArrayBuffer>(GLTFParserType.Buffer).then((buffers) => { | |
| const buffer = buffers[bufferView.buffer]; | |
| const imageBuffer = new Uint8Array(buffer, bufferView.byteOffset, bufferView.byteLength); | |
| return GLTFUtils.loadImageBuffer(imageBuffer, mimeType).then((image) => { | |
| const texture = new Texture2D(engine, image.width, image.height, undefined, samplerInfo?.mipmap); | |
| texture.setImageSource(image); | |
| texture.generateMipmaps(); | |
| return GLTFUtils.loadImageBuffer(imageBuffer, mimeType).then((image) => { | |
| const texture = new Texture2D(engine, image.width, image.height, undefined, samplerInfo?.mipmap); | |
| if (!bufferView) { | |
| throw new Error(`Buffer view not found for index: ${bufferViewIndex}`); | |
| } | |
| texture = context.get<ArrayBuffer>(GLTFParserType.Buffer).then((buffers) => { | |
| const buffer = buffers[bufferView.buffer]; | |
| const imageBuffer = new Uint8Array(buffer, bufferView.byteOffset, bufferView.byteLength); | |
| return GLTFUtils.loadImageBuffer(imageBuffer, mimeType).then((image) => { | |
| const texture = new Texture2D(engine, image.width, image.height, undefined, samplerInfo?.mipmap); |
| const bufferTextureRestoreInfo = new BufferTextureRestoreInfo(texture, bufferView, mimeType); | ||
| context.contentRestorer.bufferTextures.push(bufferTextureRestoreInfo); |
There was a problem hiding this comment.
Ensure bufferView and mimeType are valid.
Before creating BufferTextureRestoreInfo, ensure bufferView and mimeType are valid.
+ if (!bufferView || !mimeType) {
+ throw new Error(`Invalid bufferView or mimeType for texture: ${textureName}`);
+ }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 bufferTextureRestoreInfo = new BufferTextureRestoreInfo(texture, bufferView, mimeType); | |
| context.contentRestorer.bufferTextures.push(bufferTextureRestoreInfo); | |
| if (!bufferView || !mimeType) { | |
| throw new Error(`Invalid bufferView or mimeType for texture: ${textureName}`); | |
| } | |
| const bufferTextureRestoreInfo = new BufferTextureRestoreInfo(texture, bufferView, mimeType); | |
| context.contentRestorer.bufferTextures.push(bufferTextureRestoreInfo); |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- packages/loader/src/gltf/extensions/EXT_texture_webp.ts (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- packages/loader/src/gltf/extensions/EXT_texture_webp.ts
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- packages/loader/src/gltf/extensions/EXT_texture_webp.ts (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- packages/loader/src/gltf/extensions/EXT_texture_webp.ts
| this._supportWebP = document.createElement("canvas").toDataURL("image/webp").indexOf("data:image/webp") == 0; | ||
| } catch (e) { | ||
| this._supportWebP = false; | ||
| } |
Please check if the PR fulfills these requirements
Support #2309
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
What is the current behavior? (You can also link to an open issue here)
What is the new behavior (if this is a feature change)?
Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)
Other information:
Summary by CodeRabbit
New Features
sourceproperty in the texture interface for increased flexibility in texture definitions.Bug Fixes
Refactor