Conversation
WalkthroughThe changes extend KTX2 compressed texture loading by adding explicit linear filtering configuration ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web-packages/test/unit/src/effects-webgl/gl-texture.spec.ts (1)
1-60: Avoid test-order coupling for KTX2 loader registration
registerKTX2Loader(0)is only called in the"load 2d astc ktx file"test, but"upload 2d astc"also relies on the KTX2 loader being registered. Running the second test in isolation (e.g. withit.only) or reordering tests would break that assumption.I’d move loader setup/teardown to the suite hooks so all KTX2-related tests are independent:
before(() => { canvas = document.createElement('canvas'); engine = new GLEngine(canvas, { glType: 'webgl' }); renderer = engine.renderer as GLRenderer; gl = renderer.context.gl as WebGLRenderingContext; + registerKTX2Loader(0); }); after(() => { // runs once after the last test in this block engine.dispose(); @@ - // @ts-expect-error - gl = null; + // @ts-expect-error + gl = null; + // keep texture loader registry clean for other suites + unregisterKTX2Loader(); }); @@ - it('load 2d astc ktx file', async () => { - registerKTX2Loader(0); + it('load 2d astc ktx file', async () => {This keeps tests deterministic and avoids hidden dependencies between them.
Also applies to: 105-136
🧹 Nitpick comments (2)
web-packages/test/vite.config.js (1)
22-24: Configuration looks correct for test environment.The
wasmplugin withtargetEnv: 'auto-inline'is appropriate for the test environment, as it embeds the WASM binary directly in the JavaScript bundle, making tests self-contained and portable. This aligns with the PR's goal of adding KTX2 compressed texture support to tests.The comment on line 23 is redundant since it merely repeats the value. Consider removing it:
wasm({ - targetEnv: 'auto-inline', // auto-inline + targetEnv: 'auto-inline', }),web-packages/test/unit/src/effects-webgl/gl-texture.spec.ts (1)
105-136: Replace magic number37808with the ASTC 4x4 constant / runtime valueIn the
"upload 2d astc"test you hard‑code37808in both the GPU info expectation and the memory calculation:expect(gpuInfo).to.deep.equals( - [37808, 0, gl.TEXTURE_2D, [[2048, 2048]]] + [COMPRESSED_RGBA_ASTC_4x4_KHR, 0, gl.TEXTURE_2D, [[2048, 2048]]] ); … - memory += byteLengthForASTC(37808, mipmap[0], mipmap[1]); + memory += byteLengthForASTC(COMPRESSED_RGBA_ASTC_4x4_KHR, mipmap[0], mipmap[1]);or alternatively:
const [internalFormat] = gpuInfo; … memory += byteLengthForASTC(internalFormat as number, mipmap[0], mipmap[1]);Using the symbolic constant or the actual
internalFormatkeeps the test aligned if the format enum ever changes.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/effects-core/src/texture/texture-factory.ts(1 hunks)web-packages/test/unit/src/effects-webgl/gl-texture.spec.ts(5 hunks)web-packages/test/vite.config.js(2 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
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.
Learnt from: ChengYi996
Repo: galacean/effects-runtime PR: 1263
File: plugin-packages/ktx2/src/transcoder/buffer-reader.ts:105-113
Timestamp: 2025-11-25T02:19:43.276Z
Learning: The KTX2 texture format specification mandates little-endian byte order for all data fields, so KTX2 parsers and readers should always use little-endian interpretation.
📚 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-core/src/texture/texture-factory.tsweb-packages/test/unit/src/effects-webgl/gl-texture.spec.ts
📚 Learning: 2025-11-25T02:19:43.276Z
Learnt from: ChengYi996
Repo: galacean/effects-runtime PR: 1263
File: plugin-packages/ktx2/src/transcoder/buffer-reader.ts:105-113
Timestamp: 2025-11-25T02:19:43.276Z
Learning: The KTX2 texture format specification mandates little-endian byte order for all data fields, so KTX2 parsers and readers should always use little-endian interpretation.
Applied to files:
web-packages/test/unit/src/effects-webgl/gl-texture.spec.ts
🧬 Code graph analysis (2)
packages/effects-core/src/texture/texture-factory.ts (1)
packages/effects-core/src/gl/index.ts (1)
glContext(8-8)
web-packages/test/unit/src/effects-webgl/gl-texture.spec.ts (1)
plugin-packages/ktx2/src/ktx2-loader.ts (1)
registerKTX2Loader(190-194)
⏰ 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 (3)
web-packages/test/vite.config.js (1)
4-4: LGTM! Wasm import added appropriately.The addition of the
wasmimport is correct and necessary for enabling WASM-based KTX2 texture transcoding in the test environment.packages/effects-core/src/texture/texture-factory.ts (1)
124-125: Based on the information provided in the review comment and the code snippet shown, let me provide the rewritten review comment:Good defaults for compressed texture filtering; verify test passage.
Adding explicit
minFilter: glContext.LINEARandmagFilter: glContext.LINEARprovides sensible defaults for KTX2 compressed textures. The placement before the...configspread (line 127) correctly allows these defaults to be overridden by caller configuration, which is appropriate design.Verify this resolves the unit test failures for compressed textures. Consider whether any use cases (pixel art, UI elements) might require
NEARESTfiltering—though those can override via config if needed.web-packages/test/unit/src/effects-webgl/gl-texture.spec.ts (1)
622-629: ASTC 4×4 byte-size math looks correctThe updated helper
if (type === COMPRESSED_RGBA_ASTC_4x4_KHR) { return Math.floor((width + 3) / 4) * Math.floor((height + 3) / 4) * 16; }matches the ASTC 4×4 spec (4×4 blocks, 16 bytes per block) and is consistent with the new 2048×2048, single‑mip expectations. No issues from the test side.
2 participants
Summary by CodeRabbit
Release Notes
New Features
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.