Support ktx2 hdr #2587
Support ktx2 hdr #2587
Conversation
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
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (3)
🪧 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 @@
## main #2587 +/- ##
==========================================
+ Coverage 68.98% 69.03% +0.05%
==========================================
Files 961 960 -1
Lines 100339 100105 -234
Branches 8681 8678 -3
==========================================
- Hits 69215 69109 -106
+ Misses 30864 30734 -130
- Partials 260 262 +2
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
| @@ -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;
}| @@ -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
| @@ -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?
| @@ -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?
| @@ -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?
Please check if the PR fulfills these requirements
close: #2520
Summary by CodeRabbit
New Features
Refactor