Support ktx2 hdr #2587
Support ktx2 hdr #2587
Conversation
|
Warning Rate limit exceeded@gz65555 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 6 minutes and 50 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. 📒 Files selected for processing (1)
""" WalkthroughThis pull request integrates HDR support into the KTX2 workflow by adding new HDR texture formats and updating related transcoding logic. The changes span Cypress configuration improvements (including a test server spawn and shutdown), modifications to several enum definitions, and significant updates in the KTX2 loading and transcoding pipelines. Additionally, obsolete transcoder components have been removed, and new URLs are used for WebAssembly resources. Changes
Sequence Diagram(s)sequenceDiagram
participant Runner as "Cypress Runner"
participant NodeEvents as "setupNodeEvents"
participant ChildProc as "ChildProcess (spawn)"
Runner->>NodeEvents: Call setupNodeEvents()
NodeEvents->>ChildProc: Spawn test server ("npm run e2e:case")
NodeEvents->>Runner: Register shutdown callback
Runner->>NodeEvents: Complete test run
NodeEvents->>ChildProc: Shutdown test server
sequenceDiagram
participant Loader as "KTX2Loader"
participant Transcoder as "BinomialLLCTranscoder"
participant Worker as "Worker Thread"
Loader->>Transcoder: Initialize transcoder (with HDR support)
Transcoder->>Worker: Fetch new JS & WASM resources
Worker->>Transcoder: Process texture data (including HDR formats)
Transcoder->>Loader: Return transcoded result
Assessment against linked issues
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
e2e/case/texture-hdr-ktx2.ts (1)
43-44: Consider adding a fallback for unsupported HDR formats.The test loads an HDR KTX2 texture without checking for format support. Since BC6H requires the EXT_texture_compression_bptc extension which may not be available in all browsers, consider adding a fallback texture or format detection.
engine.resourceManager.load<Texture2D>("/autumn_field_puresky_1k.ktx2").then((tex) => { mtl.baseTexture = tex; + // Handle possible fallback if HDR format is not supported + if (!tex.loaded) { + console.warn("HDR texture format not supported, falling back to LDR version"); + return engine.resourceManager.load<Texture2D>("/autumn_field_puresky_1k_ldr.ktx2").then(fallbackTex => { + mtl.baseTexture = fallbackTex; + updateForE2E(engine); + initScreenshot(engine, camera); + }); + } updateForE2E(engine); initScreenshot(engine, camera); });packages/loader/src/ktx2/KTX2Loader.ts (1)
64-64: Leverage container color model to determine priority formats.
RetrievingformatPrioritiesusing the container’scolorModel(e.g.,"hdr") is a concise way to select the suitable format list. Consider logging or handling unseen color models in future expansions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
cypress.config.ts(3 hunks)e2e/case/texture-hdr-ktx2.ts(1 hunks)packages/core/src/texture/enums/TextureFormat.ts(1 hunks)packages/loader/src/ktx2/KTX2Container.ts(2 hunks)packages/loader/src/ktx2/KTX2Loader.ts(4 hunks)packages/loader/src/ktx2/KTX2TargetFormat.ts(1 hunks)packages/loader/src/ktx2/transcoder/BinomialLLCTranscoder.ts(1 hunks)packages/loader/src/ktx2/transcoder/BinomialLLCWorkerCode.ts(3 hunks)packages/loader/src/ktx2/transcoder/KhronosTranscoder.ts(0 hunks)packages/loader/src/ktx2/transcoder/KhronosWorkerCode.ts(0 hunks)packages/rhi-webgl/src/GLTexture.ts(1 hunks)
💤 Files with no reviewable changes (2)
- packages/loader/src/ktx2/transcoder/KhronosWorkerCode.ts
- packages/loader/src/ktx2/transcoder/KhronosTranscoder.ts
🧰 Additional context used
🪛 Biome (1.9.4)
packages/loader/src/ktx2/KTX2Container.ts
[error] 56-65: This getter should return a value.
(lint/suspicious/useGetterReturn)
🔇 Additional comments (17)
packages/core/src/texture/enums/TextureFormat.ts (1)
34-35: Excellent addition of BC6H texture format for HDR support.The BC6H format is properly integrated into the TextureFormat enum with a clear documentation comment explaining its purpose for high-dynamic range color spaces. The enum value assignment (36) follows the pattern in the file and doesn't conflict with existing values.
packages/loader/src/ktx2/KTX2TargetFormat.ts (1)
20-24: Good implementation of HDR texture support with BC6H and RGBA16.The addition of BC6H and RGBA16 to the KTX2TargetFormat enum is appropriate for supporting HDR textures. The comment documentation is clear, especially noting that RGBA16 is a half float type, which is important for HDR workflows. The comma after R8G8B8A8 maintains proper syntax.
e2e/case/texture-hdr-ktx2.ts (2)
1-50: Well-structured test case for HDR KTX2 textures.This test file provides a complete example for loading and displaying an HDR KTX2 texture. The implementation follows the standard patterns used in the engine and includes all necessary components (camera, light, mesh) to properly test the feature.
43-43:❓ Verification inconclusive
Verify the HDR texture path.
The path
/autumn_field_puresky_1k.ktx2is used without any checks to ensure the file exists. Consider verifying that this texture file is included in your test assets.
🏁 Script executed:
#!/bin/bash # Check if the HDR texture file exists in the expected location fd -p 'autumn_field_puresky_1k.ktx2' --type fileLength of output: 48
Action Required: Verify HDR Texture File
The automated search did not locate
/autumn_field_puresky_1k.ktx2. Please manually verify that this HDR texture file is present in your test assets. If the file is missing or located elsewhere, update the texture path accordingly.packages/loader/src/ktx2/KTX2Container.ts (1)
11-12: Support for HDR format added to ColorModel enum.The enum has been extended with the HDR (High Dynamic Range) color model with value 167, which aligns with the PR objective to support KTX2 HDR textures.
packages/loader/src/ktx2/transcoder/BinomialLLCWorkerCode.ts (3)
83-86: BasisFormat enum extended with HDR-related formats.Added support for BC6H, RGB_HALF, and RGBA_HALF formats which are essential for HDR texture processing.
89-102: Added HDR support to TargetFormat enum with helpful documentation.The TargetFormat enum now includes BC6H and properly documents that this enum is a copy of KTX2TargetFormat needed for worker context.
112-114: Implemented BC6H case in format transcoding logic.The getTranscodeFormatFromTarget function now correctly maps TargetFormat.BC6H to BasisFormat.BC6H, completing the HDR support implementation.
packages/loader/src/ktx2/transcoder/BinomialLLCTranscoder.ts (1)
13-18:✅ Verification successful
Updated URLs for transcoder resources.
The URLs for basis_transcoder.js and basis_transcoder.wasm have been updated to new locations in the portal_ccutza path.
🏁 Script executed:
#!/bin/bash # Verify that the new URLs are accessible curl -I "https://mdn.alipayobjects.com/portal_ccutza/afts/file/A*0YJjQqUYHNsAAAAAAAAAAAAAAQAAAQ/basis_transcoder.js" curl -I "https://mdn.alipayobjects.com/portal_ccutza/afts/file/A*u5r0R4OOqiQAAAAAAAAAAAAAAQAAAQ/basis_transcoder.wasm"Length of output: 3297
URL Verification Successful: Updated transcoder resources are accessible
- In file
packages/loader/src/ktx2/transcoder/BinomialLLCTranscoder.ts(lines 13–18), the URLs now correctly point to the newportal_ccutzalocations.- The verification tests show that both the JavaScript and WASM resources return HTTP 200 responses, confirming they are available.
No further modifications are required.
cypress.config.ts (4)
6-6: Added child_process imports for test server management.Import of ChildProcess and spawn functions enables launching and managing test server processes.
23-23: Enabled experimental Cypress feature for interactive run events.Setting experimentalInteractiveRunEvents to true allows using the after:run event for test server cleanup.
26-30: Added test server lifecycle management.The code now spawns a test server using
npm run e2e:casecommand and properly shuts it down after tests complete.
50-81: Improved image comparison task structure and logging.The compare task has been restructured with better timing information and a simplified conditional check for matching results.
packages/loader/src/ktx2/KTX2Loader.ts (4)
40-41: Add new HDR priority format.
The addition of an"hdr"entry referencesBC6HandRGBA16formats, which is a clear approach for handling HDR textures. Good job defining an explicit fallback, ensuring that if BC6H is unavailable, RGBA16 is selected.
47-47: Include BC6H in_supportedMap.
ReferencingGLCapabilityType.bptcfor BC6H is correct, as the same extension covers both BC6H and BC7.
66-72: Chain-based transcoding process.
The promise chain for initializing and transcoding via BinomialLLCTranscoder is straightforward. Just ensure any errors are propagated up or handled at the call site as you already appear to do in the caller.
183-186: Support new HDR texture formats.
Adding cases forBC6HandRGBA16properly expands HDR coverage. IfTextureFormat.R16G16B16A16is a half-float representation, confirm that it meets your HDR precision needs.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev/1.5 #2587 +/- ##
===========================================
+ Coverage 68.72% 68.84% +0.11%
===========================================
Files 970 969 -1
Lines 101982 101758 -224
Branches 8785 8785
===========================================
- Hits 70092 70058 -34
+ Misses 31625 31434 -191
- Partials 265 266 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| /** The BC6H format is a texture compression format designed to support high-dynamic range (HDR) color spaces in source data. */ | ||
| BC6H, | ||
| /** Half float type. */ | ||
| RGBA16 |
There was a problem hiding this comment.
Should be as consistent as possible with TextureFormat
/** RGBA format, 16 bits per channel. */
R16G16B16A16| @@ -0,0 +1,49 @@ | |||
| /** | |||
There was a problem hiding this comment.
add this e2e test to config.ts and upload fixtures
There was a problem hiding this comment.
The document might is obesolete, Would you please update it?
| @@ -40,12 +37,14 @@ export class KTX2Loader extends Loader<Texture2D | TextureCube> { | |||
| KTX2TargetFormat.ETC, | |||
| KTX2TargetFormat.BC1_BC3, | |||
| KTX2TargetFormat.PVRTC | |||
| ] | |||
| ], | |||
| hdr: [KTX2TargetFormat.BC6H, KTX2TargetFormat.RGBA16] | |||
There was a problem hiding this comment.
use [ColorModel.HDR] as key is better, the same applies to etc1s and uastc
| get isUASTC() { | ||
| return this.dataFormatDescriptor.colorModel === ColorModel.UASTC; | ||
| get colorModel() { | ||
| switch (this.dataFormatDescriptor.colorModel) { |
There was a problem hiding this comment.
just return dataFormatDescriptor.colorModel as colorModel is better:
get colorModel():ColorModel{
return this.dataFormatDescriptor.colorModel;
}
packages/rhi-webgl/src/GLTexture.ts
Outdated
| @@ -124,6 +124,11 @@ export class GLTexture implements IPlatformTexture { | |||
| internalFormat: GLCompressedTextureInternalFormat.RGBA_BPTC_UNORM_EXT, | |||
| isCompressed: true | |||
| }; | |||
| case TextureFormat.BC6H: | |||
| return { | |||
| internalFormat: gl.getExtension("EXT_texture_compression_bptc").COMPRESSED_RGB_BPTC_UNSIGNED_FLOAT_EXT, | |||
There was a problem hiding this comment.
you can use GLCompressedTextureInternalFormat.RGB_BPTC_UNSIGNED_FLOAT_EXT
There was a problem hiding this comment.
Why GLCompressedTextureInternalFormat is deprecated?
| @@ -102,6 +109,8 @@ export function transcode(buffer: Uint8Array, targetFormat: any, KTX2File: any): | |||
| return hasAlpha ? BasisFormat.ETC2 : BasisFormat.ETC1; | |||
| case TargetFormat.PVRTC: | |||
| return hasAlpha ? BasisFormat.PVRTC1_4_RGBA : BasisFormat.PVRTC1_4_RGB; | |||
| case TargetFormat.BC6H: | |||
| return BasisFormat.BC6H; | |||
| return { engine, result, targetFormat, params: ktx2Container.keyValue["GalaceanTextureParams"] as Uint8Array }; | ||
| }); | ||
| const binomialLLCWorker = KTX2Loader._getBinomialLLCTranscoder(); | ||
| return binomialLLCWorker |
There was a problem hiding this comment.
delete EngineConfiguration.ktx2Loader.transcoder
| @@ -40,12 +37,14 @@ export class KTX2Loader extends Loader<Texture2D | TextureCube> { | |||
| KTX2TargetFormat.ETC, | |||
| KTX2TargetFormat.BC1_BC3, | |||
| KTX2TargetFormat.PVRTC | |||
| ] | |||
| ], | |||
| hdr: [KTX2TargetFormat.BC6H, KTX2TargetFormat.RGBA16] | |||
There was a problem hiding this comment.
KTX2TargetFormat.RGBA16 is not processed in _detectSupportedFormat()
| meshRenderer.setMaterial(mtl); | ||
|
|
||
| engine.resourceManager.load<Texture2D>("/autumn_field_puresky_1k.ktx2").then((tex) => { | ||
| mtl.baseTexture = tex; |
There was a problem hiding this comment.
add Bloom or Tonemapping to test HDR color space
| @@ -40,12 +37,14 @@ export class KTX2Loader extends Loader<Texture2D | TextureCube> { | |||
| KTX2TargetFormat.ETC, | |||
| KTX2TargetFormat.BC1_BC3, | |||
| KTX2TargetFormat.PVRTC | |||
| ] | |||
| ], | |||
| hdr: [KTX2TargetFormat.BC6H, KTX2TargetFormat.RGBA16] | |||
There was a problem hiding this comment.
what should KTX2Loader do if textureHalfFloat is not supported in WebGL1?
There was a problem hiding this comment.
I think we should throw an error.
| @@ -31,6 +31,8 @@ export enum TextureFormat { | |||
| BC3 = 11, | |||
| /** RGB(A) compressed format, 128 bits per 4x4 pixel block. */ | |||
| BC7 = 12, | |||
| /** The BC6H format is a texture compression format designed to support high-dynamic range (HDR) color spaces in source data. */ | |||
| BC6H = 36, | |||
There was a problem hiding this comment.
Opt comments :
/** RGB HDR compressed format, 8 bits per pixel. */
| RGBA8 = 13, | ||
| BC6H = 22, | ||
| RGB_HALF = 24, | ||
| RGBA_HALF = 25 |
There was a problem hiding this comment.
RGBA_HALF should return Uint16Array as mipmap ?
| on("after:run", () => { | ||
| console.log("Shutting down test server..."); | ||
| if (server) server.kill(); | ||
| }); |
| @@ -8,7 +8,8 @@ enum DFDTransferFunction { | |||
|
|
|||
| enum ColorModel { | |||
| ETC1S = 163, | |||
| UASTC = 166 | |||
| UASTC = 166, | |||
| HDR = 167 | |||
There was a problem hiding this comment.
Should be adjusted to follow the original intention of enumeration
enum ColorModel {
ETC1S = 163,
UASTC_LDR_4X4 = 166,
UASTC_HDR_4X4 = 167
}| @@ -17,5 +17,9 @@ export enum KTX2TargetFormat { | |||
| /** RG format, 16 bits per pixel. */ | |||
| R8G8, | |||
| /** RGBA format, 32 bits per pixel. */ | |||
| R8G8B8A8 | |||
| R8G8B8A8, | |||
| /** The BC6H format is a texture compression format designed to support high-dynamic range (HDR) color spaces in source data. */ | |||
There was a problem hiding this comment.
/** RGB HDR compressed format, 8 bits per pixel. */
| RGBA8 = 13, | ||
| BC6H = 22, | ||
| RGB_HALF = 24, | ||
| RGBA_HALF = 25 |
There was a problem hiding this comment.
RGB_HALF and RGBA_HALF never use?
There was a problem hiding this comment.
RGBA_HALF has used, RGB_HALF is enum of basisu, which has not been used in web.
| @@ -91,7 +97,8 @@ export function transcode(buffer: Uint8Array, targetFormat: any, KTX2File: any): | |||
| ETC, | |||
| R8, | |||
| RG8, | |||
| RGBA8 | |||
| RGBA8, | |||
| BC6H | |||
There was a problem hiding this comment.
Why not use KTX2TargetFormat directly?
There was a problem hiding this comment.
This function will convert to string for using in the webworker, so the outside KTX2TargetFormat cannot be imported by the worker.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (2)
packages/loader/src/ktx2/KTX2Loader.ts (2)
149-156:⚠️ Potential issueMissing check for R16G16B16A16 format support
The
_detectSupportedFormatmethod doesn't include a case forKTX2TargetFormat.R16G16B16A16, which requires textureHalfFloat support similar to BC6H.Add this check to ensure proper format selection:
case KTX2TargetFormat.R8G8: if (renderer.isWebGL2) return format; + case KTX2TargetFormat.R16G16B16A16: + if (renderer.canIUse(GLCapabilityType.textureHalfFloat)) return format; }
194-195:⚠️ Potential issueInconsistent usage of string keys and enum values
While the
_priorityFormatsobject now usesColorModelenum values as keys, theinitializemethod still uses string literals "etc1s" and "uastc" to update them.Update to use enum values consistently:
if (options.priorityFormats) { - KTX2Loader._priorityFormats["etc1s"] = options.priorityFormats; - KTX2Loader._priorityFormats["uastc"] = options.priorityFormats; + KTX2Loader._priorityFormats[ColorModel.ETC1S] = options.priorityFormats; + KTX2Loader._priorityFormats[ColorModel.UASTC_LDR_4X4] = options.priorityFormats; + // Consider if you need to set for HDR as well + // KTX2Loader._priorityFormats[ColorModel.UASTC_HDR_4X4] = options.priorityFormats; }
♻️ Duplicate comments (1)
packages/loader/src/ktx2/KTX2Loader.ts (1)
198-198: Streamlined transcoder initializationThe code now consistently uses the BinomialLLCTranscoder, which is a good simplification from the previous dual transcoder approach.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
e2e/case/texture-hdr-ktx2.ts(1 hunks)packages/core/src/texture/enums/TextureFormat.ts(1 hunks)packages/loader/src/index.ts(1 hunks)packages/loader/src/ktx2/KTX2Container.ts(2 hunks)packages/loader/src/ktx2/KTX2Loader.ts(4 hunks)packages/loader/src/ktx2/KTX2TargetFormat.ts(1 hunks)packages/rhi-webgl/src/GLTexture.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/loader/src/index.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/rhi-webgl/src/GLTexture.ts
- packages/core/src/texture/enums/TextureFormat.ts
- packages/loader/src/ktx2/KTX2TargetFormat.ts
- e2e/case/texture-hdr-ktx2.ts
- packages/loader/src/ktx2/KTX2Container.ts
🔇 Additional comments (3)
packages/loader/src/ktx2/KTX2Loader.ts (3)
27-42: Improved type safety with ColorModel enumThe refactoring to use enum values (
ColorModel.ETC1S,ColorModel.UASTC_LDR_4X4,ColorModel.UASTC_HDR_4X4) instead of string literals ("etc1s", "uastc") enhances type safety and maintainability. This is a good change.
41-41: HDR support added with appropriate target formatsThe addition of support for
UASTC_HDR_4X4with BC6H and R16G16B16A16 target formats is appropriate for HDR textures. BC6H is designed specifically for HDR compressed textures, while R16G16B16A16 provides an uncompressed fallback with sufficient precision.
183-186: Appropriate texture format mapping for HDR formatsThe new mappings for BC6H and R16G16B16A16 to their corresponding engine texture formats are correctly implemented.
d0383ec to
376098b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (2)
packages/loader/src/ktx2/KTX2Loader.ts (2)
155-181:⚠️ Potential issueBC6H and R16G16B16A16 format detection needs improvement
The current
_detectSupportedFormatmethod doesn't properly handle the new HDR formats. For BC6H, we need to check if ALL capabilities are supported (not just one of them), and R16G16B16A16 requires textureHalfFloat support which isn't checked.Update the format detection to handle HDR formats:
private static _detectSupportedFormat( renderer: any, priorityFormats: KTX2TargetFormat[], isSRGB: boolean ): KTX2TargetFormat | null { for (let i = 0; i < priorityFormats.length; i++) { const format = priorityFormats[i]; const capabilities = this._capabilityMap[format]?.[isSRGB ? DFDTransferFunction.sRGB : DFDTransferFunction.linear]; if (capabilities) { - for (let j = 0; j < capabilities.length; j++) { - if (renderer.canIUse(capabilities[j])) { - return format; - } + if (format === KTX2TargetFormat.BC6H) { + // For BC6H, ALL capabilities must be supported + if (capabilities.every(cap => renderer.canIUse(cap))) { + return format; + } + } else { + // For other formats, ANY capability is sufficient + for (let j = 0; j < capabilities.length; j++) { + if (renderer.canIUse(capabilities[j])) { + return format; + } + } } } else { switch (priorityFormats[i]) { case KTX2TargetFormat.R8G8B8A8: return format; case KTX2TargetFormat.R8: case KTX2TargetFormat.R8G8: if (renderer.isWebGL2) return format; + case KTX2TargetFormat.R16G16B16A16: + if (renderer.canIUse(GLCapabilityType.textureHalfFloat)) return format; + break; } } } return null; }🧰 Tools
🪛 Biome (1.9.4)
[error] 163-163: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
216-217: 🛠️ Refactor suggestionUse enum keys consistently instead of string literals
While the rest of the code has been updated to use enum-based keys for
_priorityFormats, these lines still use string literals.if (options.priorityFormats) { - KTX2Loader._priorityFormats["etc1s"] = options.priorityFormats; - KTX2Loader._priorityFormats["uastc"] = options.priorityFormats; + KTX2Loader._priorityFormats[ColorModel.ETC1S] = options.priorityFormats; + KTX2Loader._priorityFormats[ColorModel.UASTC_LDR_4X4] = options.priorityFormats; + // Consider also setting for HDR format if needed + // KTX2Loader._priorityFormats[ColorModel.UASTC_HDR_4X4] = options.priorityFormats; }
♻️ Duplicate comments (2)
packages/loader/src/ktx2/KTX2Loader.ts (2)
220-220: Unused transcoder option removedThe code now always initializes BinomialLLCTranscoder, resolving a previous issue where the transcoder option in the config was unused.
60-63:⚠️ Potential issueGPU capability check for BC6H format needs improvement
While you've added the BC6H capability mapping, it currently only checks for BPTC support. For proper HDR texture rendering with BC6H, both BPTC compression and half-float texture support are required.
Update the capability map to check for both requirements:
[KTX2TargetFormat.BC6H]: { - [DFDTransferFunction.linear]: [GLCapabilityType.bptc], - [DFDTransferFunction.sRGB]: [GLCapabilityType.bptc] + [DFDTransferFunction.linear]: [GLCapabilityType.bptc, GLCapabilityType.textureHalfFloat], + [DFDTransferFunction.sRGB]: [GLCapabilityType.bptc, GLCapabilityType.textureHalfFloat] },
🧹 Nitpick comments (2)
packages/loader/src/ktx2/KTX2Loader.ts (2)
79-82: Fix code formatting in promise chainThe promise chain formatting doesn't follow project style guidelines. Also, the code simplification to always use BinomialLLCTranscoder is good, but the formatting needs to be fixed.
const binomialLLCWorker = KTX2Loader._getBinomialLLCTranscoder(); -return binomialLLCWorker.init().then(() => binomialLLCWorker.transcode(buffer, targetFormat)).then((result) => { +return binomialLLCWorker.init() + .then(() => binomialLLCWorker.transcode(buffer, targetFormat)) + .then((result) => {🧰 Tools
🪛 ESLint
[error] 82-82: Replace
.init().then(()·=>·binomialLLCWorker.transcode(buffer,·targetFormat))with⏎······.init()⏎······.then(()·=>·binomialLLCWorker.transcode(buffer,·targetFormat))⏎······(prettier/prettier)
🪛 GitHub Check: lint
[failure] 82-82:
Replace.init().then(()·=>·binomialLLCWorker.transcode(buffer,·targetFormat))with⏎······.init()⏎······.then(()·=>·binomialLLCWorker.transcode(buffer,·targetFormat))⏎······
251-255: Consider adding HDR-specific parameters to KTX2ParamsThe
KTX2Paramsinterface could be enhanced to include HDR-specific options, such as preferred HDR formats or HDR-specific processing settings.export interface KTX2Params { /** Priority transcoding format queue which is preferred options, default is BC7/ASTC/BC3_BC1/ETC/PVRTC/R8G8B8A8. */ /** @deprecated */ priorityFormats: KTX2TargetFormat[]; + /** Whether to use HDR processing for compatible formats */ + hdrEnabled?: boolean; + /** Priority formats specifically for HDR textures */ + hdrPriorityFormats?: KTX2TargetFormat[]; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
cypress.config.ts(3 hunks)e2e/case/texture-hdr-ktx2.ts(1 hunks)packages/core/src/texture/enums/TextureFormat.ts(1 hunks)packages/loader/src/index.ts(1 hunks)packages/loader/src/ktx2/KTX2Container.ts(2 hunks)packages/loader/src/ktx2/KTX2Loader.ts(5 hunks)packages/loader/src/ktx2/KTX2TargetFormat.ts(1 hunks)packages/loader/src/ktx2/transcoder/BinomialLLCTranscoder.ts(1 hunks)packages/loader/src/ktx2/transcoder/BinomialLLCWorkerCode.ts(3 hunks)packages/loader/src/ktx2/transcoder/KhronosTranscoder.ts(0 hunks)packages/loader/src/ktx2/transcoder/KhronosWorkerCode.ts(0 hunks)packages/rhi-webgl/src/GLTexture.ts(1 hunks)tests/src/loader/KTX2Loader.test.ts(0 hunks)
💤 Files with no reviewable changes (3)
- tests/src/loader/KTX2Loader.test.ts
- packages/loader/src/ktx2/transcoder/KhronosTranscoder.ts
- packages/loader/src/ktx2/transcoder/KhronosWorkerCode.ts
🚧 Files skipped from review as they are similar to previous changes (9)
- packages/loader/src/index.ts
- packages/loader/src/ktx2/KTX2TargetFormat.ts
- packages/rhi-webgl/src/GLTexture.ts
- packages/loader/src/ktx2/transcoder/BinomialLLCTranscoder.ts
- e2e/case/texture-hdr-ktx2.ts
- packages/loader/src/ktx2/transcoder/BinomialLLCWorkerCode.ts
- packages/core/src/texture/enums/TextureFormat.ts
- packages/loader/src/ktx2/KTX2Container.ts
- cypress.config.ts
🧰 Additional context used
🪛 ESLint
packages/loader/src/ktx2/KTX2Loader.ts
[error] 82-82: Replace .init().then(()·=>·binomialLLCWorker.transcode(buffer,·targetFormat)) with ⏎······.init()⏎······.then(()·=>·binomialLLCWorker.transcode(buffer,·targetFormat))⏎······
(prettier/prettier)
🪛 GitHub Check: lint
packages/loader/src/ktx2/KTX2Loader.ts
[failure] 82-82:
Replace .init().then(()·=>·binomialLLCWorker.transcode(buffer,·targetFormat)) with ⏎······.init()⏎······.then(()·=>·binomialLLCWorker.transcode(buffer,·targetFormat))⏎······
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: e2e (22.x)
- GitHub Check: build (22.x, windows-latest)
- GitHub Check: codecov
🔇 Additional comments (3)
packages/loader/src/ktx2/KTX2Loader.ts (3)
18-18: Import added for HDR supportThe addition of
ColorModelin the import statement is necessary to support the new HDR texture formats enumeration.
27-42: Good implementation of HDR texture format priorityThe changes in
_priorityFormatsproperly transition from string-based keys to enum-based keys (ColorModel.ETC1SandColorModel.UASTC_LDR_4X4), while adding support for HDR textures withColorModel.UASTC_HDR_4X4. The HDR format correctly prioritizes BC6H and R16G16B16A16 formats which are suitable for HDR content.
205-208: Good addition of HDR texture format mappingThe addition of BC6H and R16G16B16A16 in the
_getEngineTextureFormatmethod is necessary for properly handling HDR textures.
8da1641 to
dbee03a
Compare
There was a problem hiding this comment.
Actionable comments posted: 8
🔭 Outside diff range comments (2)
packages/loader/src/ktx2/KTX2Loader.ts (2)
175-180:⚠️ Potential issueMissing format detection for R16G16B16A16.
The
_detectSupportedFormatmethod includes case statements for several non-compressed formats, but the newly added R16G16B16A16 format is missing. This format likely requires the textureHalfFloat capability in WebGL1.switch (priorityFormats[i]) { case KTX2TargetFormat.R8G8B8A8: return format; case KTX2TargetFormat.R8: case KTX2TargetFormat.R8G8: if (renderer.isWebGL2) return format; + case KTX2TargetFormat.R16G16B16A16: + if (renderer.canIUse(GLCapabilityType.textureHalfFloat)) return format; + break; }
151-154: 🛠️ Refactor suggestionMissing error handling for HDR formats in unsupported contexts.
When HDR formats are requested but the required WebGL capabilities (like textureHalfFloat) aren't available, the current fallback to R8G8B8A8 would result in severe color data loss.
For HDR textures, consider adding specific error handling:
if (targetFormat === null) { + if (ktx2Container.colorModel === ColorModel.UASTC_HDR_4X4) { + Logger.error("HDR textures not supported by this device - required capabilities are missing"); + throw new Error("HDR textures not supported by this device"); + } Logger.warn("Can't support any compressed texture, downgrade to RGBA8"); return KTX2TargetFormat.R8G8B8A8; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
cypress.config.ts(3 hunks)e2e/case/texture-hdr-ktx2.ts(1 hunks)e2e/config.ts(1 hunks)packages/core/src/texture/enums/TextureFormat.ts(1 hunks)packages/loader/src/index.ts(1 hunks)packages/loader/src/ktx2/KTX2Container.ts(2 hunks)packages/loader/src/ktx2/KTX2Loader.ts(5 hunks)packages/loader/src/ktx2/KTX2TargetFormat.ts(1 hunks)packages/loader/src/ktx2/transcoder/BinomialLLCTranscoder.ts(1 hunks)packages/loader/src/ktx2/transcoder/BinomialLLCWorkerCode.ts(3 hunks)packages/loader/src/ktx2/transcoder/KhronosTranscoder.ts(0 hunks)packages/loader/src/ktx2/transcoder/KhronosWorkerCode.ts(0 hunks)packages/rhi-webgl/src/GLTexture.ts(1 hunks)tests/src/loader/KTX2Loader.test.ts(0 hunks)
💤 Files with no reviewable changes (3)
- tests/src/loader/KTX2Loader.test.ts
- packages/loader/src/ktx2/transcoder/KhronosTranscoder.ts
- packages/loader/src/ktx2/transcoder/KhronosWorkerCode.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/core/src/texture/enums/TextureFormat.ts
- packages/loader/src/index.ts
- packages/loader/src/ktx2/KTX2TargetFormat.ts
- packages/loader/src/ktx2/transcoder/BinomialLLCTranscoder.ts
- packages/loader/src/ktx2/KTX2Container.ts
- cypress.config.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/rhi-webgl/src/GLTexture.ts (1)
packages/core/src/texture/index.ts (1)
TextureFormat(5-5)
🪛 GitHub Check: codecov/patch
e2e/case/texture-hdr-ktx2.ts
[warning] 18-20: e2e/case/texture-hdr-ktx2.ts#L18-L20
Added lines #L18 - L20 were not covered by tests
[warning] 22-22: e2e/case/texture-hdr-ktx2.ts#L22
Added line #L22 was not covered by tests
[warning] 24-27: e2e/case/texture-hdr-ktx2.ts#L24-L27
Added lines #L24 - L27 were not covered by tests
[warning] 30-33: e2e/case/texture-hdr-ktx2.ts#L30-L33
Added lines #L30 - L33 were not covered by tests
[warning] 35-38: e2e/case/texture-hdr-ktx2.ts#L35-L38
Added lines #L35 - L38 were not covered by tests
[warning] 40-47: e2e/case/texture-hdr-ktx2.ts#L40-L47
Added lines #L40 - L47 were not covered by tests
[warning] 49-51: e2e/case/texture-hdr-ktx2.ts#L49-L51
Added lines #L49 - L51 were not covered by tests
[warning] 53-55: e2e/case/texture-hdr-ktx2.ts#L53-L55
Added lines #L53 - L55 were not covered by tests
e2e/config.ts
[warning] 149-153: e2e/config.ts#L149-L153
Added lines #L149 - L153 were not covered by tests
packages/loader/src/ktx2/KTX2Loader.ts
[warning] 209-209: packages/loader/src/ktx2/KTX2Loader.ts#L209
Added line #L209 was not covered by tests
[warning] 211-211: packages/loader/src/ktx2/KTX2Loader.ts#L211
Added line #L211 was not covered by tests
[warning] 223-223: packages/loader/src/ktx2/KTX2Loader.ts#L223
Added line #L223 was not covered by tests
packages/loader/src/ktx2/transcoder/BinomialLLCWorkerCode.ts
[warning] 83-86: packages/loader/src/ktx2/transcoder/BinomialLLCWorkerCode.ts#L83-L86
Added lines #L83 - L86 were not covered by tests
[warning] 89-91: packages/loader/src/ktx2/transcoder/BinomialLLCWorkerCode.ts#L89-L91
Added lines #L89 - L91 were not covered by tests
[warning] 100-101: packages/loader/src/ktx2/transcoder/BinomialLLCWorkerCode.ts#L100-L101
Added lines #L100 - L101 were not covered by tests
[warning] 112-113: packages/loader/src/ktx2/transcoder/BinomialLLCWorkerCode.ts#L112-L113
Added lines #L112 - L113 were not covered by tests
packages/rhi-webgl/src/GLTexture.ts
[warning] 165-168: packages/rhi-webgl/src/GLTexture.ts#L165-L168
Added lines #L165 - L168 were not covered by tests
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: e2e (22.x)
🔇 Additional comments (10)
packages/rhi-webgl/src/GLTexture.ts (1)
164-168: Verify WebGL extension availability before usageThe implementation for BC6H format works correctly, but there's a potential issue with directly accessing the compressed texture format without first checking if the extension is available. If the WebGL extension
EXT_texture_compression_bptcis not supported by the browser, this could lead to runtime errors.Consider implementing a more robust extension availability check:
case TextureFormat.BC6H: + const bptcExt = gl.getExtension("EXT_texture_compression_bptc"); + if (!bptcExt) { + throw new Error("EXT_texture_compression_bptc extension is not supported"); + } return { internalFormat: GLCompressedTextureInternalFormat.RGB_BPTC_UNSIGNED_FLOAT_EXT, isCompressed: true };This pattern aligns with other robust extension checks in the codebase and provides clearer error messaging.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 165-168: packages/rhi-webgl/src/GLTexture.ts#L165-L168
Added lines #L165 - L168 were not covered by testse2e/config.ts (1)
150-153: Appropriately added KTX2HDR test configurationThe addition of the KTX2HDR test case is correctly configured with appropriate category and threshold value, matching other texture tests in the configuration.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 149-153: e2e/config.ts#L149-L153
Added lines #L149 - L153 were not covered by testspackages/loader/src/ktx2/transcoder/BinomialLLCWorkerCode.ts (2)
89-91: Helpful comment clarifying TargetFormat's relationship to KTX2TargetFormatThe comment explaining that TargetFormat mirrors KTX2TargetFormat is helpful for maintainability.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 89-91: packages/loader/src/ktx2/transcoder/BinomialLLCWorkerCode.ts#L89-L91
Added lines #L89 - L91 were not covered by tests
112-113: BC6H transcoding case added correctlyThe BC6H case in getTranscodeFormatFromTarget function correctly maps to BasisFormat.BC6H.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 112-113: packages/loader/src/ktx2/transcoder/BinomialLLCWorkerCode.ts#L112-L113
Added lines #L112 - L113 were not covered by testse2e/case/texture-hdr-ktx2.ts (2)
1-56: Well-structured e2e test for HDR KTX2 texturesThe e2e test implements a good validation scenario for HDR KTX2 textures with both bloom and tonemapping effects applied, which is critical for testing HDR rendering.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 18-20: e2e/case/texture-hdr-ktx2.ts#L18-L20
Added lines #L18 - L20 were not covered by tests
[warning] 22-22: e2e/case/texture-hdr-ktx2.ts#L22
Added line #L22 was not covered by tests
[warning] 24-27: e2e/case/texture-hdr-ktx2.ts#L24-L27
Added lines #L24 - L27 were not covered by tests
[warning] 30-33: e2e/case/texture-hdr-ktx2.ts#L30-L33
Added lines #L30 - L33 were not covered by tests
[warning] 35-38: e2e/case/texture-hdr-ktx2.ts#L35-L38
Added lines #L35 - L38 were not covered by tests
[warning] 40-47: e2e/case/texture-hdr-ktx2.ts#L40-L47
Added lines #L40 - L47 were not covered by tests
[warning] 49-51: e2e/case/texture-hdr-ktx2.ts#L49-L51
Added lines #L49 - L51 were not covered by tests
[warning] 53-55: e2e/case/texture-hdr-ktx2.ts#L53-L55
Added lines #L53 - L55 were not covered by tests
49-51:✅ Verification successful
Verify the test fixture availability
The test relies on a specific KTX2 texture file being available at the path "/autumn_field_puresky_1k.ktx2". Ensure this file is properly included in the test fixtures.
🏁 Script executed:
#!/bin/bash # Check if the HDR KTX2 test fixture exists in the expected location find public -name "autumn_field_puresky_1k.ktx2" || echo "Test fixture not found"Length of output: 145
🏁 Script executed:
#!/bin/bash # Search the entire repository for the specific HDR KTX2 fixture find . -type f -name "autumn_field_puresky_1k.ktx2" || echo "Test fixture not found" # List any existing .ktx2 files to see where fixtures are stored find . -type f -name "*.ktx2" | head -n 10Length of output: 190
Fixture file confirmed in test fixtures
The KTX2 fileautumn_field_puresky_1k.ktx2is present ate2e/.dev/public/autumn_field_puresky_1k.ktx2, so it will be served correctly at"/autumn_field_puresky_1k.ktx2". No further changes are needed here.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 49-51: e2e/case/texture-hdr-ktx2.ts#L49-L51
Added lines #L49 - L51 were not covered by testspackages/loader/src/ktx2/KTX2Loader.ts (4)
18-18: Appropriate import update for the ColorModel enum.The addition of the ColorModel import is necessary for the refactored code that now uses enum values instead of string literals, which improves type safety.
79-93: Streamlined transcoding process, always using BinomialLLCTranscoder.The code now consistently uses
BinomialLLCTranscoderand properly resolves the reported issues in past reviews about unused transcoder options. This simplification makes the code more maintainable.
41-41: Added support for HDR textures with appropriate format priorities.The addition of a format priority list for
ColorModel.UASTC_HDR_4X4is well-structured, listing BC6H as the preferred format followed by R16G16B16A16 as a fallback.
60-63:❌ Incorrect review comment
BC6H format requires additional capability checks.
The BC6H format requires both BPTC and textureHalfFloat capabilities, but the current implementation only checks for BPTC.
[KTX2TargetFormat.BC6H]: { - [DFDTransferFunction.linear]: [GLCapabilityType.bptc], - [DFDTransferFunction.sRGB]: [GLCapabilityType.bptc] + [DFDTransferFunction.linear]: [GLCapabilityType.bptc, GLCapabilityType.textureHalfFloat], + [DFDTransferFunction.sRGB]: [GLCapabilityType.bptc, GLCapabilityType.textureHalfFloat] },Additionally, the _detectSupportedFormat method needs to be updated to check that all capabilities are present for the BC6H format:
if (capabilities) { - for (let j = 0; j < capabilities.length; j++) { - if (renderer.canIUse(capabilities[j])) { - return format; - } - } + if (format === KTX2TargetFormat.BC6H) { + // For BC6H, all capabilities must be supported + if (capabilities.every(cap => renderer.canIUse(cap))) { + retu 10000 rn format; + } + } else { + // For other formats, any capability is sufficient + for (let j = 0; j < capabilities.length; j++) { + if (renderer.canIUse(capabilities[j])) { + return format; + } + } + } }
🏁 Script executed:
#!/bin/bash # Check if BC6H requires textureHalfFloat in other parts of the code rg -A 3 -B 3 "BC6H|textureHalfFloat" --glob="*.ts" packages/Length of output: 9902
🏁 Script executed:
#!/bin/bash # Display the capability map section in KTX2Loader.ts around BC6H and R16G16B16A16 rg -n -A5 -B5 "private static _capabilityMap" -n packages/loader/src/ktx2/KTX2Loader.tsLength of output: 565
Ignore textureHalfFloat requirement for BC6H
The BC6H compressed format is supported solely via the BPTC extension (EXT_texture_compression_bptc). No additional textureHalfFloat capability check is needed—the existing mapping of BC6H to[GLCapabilityType.bptc]and its detection logic in_detectSupportedFormatare correct.Likely an incorrect or invalid review comment.
| RGBA8 = 13, | ||
| BC6H = 22, | ||
| RGB_HALF = 24, | ||
| RGBA_HALF = 25 |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider documenting the usage of RGB_HALF and RGBA_HALF formats
You've added RGB_HALF and RGBA_HALF formats to the BasisFormat enum, but previous comments questioned whether these formats are ever used. Their inclusion suggests planned support for half-float texture formats, which would be valuable for HDR content.
Consider adding documentation about the intended use of these formats, especially if they're part of future HDR texture support plans but not yet utilized.
🏁 Script executed:
#!/bin/bash
# Check if RGB_HALF and RGBA_HALF are referenced anywhere in the codebase
rg "RGB_HALF|RGBA_HALF" --type tsLength of output: 195
Add documentation for RGB_HALF and RGBA_HALF in BasisFormat
A search confirms that RGB_HALF and RGBA_HALF are only defined in the enum and aren’t referenced elsewhere. Please add documentation clarifying their intended use—e.g. planned HDR support, expected transcoder behavior, and any fallbacks—so that future maintainers and users understand when and how to use these half-float formats.
Points to address:
- File:
packages/loader/src/ktx2/transcoder/BinomialLLCWorkerCode.ts - Lines 83–86 (
BasisFormatenum entries forRGB_HALFandRGBA_HALF) - Add inline JSDoc or a note in the surrounding comments explaining:
- Why these values were introduced
- How/when they’ll be used by the transcoder
- Any prerequisites or feature flags for HDR texture support
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 83-86: packages/loader/src/ktx2/transcoder/BinomialLLCWorkerCode.ts#L83-L86
Added lines #L83 - L86 were not covered by tests
💡 Verification agent
❓ Verification inconclusive
Missing test coverage for BC6H transcoding
The BC6H format additions in both the BasisFormat enum and the getTranscodeFormatFromTarget function lack test coverage according to static analysis.
Consider adding specific unit tests for the BC6H transcoding pathway to ensure it functions correctly in various scenarios.
Also applies to: 112-113
🏁 Script executed:
#!/bin/bash
# Check if there are tests covering the BC6H transcoding
rg -A 5 "BC6H.*transcode|transcode.*BC6H" --type ts test/Length of output: 105
#!/bin/bash
# List all test files in the repository
echo "Test files in repository:"
find . -type f \( -name "*.test.ts" -o -name "*.test.js" -o -name "*.spec.ts" -o -name "*.spec.js" \)
# Search for any references to BC6H in those test files
echo -e "\nSearching for 'BC6H' in test files:"
find . -type f \( -name "*.test.ts" -o -name "*.test.js" -o -name "*.spec.ts" -o -name "*.spec.js" \) -exec grep -Hn "BC6H" {} + || true#!/bin/bash
# List all test and spec files in the repository
echo "=== Test/Spec files found ==="
find . -type f \( -iname "*test.ts" -o -iname "*test.js" -o -iname "*spec.ts" -o -iname "*spec.js" \) -print
# Search for any BC6H references across the entire repo
echo -e "\n=== All BC6H references ==="
rg --color=never -n "BC6H" .
# Search for BC6H specifically in test/spec files
echo -e "\n=== BC6H in test/spec files ==="
grep -Hn "BC6H" $(find . -type f \( -iname "*test.ts" -o -iname "*test.js" -o -iname "*spec.ts" -o -iname "*spec.js" \)) || echo "No BC6H occurrences in test/spec files."Add unit tests for BC6H transcoding
It appears there are no existing tests covering the new BC6H format in BinomialLLCWorkerCode.ts (lines 83–86 and 112–113). Please add specific unit tests to exercise the BC6H transcoding path and verify correct behavior across representative inputs.
Relevant locations:
- packages/loader/src/ktx2/transcoder/BinomialLLCWorkerCode.ts (enum entries at lines 83–86, 112–113)
- getTranscodeFormatFromTarget function handling BC6H
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 83-86: packages/loader/src/ktx2/transcoder/BinomialLLCWorkerCode.ts#L83-L86
Added lines #L83 - L86 were not covered by tests
| RGBA8, | ||
| BC6H |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Ensure BC6H is properly added to the TargetFormat enum
The BC6H format has been correctly added to the TargetFormat enum to align with the KTX2TargetFormat.
🏁 Script executed:
#!/bin/bash
# Confirm that BC6H exists in both enums and they're aligned
rg -A 15 "enum KTX2TargetFormat" --type tsLength of output: 1375
Add missing BC6H to KTX2TargetFormat enum
BC6H was added in BinomialLLCWorkerCode.ts but is not present in packages/loader/src/ktx2/KTX2TargetFormat.ts. To keep both enums aligned, insert BC6H (with its doc comment) into the KTX2TargetFormat enum, for example just after BC7:
export enum KTX2TargetFormat {
/** RGB(A) compressed format, 128 bits per 4x4 pixel block. */
ASTC,
/** RGB(A) compressed format, 128 bits per 4x4 pixel block (HDR). */
BC7,
/** RGB(A) compressed format, HDR, 128 bits per 4×4 pixel block. */
BC6H, ← add this line
/** RGB(A) compressed format, 4 bits per pixel if no alpha channel, 8 bits per pixel if has alpha channel. */
BC1_BC3,
…
}- File to update: packages/loader/src/ktx2/KTX2TargetFormat.ts
- Add BC6H with a matching documentation comment.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 100-101: packages/loader/src/ktx2/transcoder/BinomialLLCWorkerCode.ts#L100-L101
Added lines #L100 - L101 were not covered by tests
| } else { | ||
| return KTX2Loader._getBinomialLLCTranscoder(options.workerCount).init(); | ||
| } | ||
| return KTX2Loader._getBinomialLLCTranscoder(options.workerCount).init(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplified initialization but missing test coverage.
The initialization now always uses _getBinomialLLCTranscoder, which is a good simplification. However, this line lacks test coverage according to the static analysis.
Please add a unit test to verify the initialization behavior:
// Add this to the appropriate test file
it("should properly initialize BinomialLLCTranscoder with the configured worker count", async () => {
const engine = {} as Engine;
const config = {
ktx2Loader: {
workerCount: 2
}
} as EngineConfiguration;
const getBinomialSpy = jest.spyOn(KTX2Loader as any, '_getBinomialLLCTranscoder');
const mockTranscoder = { init: jest.fn().mockResolvedValue(undefined) };
getBinomialSpy.mockReturnValue(mockTranscoder);
await loader.initialize(engine, config);
expect(getBinomialSpy).toHaveBeenCalledWith(2);
expect(mockTranscoder.init).toHaveBeenCalled();
getBinomialSpy.mockRestore();
});🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 223-223: packages/loader/src/ktx2/KTX2Loader.ts#L223
Added line #L223 was not covered by tests
| case KTX2TargetFormat.BC6H: | ||
| return TextureFormat.BC6H; | ||
| case KTX2TargetFormat.R16G16B16A16: | ||
| return TextureFormat.R16G16B16A16; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Added support for HDR texture formats but missing test coverage.
The new texture formats for HDR support (BC6H and R16G16B16A16) have been properly added to the _getEngineTextureFormat method, but these lines lack test coverage according to the static analysis.
Please ensure that you add unit tests to cover these new format mappings. Here's a suggestion for a test:
// Add this to the appropriate test file
it("should map BC6H and R16G16B16A16 to the correct engine formats", () => {
// Mock transcodeResult
const transcodeResult = { hasAlpha: false } as TranscodeResult;
// Test BC6H mapping
const bc6hFormat = KTX2Loader["_getEngineTextureFormat"](KTX2TargetFormat.BC6H, transcodeResult);
expect(bc6hFormat).toBe(TextureFormat.BC6H);
// Test R16G16B16A16 mapping
const r16Format = KTX2Loader["_getEngineTextureFormat"](KTX2TargetFormat.R16G16B16A16, transcodeResult);
expect(r16Format).toBe(TextureFormat.R16G16B16A16);
});🏁 Script executed:
#!/bin/bash
# Search for any existing tests referencing KTX2Loader
rg -n 'KTX2Loader' -g '*.spec.ts'
# Search for BC6H references in test files
rg -n 'BC6H' -g '*.spec.ts'
# Search for R16G16B16A16 references in test files
rg -n 'R16G16B16A16' -g '*.spec.ts'Length of output: 544
🏁 Script executed:
#!/bin/bash
# List all test files in the repository
fd --extension ts --extension js | grep -E '\.(test|spec)\.(ts|js)$' || echo "No test files found"Length of output: 3321
🏁 Script executed:
#!/bin/bash
rg -n 'BC6H' tests/src/loader/KTX2Loader.test.ts
rg -n 'R16G16B16A16' tests/src/loader/KTX2Loader.test.ts
rg -n '_getEngineTextureFormat' tests/src/loader/KTX2Loader.test.tsLength of output: 173
Add unit tests for HDR texture format mappings
The new mappings for KTX2TargetFormat.BC6H and KTX2TargetFormat.R16G16B16A16 in KTX2Loader._getEngineTextureFormat are not covered by existing tests. Please add coverage in tests/src/loader/KTX2Loader.test.ts:
• File to update: tests/src/loader/KTX2Loader.test.ts
Suggested test addition:
import { KTX2Loader } from "../../../packages/loader/src/ktx2/KTX2Loader";
import { KTX2TargetFormat } from "../../../packages/loader/src/ktx2/KTX2Interfaces";
import { TextureFormat } from "@your-engine/core";
describe("KTX2Loader _getEngineTextureFormat", () => {
const dummyResult = { hasAlpha: false } as any;
it("maps BC6H to TextureFormat.BC6H", () => {
const format = (KTX2Loader as any)["_getEngineTextureFormat"](
KTX2TargetFormat.BC6H,
dummyResult,
);
expect(format).toBe(TextureFormat.BC6H);
});
it("maps R16G16B16A16 to TextureFormat.R16G16B16A16", () => {
const format = (KTX2Loader as any)["_getEngineTextureFormat"](
KTX2TargetFormat.R16G16B16A16,
dummyResult,
);
expect(format).toBe(TextureFormat.R16G16B16A16);
});
});🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 209-209: packages/loader/src/ktx2/KTX2Loader.ts#L209
Added line #L209 was not covered by tests
[warning] 211-211: packages/loader/src/ktx2/KTX2Loader.ts#L211
Added line #L211 was not covered by tests
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
packages/loader/src/ktx2/KTX2Loader.ts (1)
224-225:⚠️ Potential issueUse enum values instead of string literals
There's an inconsistency between the _priorityFormats object (which now uses ColorModel enum values as keys) and these assignments that still use string literals.
- if (options.priorityFormats) { - KTX2Loader._priorityFormats["etc1s"] = options.priorityFormats; - KTX2Loader._priorityFormats["uastc"] = options.priorityFormats; - } + if (options.priorityFormats) { + KTX2Loader._priorityFormats[ColorModel.ETC1S] = options.priorityFormats; + KTX2Loader._priorityFormats[ColorModel.UASTC_LDR_4X4] = options.priorityFormats; + // Consider also adding for HDR format + KTX2Loader._priorityFormats[ColorModel.UASTC_HDR_4X4] = options.priorityFormats; + }
♻️ Duplicate comments (3)
packages/loader/src/ktx2/KTX2Loader.ts (3)
228-228: Simplified initialization to use only BinomialLLCTranscoderThe initialization now always uses
_getBinomialLLCTranscoder, which is a good simplification. However, this line lacks test coverage according to previous review comments.
27-34:⚠️ Potential issueRefactored to use enum values but potential regression with string literals
Good refactoring to use ColorModel enum values instead of string literals, which improves type safety and code clarity. Additionally, adding support for HDR textures with the new UASTC_HDR_4X4 color model is a nice enhancement.
However, there's a potential regression at lines 224-225 where the old string literals "etc1s" and "uastc" are still being used, but the keys in _priorityFormats are now enum values.
- KTX2Loader._priorityFormats["etc1s"] = options.priorityFormats; - KTX2Loader._priorityFormats["uastc"] = options.priorityFormats; + KTX2Loader._priorityFormats[ColorModel.ETC1S] = options.priorityFormats; + KTX2Loader._priorityFormats[ColorModel.UASTC_LDR_4X4] = options.priorityFormats;Also applies to: 41-41
60-67:⚠️ Potential issueAdded capability checks for new HDR texture formats
Good addition of capability checks for BC6H and R16G16B16A16 formats to support HDR textures. However, there's a potential issue with how BC6H capabilities are checked.
In the _detectSupportedFormat method (line 173-177), capabilities are checked with an OR condition rather than requiring both BPTC and textureHalfFloat for BC6H. This could lead to selecting BC6H when half-float textures aren't supported.
private static _detectSupportedFormat( renderer: any, priorityFormats: KTX2TargetFormat[], isSRGB: boolean ): KTX2TargetFormat | null { for (let i = 0; i < priorityFormats.length; i++) { const format = priorityFormats[i]; const capabilities = this._capabilityMap[format]?.[isSRGB ? DFDTransferFunction.sRGB : DFDTransferFunction.linear]; if (capabilities) { - for (let j = 0; j < capabilities.length; j++) { - if (renderer.canIUse(capabilities[j])) { - return format; - } - } + if (format === KTX2TargetFormat.BC6H) { + // BC6H requires both BPTC and textureHalfFloat capabilities + if (capabilities.every(cap => renderer.canIUse(cap))) { + return format; + } + } else { + // For other formats, any capability is sufficient + for (let j = 0; j < capabilities.length; j++) { + if (renderer.canIUse(capabilities[j])) { + return format; + } + } + } } else { // ...existing code...Additionally, the R16G16B16A16 format should check for textureHalfFloat support, and throw an error if it's not available, especially in WebGL1.
🧹 Nitpick comments (1)
packages/loader/src/ktx2/KTX2Loader.ts (1)
85-85: Fix string literal quotes styleUse double quotes instead of single quotes for string literals to maintain consistency with the codebase style.
- console.log('target format', KTX2TargetFormat[targetFormat]); + console.log("target format", KTX2TargetFormat[targetFormat]);🧰 Tools
🪛 ESLint
[error] 85-85: Replace
'target·format'with"target·format"(prettier/prettier)
🪛 GitHub Check: lint
[failure] 85-85:
Replace'target·format'with"target·format"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
e2e/case/texture-hdr-ktx2.ts(1 hunks)packages/loader/src/ktx2/KTX2Loader.ts(5 hunks)packages/loader/src/ktx2/transcoder/BinomialLLCWorkerCode.ts(5 hunks)
✅ Files skipped from review due to trivial changes (1)
- e2e/case/texture-hdr-ktx2.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/loader/src/ktx2/transcoder/BinomialLLCWorkerCode.ts
🧰 Additional context used
🪛 ESLint
packages/loader/src/ktx2/KTX2Loader.ts
[error] 85-85: Replace 'target·format' with "target·format"
(prettier/prettier)
🪛 GitHub Check: lint
packages/loader/src/ktx2/KTX2Loader.ts
[failure] 85-85:
Replace 'target·format' with "target·format"
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: codecov
- GitHub Check: build (22.x, windows-latest)
- GitHub Check: e2e (22.x)
🔇 Additional comments (3)
packages/loader/src/ktx2/KTX2Loader.ts (3)
18-18: Improved import to include ColorModel enum for HDR supportGood addition of the ColorModel enum import, which allows for distinguishing between LDR and HDR UASTC formats.
86-98: Simplified transcoding workflow by removing KhronosTranscoderGood simplification of the transcoding workflow to use only BinomialLLCTranscoder, which reduces complexity and makes the code more maintainable.
213-216: Added mapping for HDR texture formatsGood addition of mappings for new HDR texture formats (BC6H and R16G16B16A16). However, these new format mappings lack test coverage according to previous review comments.
Please add unit tests for these new format mappings as suggested in a previous review. The test should verify that:
- BC6H maps to TextureFormat.BC6H
- R16G16B16A16 maps to TextureFormat.R16G16B16A16
Please check if the PR fulfills these requirements
close: #2520
Summary by CodeRabbit
New Features
Refactor