Conversation
📝 WalkthroughWalkthroughAdds a propagated Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App
participant TextureAPI as Texture.createWithData
participant GLTexture
participant WebGL as WebGLContext
App->>TextureAPI: createWithData(..., generateMipmap)
TextureAPI->>GLTexture: new GLTexture(..., generateMipmap)
GLTexture->>WebGL: texImage2D / texSubImage2D (upload data/image)
alt generateMipmap == true
GLTexture->>GLTexture: createMipmap(width,height) check
alt can generate (WebGL2 or POT)
GLTexture->>WebGL: generateMipmap()
WebGL-->>GLTexture: mipmaps created
else cannot generate
GLTexture->>GLTexture: set minFilter = LINEAR (downgrade)
GLTexture->>GLTexture: log error/info
end
else generateMipmap == false
Note right of GLTexture: keep existing filters/settings
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/effects-webgl/src/gl-texture.ts (1)
202-228: Consider adding mipmap support for cube map data textures.The new mipmap generation logic (lines 211-227) only applies to 2D data textures. Cube map data textures (lines 202-208) don't receive mipmap generation, even when
generateMipmapis set.For consistency and completeness, consider adding similar mipmap logic for cube maps after the loop at line 208.
🔎 Suggested implementation for cube map mipmaps
if (target === gl.TEXTURE_CUBE_MAP) { optionsCube.forEach((data, key) => { const [x, y] = this.texImage2DData(gl, gl.TEXTURE_CUBE_MAP_POSITIVE_X + key, 0, internalFormat, format, type, data as TextureDataType); width = Math.max(x, width); height = Math.max(y, height); }); + const wantsMip = + source.minFilter === gl.LINEAR_MIPMAP_LINEAR || + source.minFilter === gl.LINEAR_MIPMAP_NEAREST || + source.minFilter === gl.NEAREST_MIPMAP_LINEAR || + source.minFilter === gl.NEAREST_MIPMAP_NEAREST; + + const canGenMip = + !!generateMipmap && ((isPowerOfTwo(width) && isPowerOfTwo(height)) || isWebGL2(gl)); + + if (wantsMip) { + if (canGenMip) { + gl.generateMipmap(target); + } else { + source.minFilter = gl.LINEAR; + } + } } else {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/effects-webgl/src/gl-texture.tspackages/effects/src/index.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-10T07:59:18.725Z
Learnt from: ChengYi996
Repo: galacean/effects-runtime PR: 1263
File: packages/effects-core/src/texture/ktx2/transcoder/binomial-workercode.ts:99-110
Timestamp: 2025-11-10T07:59:18.725Z
Learning: In packages/effects-core/src/texture/ktx2/transcoder/binomial-workercode.ts, the KTX2TargetFormat.ETC case in getTranscodeFormatFromTarget already handles ETC1 through the hasAlpha flag (returns BasisFormat.ETC1 when hasAlpha is false, BasisFormat.ETC2 when true), so a separate ETC1 case is not needed.
Applied to files:
packages/effects-webgl/src/gl-texture.ts
🧬 Code graph analysis (1)
packages/effects-webgl/src/gl-texture.ts (3)
packages/effects-core/src/texture/types.ts (2)
Texture2DSourceOptionsImage(131-136)Texture2DSourceOptionsData(138-142)packages/effects-core/src/utils/index.ts (1)
isPowerOfTwo(127-129)packages/effects-core/src/render/gpu-capability.ts (1)
isWebGL2(57-59)
🔇 Additional comments (3)
packages/effects/src/index.ts (2)
37-37: LGTM - Good backward compatibility.The default value of
falseensures existing code continues to work without mipmaps, which is the expected behavior.
52-52: LGTM - Proper option propagation.The
generateMipmapoption is correctly passed to the GLTexture constructor, following the same pattern as other texture configuration options.packages/effects-webgl/src/gl-texture.ts (1)
211-227: LGTM - Solid mipmap generation logic.The implementation correctly:
- Detects when mipmaps are required based on
minFiltersettings- Validates mipmap generation capability (power-of-two dimensions for WebGL1, or WebGL2)
- Generates mipmaps when feasible, or gracefully downgrades to
LINEARfiltering to prevent sampling errorsThe fallback strategy ensures textures render correctly even when mipmap generation isn't possible.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/effects-webgl/src/gl-texture.ts (1)
134-134: Verify thatTexture2DSourceOptionsDatatype definition includesgenerateMipmap.This is the same concern raised in the previous review: the type cast includes
Texture2DSourceOptionsData, but according to the type definitions inpackages/effects-core/src/texture/types.ts, onlyTexture2DSourceOptionsImagedeclares thegenerateMipmapproperty. AccessinggenerateMipmapfromTexture2DSourceOptionsDatawill beundefinedat runtime unless the type definition has been updated.Please confirm that
generateMipmap?: booleanhas been added to theTexture2DSourceOptionsDatainterface.#!/bin/bash # Verify that Texture2DSourceOptionsData includes generateMipmap property rg -n -A 10 "interface Texture2DSourceOptionsData" packages/effects-core/src/texture/types.ts
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/effects-webgl/src/gl-texture.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-10T07:59:18.725Z
Learnt from: ChengYi996
Repo: galacean/effects-runtime PR: 1263
File: packages/effects-core/src/texture/ktx2/transcoder/binomial-workercode.ts:99-110
Timestamp: 2025-11-10T07:59:18.725Z
Learning: In packages/effects-core/src/texture/ktx2/transcoder/binomial-workercode.ts, the KTX2TargetFormat.ETC case in getTranscodeFormatFromTarget already handles ETC1 through the hasAlpha flag (returns BasisFormat.ETC1 when hasAlpha is false, BasisFormat.ETC2 when true), so a separate ETC1 case is not needed.
Applied to files:
packages/effects-webgl/src/gl-texture.ts
🧬 Code graph analysis (1)
packages/effects-webgl/src/gl-texture.ts (3)
packages/effects-core/src/texture/types.ts (2)
Texture2DSourceOptionsImage(131-136)Texture2DSourceOptionsData(138-142)packages/effects-core/src/utils/index.ts (1)
isPowerOfTwo(127-129)packages/effects-core/src/render/gpu-capability.ts (1)
isWebGL2(57-59)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (javascript-typescript)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/effects-webgl/src/gl-texture.ts (1)
134-134: Type safety issue:generateMipmapis not defined onTexture2DSourceOptionsData.According to the type definitions, only
Texture2DSourceOptionsImagehas thegenerateMipmapproperty. Accessing it fromTexture2DSourceOptionsDatawill result inundefinedat runtime, which is then defaulted tofalse.Since the PR objective is to enable mipmap generation for data textures, the proper fix is to add
generateMipmap?: booleanto theTexture2DSourceOptionsDatainterface inpackages/effects-core/src/texture/types.ts.🔎 Proposed fix: Update the type definition
In
packages/effects-core/src/texture/types.ts, update theTexture2DSourceOptionsDatainterface:export interface Texture2DSourceOptionsData extends TextureOptionsBase { sourceType?: TextureSourceType.data, data: TextureDataType, target?: WebGLRenderingContext['TEXTURE_2D'], + generateMipmap?: boolean, }
🧹 Nitpick comments (1)
packages/effects-webgl/src/gl-texture.ts (1)
189-195: Uselogger.errorfor consistency.The file consistently uses
logger.errorfor error logging (lines 147, 162, 496), but this helper usesconsole.error. For consistency and proper logging infrastructure, uselogger.errorinstead.🔎 Proposed fix
const createMipmap = (textureWidth: number, textureHeight: number) => { if ((isPowerOfTwo(textureWidth) && isPowerOfTwo(textureHeight)) || isWebGL2(gl)) { gl.generateMipmap(target); } else { - console.error('Mipmap generation failed. Texture width and height must be power of two in WebGL1.'); + logger.error('Mipmap generation failed. Texture width and height must be power of two in WebGL1.'); } };
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/effects-webgl/src/gl-texture.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-10T07:59:18.725Z
Learnt from: ChengYi996
Repo: galacean/effects-runtime PR: 1263
File: packages/effects-core/src/texture/ktx2/transcoder/binomial-workercode.ts:99-110
Timestamp: 2025-11-10T07:59:18.725Z
Learning: In packages/effects-core/src/texture/ktx2/transcoder/binomial-workercode.ts, the KTX2TargetFormat.ETC case in getTranscodeFormatFromTarget already handles ETC1 through the hasAlpha flag (returns BasisFormat.ETC1 when hasAlpha is false, BasisFormat.ETC2 when true), so a separate ETC1 case is not needed.
Applied to files:
packages/effects-webgl/src/gl-texture.ts
🧬 Code graph analysis (1)
packages/effects-webgl/src/gl-texture.ts (3)
packages/effects-core/src/texture/types.ts (2)
Texture2DSourceOptionsImage(131-136)Texture2DSourceOptionsData(138-142)packages/effects-core/src/utils/index.ts (1)
isPowerOfTwo(127-129)packages/effects-core/src/render/gpu-capability.ts (1)
isWebGL2(57-59)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (2)
packages/effects-webgl/src/gl-texture.ts (2)
220-222: Good addition: enables mipmap generation for data textures.This correctly implements the PR objective by allowing data textures to generate mipmaps when explicitly requested. The conditional check ensures mipmaps are only generated when the
generateMipmapflag is set.
241-243: Good refactoring: centralizes mipmap generation logic.The refactoring to use the
createMipmaphelper unifies the mipmap generation logic across different texture source types (data, image, video) and adds consistent error handling for WebGL1 non-power-of-two textures.
3 participants
支持createWithData的纹理生成mipmap
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.