Support sRGB color space #2593
Support sRGB color space #2593
Conversation
WalkthroughThis pull request implements extensive modifications across the engine’s core, texture, loader, render pipeline, and testing modules. Changes introduce new parameters and static properties for handling sRGB color spaces, update shader macro management, and refine GL capability detection. Texture classes and loader functions are updated to pass an additional sRGB flag, while GL texture and render target management modules receive new validation and configuration logic. The render pipeline modules are refactored to improve background texture management and shader macro unions. Additional tests and e2e cases validate these changes, and new utility functions for texture processing are introduced. Changes
Sequence Diagram(s)sequenceDiagram
participant Parser as GLTFTextureParser
participant Context as GLTFParserContext
participant Loader as Texture2DLoader
participant Texture as Texture2D
Parser->>Context: parse(textureIndex, isSRGBColorSpace)
Context-->>Parser: Material info with sRGB flag
Parser->>Loader: _parseTexture(..., isSRGBColorSpace)
Loader->>Texture: new Texture2D(..., isSRGBColorSpace)
Texture-->>Loader: Texture instance returned
Loader-->>Parser: Parsed Texture with sRGB configuration
Possibly related PRs
Suggested labels
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🪧 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 (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/loader/src/TextureCubeLoader.ts (1)
34-40: Constructor call updated to support sRGB color space.The TextureCube constructor is now called with the
isSRGBColorSpaceparameter derived from loading parameters, allowing for dynamic configuration of the color space based on the texture loading requirements.One potential improvement would be to explicitly pass the format and mipmap parameters with their default values instead of using
undefined, which would make the code more readable.Consider using explicit defaults instead of
undefined:const texture = new TextureCube( resourceManager.engine, width, - undefined, - undefined, + TextureFormat.R8G8B8A8, + true, item.params?.isSRGBColorSpace ?? false );packages/loader/src/Texture2DLoader.ts (1)
33-41: Pass sRGB parameter to texture constructorThe
Texture2Dconstructor now properly passes theisSRGBColorSpaceparameter. However, I note that theusageparameter is set toundefinedrather than letting it use the default value from the constructor.Consider removing the explicit
undefinedfor theusageparameter to use the constructor's default value:const texture = new Texture2D( resourceManager.engine, image.width, image.height, format, mipmap, - undefined, isSRGBColorSpace );packages/loader/src/gltf/parser/GLTFParserContext.ts (1)
60-102: Document the purpose of the extra parameters in JSDoc.While the implementation correctly handles the extra parameters, it would be helpful to document their purpose and expected values in the method's JSDoc comments for better developer understanding.
Add JSDoc documentation for the
...extraparameters:/** * Gets a resource by type and index. * @param type The type of resource to get * @param index The index of the resource +* @param extra Additional parameters to pass to the parser (e.g., isSRGBColorSpace for textures) * @returns A promise that resolves to the requested resource */ get<T>(type: GLTFParserType, index: number, ...extra): Promise<T>;packages/loader/src/ktx2/KTX2Loader.ts (1)
103-103: Static method uses 'this' instead of class nameThe code uses
thisto refer to the class in a static context, which can be confusing and is flagged by static analysis. Consider using the class name directly.- const engineFormat = this._getEngineTextureFormat(targetFormat, transcodeResult); + const engineFormat = KTX2Loader._getEngineTextureFormat(targetFormat, transcodeResult);🧰 Tools
🪛 Biome (1.9.4)
[error] 103-103: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (14)
packages/core/src/shaderlib/begin_mobile_frag.glslis excluded by!**/*.glslpackages/core/src/shaderlib/common.glslis excluded by!**/*.glslpackages/core/src/shaderlib/extra/particle.fs.glslis excluded by!**/*.glslpackages/core/src/shaderlib/extra/skybox.fs.glslis excluded by!**/*.glslpackages/core/src/shaderlib/extra/sprite.fs.glslis excluded by!**/*.glslpackages/core/src/shaderlib/extra/text.fs.glslis excluded by!**/*.glslpackages/core/src/shaderlib/extra/unlit.fs.glslis excluded by!**/*.glslpackages/core/src/shaderlib/pbr/ibl_frag_define.glslis excluded by!**/*.glslpackages/core/src/shaderlib/pbr/pbr_frag.glslis excluded by!**/*.glslpackages/core/src/shaderlib/pbr/pbr_helper.glslis excluded by!**/*.glslpackages/shader-shaderlab/src/shaders/Common.glslis excluded by!**/*.glslpackages/shader-shaderlab/src/shaders/shadingPBR/FragmentPBR.glslis excluded by!**/*.glslpackages/shader-shaderlab/src/shaders/shadingPBR/LightIndirectFunctions.glslis excluded by!**/*.glslpackages/ui/src/shader/uiDefault.fs.glslis excluded by!**/*.glsl
📒 Files selected for processing (28)
packages/core/src/2d/text/SubFont.ts(1 hunks)packages/core/src/Engine.ts(2 hunks)packages/core/src/base/Constant.ts(1 hunks)packages/core/src/texture/Texture.ts(2 hunks)packages/core/src/texture/Texture2D.ts(2 hunks)packages/core/src/texture/Texture2DArray.ts(2 hunks)packages/core/src/texture/TextureCube.ts(1 hunks)packages/core/src/texture/enums/TextureFormat.ts(1 hunks)packages/loader/src/SpriteAtlasLoader.ts(1 hunks)packages/loader/src/Texture2DLoader.ts(2 hunks)packages/loader/src/TextureCubeLoader.ts(1 hunks)packages/loader/src/gltf/GLTFUtils.ts(1 hunks)packages/loader/src/gltf/extensions/EXT_texture_webp.ts(2 hunks)packages/loader/src/gltf/extensions/KHR_materials_pbrSpecularGlossiness.ts(2 hunks)packages/loader/src/gltf/extensions/KHR_materials_sheen.ts(1 hunks)packages/loader/src/gltf/parser/GLTFMaterialParser.ts(2 hunks)packages/loader/src/gltf/parser/GLTFParser.ts(1 hunks)packages/loader/src/gltf/parser/GLTFParserContext.ts(2 hunks)packages/loader/src/gltf/parser/GLTFTextureParser.ts(4 hunks)packages/loader/src/ktx2/KTX2Loader.ts(2 hunks)packages/loader/src/resource-deserialize/resources/texture2D/TextureDecoder.ts(1 hunks)packages/rhi-webgl/src/GLCapability.ts(5 hunks)packages/rhi-webgl/src/GLRenderTarget.ts(1 hunks)packages/rhi-webgl/src/GLTexture.ts(6 hunks)packages/rhi-webgl/src/GLTexture2D.ts(2 hunks)packages/rhi-webgl/src/GLTexture2DArray.ts(2 hunks)packages/rhi-webgl/src/GLTextureCube.ts(2 hunks)packages/rhi-webgl/src/type.ts(1 hunks)
🧰 Additional context used
🧬 Code Definitions (9)
packages/loader/src/gltf/extensions/KHR_materials_sheen.ts (1)
packages/core/src/material/PBRMaterial.ts (2) (2)
sheenColorTexture(277-279)sheenColorTexture(281-288)
packages/rhi-webgl/src/GLRenderTarget.ts (2)
packages/rhi-webgl/src/GLTexture.ts (1) (1)
GLTexture(19-743)packages/core/src/texture/Texture.ts (2) (2)
format(45-47)isSRGBColorSpace(38-40)
packages/rhi-webgl/src/GLTextureCube.ts (2)
packages/rhi-webgl/src/GLTexture.ts (1) (1)
GLTexture(19-743)packages/core/src/texture/Texture.ts (2) (2)
format(45-47)isSRGBColorSpace(38-40)
packages/core/src/texture/TextureCube.ts (1)
packages/core/src/texture/Texture.ts (2) (2)
format(45-47)isSRGBColorSpace(38-40)
packages/rhi-webgl/src/GLTexture2DArray.ts (2)
packages/rhi-webgl/src/GLTexture.ts (1) (1)
GLTexture(19-743)packages/core/src/texture/Texture.ts (2) (2)
format(45-47)isSRGBColorSpace(38-40)
packages/core/src/texture/Texture2D.ts (1)
packages/core/src/texture/Texture.ts (5) (5)
width(52-54)height(59-61)format(45-47)usage(66-68)isSRGBColorSpace(38-40)
packages/core/src/texture/Texture2DArray.ts (1)
packages/core/src/texture/Texture.ts (4) (4)
width(52-54)height(59-61)format(45-47)isSRGBColorSpace(38-40)
packages/loader/src/gltf/parser/GLTFTextureParser.ts (1)
packages/loader/src/gltf/parser/GLTFParserContext.ts (1) (1)
GLTFParserContext(23-201)
packages/rhi-webgl/src/GLTexture2D.ts (2)
packages/rhi-webgl/src/GLTexture.ts (1) (1)
GLTexture(19-743)packages/core/src/texture/Texture.ts (2) (2)
format(45-47)isSRGBColorSpace(38-40)
🪛 Biome (1.9.4)
packages/loader/src/ktx2/KTX2Loader.ts
[error] 103-103: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
🔇 Additional comments (65)
packages/core/src/texture/Texture.ts (2)
28-28: Approved: New property for sRGB color space support.Adding this protected property correctly implements the needed state tracking for sRGB color space textures.
37-40: Approved: Appropriate getter implementation.The getter provides a clean API for checking if a texture is in sRGB color space, maintaining proper encapsulation.
packages/loader/src/gltf/extensions/KHR_materials_pbrSpecularGlossiness.ts (2)
31-31:✅ Verification successful
Ensure consistent sRGB handling for diffuse texture.
The
trueparameter indicates that this texture should be loaded with sRGB color space. This aligns with the PR objective to support sRGB color space.
🏁 Script executed:
#!/bin/bash # Check if other texture loads for diffuse textures consistently use the sRGB parameter rg -B 2 -A 2 "context\.get.*Texture.*diffuseTexture" --type tsLength of output: 723
sRGB Texture Loading Verified
The code in
packages/loader/src/gltf/extensions/KHR_materials_pbrSpecularGlossiness.tscorrectly loads the diffuse texture with the sRGB parameter (by passingtrue), which aligns with the PR’s objective. The verification output confirms that this approach is consistently applied within this context. No additional changes are necessary.
52-52:✅ Verification successful
Ensure consistent sRGB handling for specular glossiness texture.
The
trueparameter indicates that this texture should be loaded with sRGB color space, properly implementing the sRGB support feature.
🏁 Script executed:
#!/bin/bash # Check if other texture loads for specular textures consistently use the sRGB parameter rg -B 2 -A 2 "context\.get.*Texture.*specularGlossinessTexture" --type tsLength of output: 731
Approval: sRGB Handling Confirmed for Specular Glossiness Texture
The executed search confirms that the specular glossiness texture is loaded with the sRGB option enabled via the
trueparameter. The code inpackages/loader/src/gltf/extensions/KHR_materials_pbrSpecularGlossiness.tsat line 52 is correct and consistent with the intended sRGB handling.
- The
context.get<Texture2D>(GLTFParserType.Texture, specularGlossinessTexture.index, true)call correctly enforces sRGB loading.No changes are required.
packages/core/src/Engine.ts (2)
55-55: Approved: Macro for handling absence of sRGB support.Adding this static shader macro allows the engine to conditionally compile shader code based on sRGB capability support.
261-263: Approved: Proper fallback when sRGB isn't supported.This code correctly enables the fallback macro when the hardware doesn't support sRGB, allowing the engine to gracefully degrade functionality.
packages/core/src/base/Constant.ts (2)
121-121: Approved: Enabling s3tc_srgb compressed texture format support.Uncommenting this line adds support for the S3TC compressed texture format with sRGB encoding, complementing the overall sRGB feature implementation.
127-127: Approved: Added core sRGB extension capability.This addition enables the engine to detect support for the EXT_sRGB WebGL extension, which is essential for proper sRGB texture handling.
packages/loader/src/SpriteAtlasLoader.ts (1)
55-55: Added sRGB color space support for sprite atlas texturesThe addition of
isSRGBColorSpace: trueparameter ensures that sprite atlas textures are loaded in sRGB color space, which will enhance color rendering accuracy for sprites. This aligns with the PR's objective to support sRGB color space throughout the engine.packages/core/src/2d/text/SubFont.ts (1)
96-96:✅ Verification successful
Enabled sRGB color space for font texture atlas
The modification adds support for sRGB color space in font texture atlases, which will ensure text renders with proper gamma correction. This change is important as text colors can appear different in linear vs. sRGB color spaces.
🏁 Script executed:
#!/bin/bash # Checking if there are other Texture2D instances elsewhere in the codebase that might need updating with sRGB parameter # Look for Texture2D instantiations without the sRGB parameter rg "new Texture2D\(.*\)" --type ts | grep -v "true\s*)" | grep -v "false\s*)"Length of output: 6292
sRGB Enabled for Font Texture Atlas in SubFont.ts
The change in
packages/core/src/2d/text/SubFont.tsnow correctly passestrueas the sRGB flag:const texture = new Texture2D(engine, 256, 256, TextureFormat.R8G8B8A8, false, undefined, true);This ensures that font textures render with proper gamma correction. A repository-wide search shows that other
Texture2Dinstantiations either:
- Rely on dynamic evaluation of the sRGB flag (e.g., via
isSRGBor!!isSRGBColorSpace), or- Intentionally omit an explicit sRGB parameter (thus defaulting as needed).
Please verify that this selective enablement is intentional and that only font textures require an explicit sRGB override.
packages/loader/src/gltf/extensions/KHR_materials_sheen.ts (1)
28-28: Added sRGB color space support for sheen color texturesThe additional boolean parameter
trueensures that sheen color textures are loaded in sRGB color space. This is consistent with other texture loading changes in this PR.Note that on line 36, the sheenRoughnessTexture is still loaded without the sRGB parameter, which is correct since roughness textures contain non-color data that should remain in linear space.
packages/rhi-webgl/src/type.ts (1)
112-116:✅ Verification successful
Added WebGL sRGB compressed texture format support
Added support for sRGB variants of S3TC compressed texture formats, enabling proper color space handling for S3TC compressed textures. This is an important addition for maintaining color accuracy when using these compressed formats.
🏁 Script executed:
#!/bin/bash # Checking if all S3TC formats are properly handled in the engine's format detection code # Look for places where the S3TC texture formats are used rg "S3TC_DXT" --type tsLength of output: 1734
S3TC sRGB Support Verified and Integrated
The added sRGB variants in
packages/rhi-webgl/src/type.ts(lines 112-116) are now properly referenced across the codebase. We confirmed inGLTexture.tsand related files that the new sRGB formats (e.g.,SRGB_S3TC_DXT1_EXT,SRGB_ALPHA_S3TC_DXT1_EXT, etc.) are correctly handled in the engine's format detection. This ensures proper color space management when using S3TC textures.
- Verified Locations:
packages/rhi-webgl/src/GLTexture.ts– Usage in conditional format selection confirms proper integration.packages/loader/src/compressed-texture/KhronosTextureContainer.ts– Cases demonstrate consistency with other S3TC formats.The changes are approved as added support functions correctly.
packages/loader/src/gltf/GLTFUtils.ts (1)
353-353: Enhanced parameter type supports more inputs.The parameter type for
imageBufferhas been expanded from justArrayBuffertoArrayBuffer | TypedArray, allowing the method to accept bothArrayBufferinstances and anyTypedArraysuch asUint8ArrayorFloat32Array. This change increases flexibility while maintaining compatibility with existing code.packages/rhi-webgl/src/GLRenderTarget.ts (2)
49-49: Destructuring for better property access.Changed from directly accessing
_formatto destructuring bothformatandisSRGBColorSpacefrom the color texture object. This provides cleaner access to the properties needed for validation.
53-55: Added important sRGB format validation.This validation check ensures that when using sRGB color space, only the R8G8B8A8 format (with alpha channel) is supported. The R8G8B8 format (without alpha) is explicitly disallowed, which is an important constraint for correct sRGB rendering.
This constraint is necessary because sRGB conversion typically requires specific pixel formats, and the WebGL implementation might only support sRGB with formats that include an alpha channel.
packages/rhi-webgl/src/GLCapability.ts (3)
170-170: Added new sRGB-related capability types.Added
s3tc_srgbandsRGBto the GLCapabilityType list, which enables the engine to detect and track support for sRGB texture formats.Also applies to: 182-182
214-217: Added capability detection for sRGB support.Implemented capability detection for sRGB-related features:
s3tc_srgb: Checks for the S3TC sRGB extensionsRGB: Checks for WebGL2 support or the sRGB extensionThis properly detects whether the browser and GPU support sRGB textures, which is essential for the feature implementation.
306-309: Added WebGL compatibility mapping for sRGB formats.Implemented compatibility handling for sRGB constants:
- Maps
SRGB8toSRGB_EXT- Maps
SRGB8_ALPHA8toSRGB_ALPHA_EXTThis ensures proper WebGL constant mapping for sRGB formats when using the extension in WebGL1 contexts.
packages/loader/src/gltf/parser/GLTFParser.ts (1)
133-133: Enhanced method signature for flexibility.The abstract
parsemethod has been modified to accept rest parameters (...extra), allowing it to handle additional arguments. This change enables passing sRGB-related information through the parsing pipeline while maintaining backward compatibility.This modification aligns with other methods in the class that already use rest parameters for flexibility, creating a consistent API pattern throughout the parser system.
packages/loader/src/gltf/extensions/EXT_texture_webp.ts (2)
33-34: Parameter added for sRGB color space support.The addition of the
isSRGBColorSpaceparameter to the method signature is consistent with the PR objective to support sRGB color space in textures.
43-44: Successfully propagated sRGB parameter to texture parser.The
isSRGBColorSpaceparameter is correctly passed to the_parseTexturemethod, ensuring that WebP textures can be properly processed with sRGB color space considerations.packages/core/src/texture/TextureCube.ts (3)
19-19: Documentation updated for the new sRGB parameter.The JSDoc comment has been properly updated to include the description of the new parameter, which helps with API documentation.
21-21: Constructor signature updated to support sRGB color space.The constructor now accepts an optional
isSRGBColorSpaceparameter with a default value offalse, which aligns with the PR objective to support sRGB color space.
29-29: Class instance updated to store sRGB color space setting.The
_isSRGBColorSpaceproperty is now properly initialized with the provided parameter value, enabling the texture to retain its color space information.packages/core/src/texture/Texture2DArray.ts (3)
29-29: Documentation updated for the new sRGB parameter.The JSDoc comment has been properly updated to include the description of the new parameter, which helps with API documentation.
37-38: Constructor signature updated to support sRGB color space.The constructor now accepts an optional
isSRGBColorSpaceparameter with a default value offalse, which aligns with the PR objective to support sRGB color space.
47-47: Class instance updated to store sRGB color space setting.The
_isSRGBColorSpaceproperty is now properly initialized with the provided parameter value, enabling the texture to retain its color space information.packages/rhi-webgl/src/GLTexture2DArray.ts (3)
12-12: New parameter for sRGB color space supportThe addition of
isSRGBColorSpacein the destructuring assignment is necessary to support sRGB color space in texture arrays, consistent with the PR objectives.
18-18: Improved validation logicMoving the texture validation to a dedicated
_validatemethod is a good architectural improvement that centralizes validation logic and makes the code more maintainable.
21-21: Format details now account for sRGB color spaceThe updated call to
_getFormatDetailnow includes theisSRGBColorSpaceparameter, ensuring proper texture format handling for sRGB textures.packages/core/src/texture/enums/TextureFormat.ts (1)
66-66:❓ Verification inconclusive
Strategic reordering of texture format values
The reordering of enum values (moving
Stencilfrom 28 to 34 and adjusting other depth-related formats) appears to be intentional to maintain logical grouping of related formats while accommodating the sRGB support.However, changing enum values in a public API could potentially cause compatibility issues if external code relies on the specific numeric values.
Also applies to: 68-72, 74-78
🏁 Script executed:
#!/bin/bash # Check for hardcoded uses of these texture format enum values echo "Searching for hardcoded enum values that might need updating..." rg -A1 -B1 "= (28|29|30|31|32|33|34)" --type tsLength of output: 10720
Action required: Confirm external API consumers reference enum names, not literal numbers
The strategic reordering in
TextureFormat.ts(and similarly inRenderBufferDepthFormat.ts) now groups depth-related formats logically—with the updated mapping:
DepthStencil = 28Depth16 = 29Depth24 = 30Depth32 = 31Depth24Stencil8 = 32Depth32Stencil8 = 33Stencil = 34A grep of hardcoded numeric values (28–34) did not flag any obvious internal issues. However, because these enums form part of our public API, please verify that any external consumers rely on the enum identifiers rather than the numeric literals to avoid compatibility issues.
packages/core/src/texture/Texture2D.ts (3)
21-21: Added sRGB color space supportThe addition of the
isSRGBColorSpaceparameter with clear documentation and proper initialization is well-implemented. Setting the default tofalsemaintains backward compatibility with existing code.Also applies to: 30-30, 39-39
27-29: Simplified default parameter declarationsRemoving explicit type annotations for default parameters improves code readability while maintaining type safety through TypeScript's type inference.
41-41: Streamlined depth texture detectionThe consolidated logic for determining if a texture is a depth texture simplifies the code while maintaining the same functionality. This makes the code more readable and maintainable.
packages/loader/src/Texture2DLoader.ts (2)
30-31: Added sRGB color space support to texture loaderThe inclusion of
isSRGBColorSpacein the parameter destructuring ensures that the loader can properly handle sRGB textures based on user configuration.
82-83: Updated interface documentationThe
Texture2DParamsinterface has been updated to include the newisSRGBColorSpaceproperty with appropriate documentation, allowing users to specify their preference for sRGB color space when loading textures.packages/rhi-webgl/src/GLTextureCube.ts (3)
15-15: Added support for sRGB color space in GLTextureCube.The constructor now extracts
isSRGBColorSpacefrom the textureCube object, which is a key addition for implementing sRGB color space support throughout the engine.
18-18: Improved validation approach with dedicated method.Replacing direct validation logic with a centralized
_validatemethod is good practice for better code organization. This makes the code more maintainable by isolating validation logic.
20-20: Updated format detail retrieval to consider color space.The
_getFormatDetailmethod now receives theisSRGBColorSpaceparameter, which is essential for correctly handling texture format details based on the color space.packages/loader/src/gltf/parser/GLTFMaterialParser.ts (3)
67-67: Correctly marked baseColorTexture as using sRGB color space.Adding the
trueparameter to indicate sRGB color space for the baseColorTexture is appropriate, as color textures should typically use sRGB encoding for correct display.
95-95: Correctly marked emissiveTexture as using sRGB color space.The emissive texture should also use sRGB color space since it contains color information, making this change appropriate.
84-86:✅ Verification successful
Verify if other texture types need sRGB color space support.
While baseColor and emissive textures are correctly marked as sRGB, it's important to verify if other texture types like roughnessMetallic, normal, and occlusion should remain in linear color space.
Also applies to: 112-114, 125-127
🏁 Script executed:
#!/bin/bash # Check if normal, roughnessMetallic, and occlusion textures are correctly using linear color space # Let's look at the PBR material implementation to confirm texture color space conventions rg -A 5 "roughnessMetallic|normalTexture|occlusionTexture" --glob "*.ts" | grep -v "test" # Check for any shader code handling these textures rg "texture.*[Nn]ormal|texture.*[Oo]cclusion|texture.*[Rr]oughness|texture.*[Mm]etallic" --glob "*.glsl" --glob "*.vs" --glob "*.fs"Length of output: 24360
Action: Confirm Texture Color Space Intent
After verifying the codebase and shader logic, it’s clear that the current handling is correct. BaseColor and emissive textures are intentionally marked as sRGB, while roughnessMetallic, normal, and occlusion textures remain in linear space as required by the PBR pipeline. There is no evidence suggesting that these non-base textures need sRGB conversion.
- GLTFMaterialParser.ts (Lines 84-86, 112-114, 125-127): These blocks load textures without any sRGB conversion, which matches the PBR requirements.
- Shader Code: The shaders explicitly use linear samples for normal, roughnessMetallic, and occlusion textures.
No changes are needed based on our verification.
packages/loader/src/resource-deserialize/resources/texture2D/TextureDecoder.ts (3)
19-19: Added sRGB color space flag to texture serialization format.Reading the
isSRGBColorSpacevalue from the buffer reader enables proper serialization and deserialization of this important texture property.
24-24: Updated Texture2D constructor to include sRGB parameter.The Texture2D constructor now correctly receives the sRGB color space flag, ensuring that textures are created with the appropriate color space setting.
19-24:❓ Verification inconclusive
Verify backwards compatibility for texture deserialization.
Since a new field has been added to the texture serialization format, verify that older serialized textures (without the sRGB flag) can still be loaded correctly.
🌐 Web query:
What is the best practice for handling backward compatibility when adding new fields to a serialization format?💡 Result:
To ensure backward compatibility when adding new fields to a serialization format, follow these best practices informed by industry standards and schema evolution principles:
Use Schema-Compatible Field Addition
- Add new fields with unique identifiers
Assign new, unused field numbers (e.g., Protobuf tags) to avoid collisions with existing fields[1][4]. These identifiers are critical for maintaining wire compatibility.- Provide default values
New fields should include default values (e.g.,"default": "green"in Avro) to ensure older data can be deserialized by newer systems without errors[3][4].- Avoid structural changes
Do not alter existing fields' types, tag numbers, oroneOfgroupings, as these changes break wire compatibility[1].Leverage Schema Evolution Features
- Adopt backward compatibility modes
Tools like Confluent Schema Registry enforce compatibility rules (e.g.,BACKWARD,FULL), allowing new schemas to read older data by ignoring unrecognized fields or applying defaults[3][4].- Deprecate, don’t delete
Mark obsolete fields as deprecated rather than removing them, and reserve their identifiers to prevent accidental reuse[1][4].Structural and Semantic Considerations
- Ensure semantic consistency
New fields should not alter the interpretation of existing data (e.g., changing a field’s meaning from "user ID" to "account ID")[1].- Test replay compatibility
Validate schema changes using replay tests to confirm older workflow executions remain functional[6].Example Implementation (Avro/Protobuf)
{ "namespace": "example.avro", "type": "record", "name": "user", "fields": [ {"name": "name", "type": "string"}, {"name": "favorite_number", "type": "int"}, {"name": "favorite_color", "type": "string", "default": "green"} // New field with default ] }In this Avro schema, adding
favorite_colorwith a default ensures backward compatibility. Older data missing this field will use "green" during deserialization[3].Tooling and Testing
- Use schema registries for automated compatibility checks (e.g., Confluent Schema Registry’s
BACKWARDmode)[3].- Avoid required fields when possible, as they cannot be safely removed or modified[4].
By prioritizing these strategies, systems can evolve schemas while maintaining interoperability with existing data and clients.
Citations:
- 1: https://docs.spacetime.aalyria.com/dev-guides/backward-compatibility/
- 2: https://www.vbforums.com/showthread.php?254115-Serialization-backward-compatibility
- 3: https://docs.confluent.io/platform/current/schema-registry/fundamentals/schema-evolution.html
- 4: https://earthly.dev/blog/backward-and-forward-compatibility/
- 5: https://en.wikipedia.org/wiki/Comparison_of_data-serialization_formats
- 6: https://community.temporal.io/t/temporal-activity-schema-change-backwards-compatibility/3297
- 7: Questions: Versioning and forward/backward compatibility msgpack/msgpack#142
- 8: Backwards compatibility FasterXML/jackson#122
- 9: https://docs.unity3d.com/6000.0/Documentation/Manual/script-serialization-best-practices.html
Ensure backward compatibility for texture deserialization.
Please verify that older texture files—those without the new sRGB flag—are still deserialized correctly. In particular, confirm that:
- When deserializing legacy textures, the absence of the sRGB flag correctly triggers a default value (e.g., treating the value as false).
- The conversion using
!!isSRGBColorSpacereliably sets the flag when the field is missing, or that theBufferReaderprovides a safe fallback.- Adequate tests exist (or are added) to simulate loading older textures to ensure no regressions in texture rendering occur.
packages/loader/src/gltf/parser/GLTFParserContext.ts (2)
60-62: Extended GLTFParserContext.get method to support variable parameters.The method signature has been updated to accept additional parameters using the rest operator (
...extra), which is essential for passing the sRGB color space flag to texture parsers.
87-87: Properly forwarded additional parameters to nested calls.The implementation correctly forwards the extra parameters to both recursive
getcalls andparsemethod calls, ensuring consistent parameter propagation throughout the parsing pipeline.Also applies to: 89-89, 96-96
packages/rhi-webgl/src/GLTexture.ts (5)
29-34: Constructor parameterisSRGBColorSpaceadded to support sRGB color spaceThe
_getFormatDetailmethod now accepts anisSRGBColorSpaceparameter, which will allow the engine to correctly handle textures in sRGB color space. This is a key part of implementing the sRGB color space support mentioned in the PR objectives.
38-46: R8G8B8 and R8G8B8A8 formats now support sRGB color spaceThe implementation correctly handles sRGB variants for RGB and RGBA formats, using the appropriate
SRGB8andSRGB8_ALPHA8internal formats whenisSRGBColorSpaceis true. This provides essential support for commonly used texture formats in sRGB color space.
115-117: Compressed texture formats now support sRGB color spaceThe implementation adds support for sRGB variants of compressed texture formats (BC1, BC3, BC7, ETC2_RGB, ETC2_RGBA8, ASTC_4x4), which is crucial for maintaining color accuracy across different compression formats.
Also applies to: 122-124, 129-131, 141-143, 153-155, 180-182
668-674: Refactored frame buffer retrievalThe implementation of
_getReadFrameBufferhas been simplified to ensure a framebuffer is created only when needed, which is a clean optimization.
676-712: Added texture validation with sRGB supportThe new
_validatemethod provides robust validation for textures, including:
- Checking if the texture format is supported
- Warning about non-power-of-2 textures with mipmaps in WebGL1
- Special handling for sRGB textures with mipmaps
This improves error handling and provides clear warnings when certain combinations of texture settings are not supported.
However, the warning message on line 705 has a specific constraint that could use clarification:
- "Auto-generating mipmaps for sRGB textures is only supported in [WebGL2 + R8G8B8A8], and has automatically downgraded to non-mipmap" + "Auto-generating mipmaps for sRGB textures is only supported in WebGL2 with R8G8B8A8 format, and has automatically downgraded to non-mipmap"packages/loader/src/ktx2/KTX2Loader.ts (4)
80-86: Added ktx2Container to returned data structureThe
_parseBuffermethod now includes thektx2Containerin its return value, which allows downstream consumers to access important metadata like the sRGB flag.
92-98: Added ktx2Container parameter to texture creation methodThe
_createTextureByBuffermethod signature has been updated to accept and use thektx2Containerdirectly, extracting theisSRGBflag from it. This is a cleaner approach than passing multiple individual parameters.
106-107: Added sRGB color space support to texture creationThe texture creation now passes the
isSRGBflag to bothTexture2DandTextureCubeconstructors, ensuring textures created from KTX2 files correctly preserve their color space information.Also applies to: 112-113
237-238: Updated parameter passing in load methodThe
loadmethod now correctly unpacks and passes thektx2Containerto_createTextureByBuffer, which aligns with the updated method signature.packages/rhi-webgl/src/GLTexture2D.ts (4)
1-1: Updated imports to remove unused dependenciesThe imports have been cleaned up to remove
LoggerandTextureFormatwhich are no longer directly used in this file, improving code cleanliness.
15-15: Added isSRGBColorSpace property extractionThe constructor now extracts the
isSRGBColorSpaceproperty from the texture, which is needed for proper texture format handling with sRGB support.
18-18: Added texture validationThe constructor now calls the new
_validatemethod from the parent class, which centralizes validation logic for all texture types, making the code more maintainable and consistent.
20-20: Updated format detail retrieval with sRGB supportThe call to
_getFormatDetailnow includes theisSRGBColorSpaceparameter, ensuring the correct format details are retrieved based on the texture's color space.packages/loader/src/gltf/parser/GLTFTextureParser.ts (5)
23-24: Added isSRGBColorSpace parameter to _parseTexture methodThe
_parseTexturemethod now accepts an optionalisSRGBColorSpaceparameter, allowing glTF textures to be loaded with the correct color space.
43-45: Added isSRGBColorSpace to texture loading parametersThe texture loading parameters now include the
isSRGBColorSpaceflag, ensuring external texture loaders respect the desired color space.
63-71: Added isSRGBColorSpace to Texture2D constructorWhen creating a new texture from an image buffer, the
isSRGBColorSpaceparameter is now passed to theTexture2Dconstructor, ensuring the texture is created with the correct color space.
89-89: Updated parse method to require isSRGBColorSpace parameterThe
parsemethod now requires theisSRGBColorSpaceparameter, which is a breaking change but necessary to ensure all glTF textures are created with the correct color space information.
95-96: Propagated isSRGBColorSpace to extension parsing and texture creationThe
isSRGBColorSpaceparameter is correctly passed to bothGLTFParser.executeExtensionsCreateAndParseand_parseTexture, ensuring consistent color space handling throughout the glTF parsing process.Also applies to: 99-106
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
e2e/config.ts (1)
139-145: Add a short doc commentIt's helpful to add a quick note describing the purpose of the new "Texture.sRGB" test for future maintainers. This clarifies why sRGB testing is added here.
packages/loader/src/ktx2/KTX2Container.ts (1)
52-52: Consider null-safetyIf
this.dataFormatDescriptormight be undefined in edge cases, consider a null check to prevent runtime exceptions.tests/src/loader/KTX2Loader.test.ts (1)
86-88: Fix minor typo in commentThe comment reads "pbrtc" instead of "pvrtc." Consider:
- // pbrtc don't support sRGB, should downgrade to R8G8B8A8 + // PVRTC doesn't support sRGB, should downgrade to R8G8B8A8packages/loader/src/ktx2/KTX2Loader.ts (3)
92-98: Fallback for missing “GalaceanTextureParams”Consider defaulting to safe wrap/filter modes if
GalaceanTextureParamsis not present inkeyValue.
147-148: Eliminate ‘this’ in static scopeUse
KTX2Loader._detectSupportedFormat(...)instead ofthis._detectSupportedFormat(...):- const targetFormat = this._detectSupportedFormat(renderer, priorityFormats, isSRGB); + const targetFormat = KTX2Loader._detectSupportedFormat(renderer, priorityFormats, isSRGB);🧰 Tools
🪛 Biome (1.9.4)
[error] 148-148: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
165-168: Avoid using ‘this’ in static methodReplace
this._supportedMapwithKTX2Loader._supportedMapto clarify references:- const capabilities = this._supportedMap[format]?.[isSRGB ? DFDTransferFunction.sRGB : DFDTransferFunction.linear]; + const capabilities = KTX2Loader._supportedMap[format]?.[isSRGB ? DFDTransferFunction.sRGB : DFDTransferFunction.linear];Also applies to: 172-172
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (7)
e2e/fixtures/originImage/Advance_project-loader.jpgis excluded by!**/*.jpge2e/fixtures/originImage/Animator_animator-blendShape-quantization.jpgis excluded by!**/*.jpge2e/fixtures/originImage/Animator_animator-blendShape.jpgis excluded by!**/*.jpge2e/fixtures/originImage/Material_material-blendMode.jpgis excluded by!**/*.jpge2e/fixtures/originImage/Texture_texture-sRGB-KTX2.jpgis excluded by!**/*.jpgpackages/core/src/postProcess/shaders/UberPost.glslis excluded by!**/*.glslpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (22)
e2e/case/.initPostProcessEnv.ts(1 hunks)e2e/case/animator-blendShape.ts(1 hunks)e2e/case/material-blinn-phong.ts(1 hunks)e2e/case/particleRenderer-dream.ts(1 hunks)e2e/case/particleRenderer-textureSheetAnimation.ts(1 hunks)e2e/case/primitive-capsule.ts(1 hunks)e2e/case/primitive-cone.ts(1 hunks)e2e/case/primitive-cuboid.ts(1 hunks)e2e/case/primitive-cylinder.ts(1 hunks)e2e/case/primitive-plane.ts(1 hunks)e2e/case/primitive-sphere.ts(1 hunks)e2e/case/primitive-torus.ts(1 hunks)e2e/case/project-loader.ts(2 hunks)e2e/case/spriteMask-customStencil.ts(1 hunks)e2e/case/texture-sRGB-KTX2.ts(1 hunks)e2e/config.ts(1 hunks)e2e/package.json(1 hunks)packages/loader/src/gltf/parser/GLTFParserContext.ts(2 hunks)packages/loader/src/gltf/parser/GLTFTextureParser.ts(6 hunks)packages/loader/src/ktx2/KTX2Container.ts(1 hunks)packages/loader/src/ktx2/KTX2Loader.ts(6 hunks)tests/src/loader/KTX2Loader.test.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- e2e/case/animator-blendShape.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/loader/src/gltf/parser/GLTFParserContext.ts
- packages/loader/src/gltf/parser/GLTFTextureParser.ts
🧰 Additional context used
🪛 Biome (1.9.4)
packages/loader/src/ktx2/KTX2Loader.ts
[error] 115-115: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 148-148: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 172-172: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build (22.x, windows-latest)
🔇 Additional comments (26)
packages/loader/src/ktx2/KTX2Container.ts (1)
4-4: Enum export clarityExporting
DFDTransferFunctionincreases flexibility for usage in other modules. Looks good!tests/src/loader/KTX2Loader.test.ts (1)
93-95: Verify sRGB fallbackWhen
s3tc_srgbis reported, the code transcodes to BC1. Please confirm there's no separate BC1_sRGB variant needed in the engine. Do you want a script to check code references for BC1_sRGB usage?Also applies to: 101-102
packages/loader/src/ktx2/KTX2Loader.ts (3)
18-18: New import for sRGB handlingImporting
DFDTransferFunctionandKTX2Containeris appropriate for extended sRGB support.
46-63: Robust sRGB coverageExpanding
_supportedMapto include sRGB variants is well done, ensuring correct handling across formats.
103-104: Consistent isSRGB usagePassing
isSRGBtoTexture2D/TextureCubeconstructors is a solid approach. The code aligns with newly introduced sRGB support.Also applies to: 110-110, 118-118, 124-124
e2e/package.json (1)
14-14: New dependency added for shader support.This addition of
@galacean/engine-shader-shaderlabaligns with the PR objective to support sRGB color space, likely providing the shader implementations needed for GPU-based gamma to linear conversion.e2e/case/.initPostProcessEnv.ts (1)
57-60:❓ Verification inconclusive
Added sRGB color space support for texture loading.
The
isSRGBColorSpace: trueparameter has been correctly added to enable sRGB color space support for this texture, aligning with the PR requirements.However, I notice that the ambient light texture loaded in lines 39-54 doesn't have this parameter. Is this intentional, or should that texture also use sRGB color space?
🏁 Script executed:
#!/bin/bash # Check if other texture loading calls in the codebase have been updated with isSRGBColorSpace rg -A 5 -B 5 "ambientLight.*specularTexture" --type tsLength of output: 14459
Review Note: Verify Consistency of sRGB Support for Ambient Light Textures
I’ve confirmed that the PR correctly adds the
isSRGBColorSpace: trueparameter for the updated texture (see the MDN URL snippet). However, as noted, the ambient light texture loading (lines 39–54 ine2e/case/.initPostProcessEnv.tsand similar usage elsewhere) still only usestextureDecodeRGBMwithout any sRGB parameter. Could you please verify whether the ambient light texture is intentionally left without sRGB support, or if it too should be updated to includeisSRGBColorSpace: true? If the omission is deliberate, a brief inline note explaining the rationale would be helpful.e2e/case/spriteMask-customStencil.ts (1)
42-45:❓ Verification inconclusive
Added sRGB color space support for sprite texture.
The
isSRGBColorSpace: trueparameter has been correctly added to the sprite texture loading, which is in line with the PR objective to support sRGB color space for sprite colors.However, I notice that the mask texture loaded at lines 48-51 doesn't have this parameter. Is this intentional? Different handling for sprite textures vs. mask textures?
🏁 Script executed:
#!/bin/bash # Check if mask textures require sRGB color space in other parts of the codebase rg -A 3 "mask.*texture.*params" --type tsLength of output: 43
Action Required: Verify Mask Texture sRGB Handling
- The sprite texture now correctly includes
isSRGBColorSpace: trueas intended.- However, the mask texture segment (lines 48–51 in
e2e/case/spriteMask-customStencil.ts) does not include the sRGB parameter.- Our search for similar usage in the codebase (using
rg) returned no instances where mask textures are configured with sRGB support. This absence suggests that the handling may be intentionally different for mask textures—but it also warrants a manual review to ensure this is the intended behavior.Please verify if the omission of the sRGB parameter for mask textures is deliberate or if the feature should be added consistently.
e2e/case/primitive-plane.ts (1)
41-44: Added sRGB color space support for texture loading.The
isSRGBColorSpace: trueparameter has been correctly added to enable sRGB color space for this texture. This is consistent with the PR requirement to specify this parameter for proper color handling.e2e/case/primitive-cuboid.ts (1)
41-44: Correctly configured texture with sRGB color spaceThe addition of
paramswithisSRGBColorSpace: truecorrectly implements sRGB color space support for this texture, aligning with the PR objectives.e2e/case/primitive-capsule.ts (1)
41-44: Consistent sRGB configuration appliedThe addition of
paramswithisSRGBColorSpace: trueis consistent with other primitive examples and correctly implements sRGB color space support.e2e/case/primitive-torus.ts (1)
41-44: Consistent sRGB configuration appliedThe addition of
paramswithisSRGBColorSpace: trueis consistent with other primitive examples and correctly implements sRGB color space support.e2e/case/material-blinn-phong.ts (1)
43-46: sRGB color space applied to base texture onlyThe
isSRGBColorSpace: trueparameter has been added correctly to the base texture, but intentionally not to the normal texture (which is appropriate as normal maps represent directional data, not visual colors).Could you add documentation about which texture types should use sRGB color space and which shouldn't? This would help users understand when to use this parameter, especially since this is mentioned as a breaking change in the PR.
e2e/case/primitive-sphere.ts (1)
41-44: Correctly implemented sRGB color space configuration.The addition of the
paramsobject withisSRGBColorSpace: trueensures the texture is loaded with sRGB color space support, which aligns with the PR objectives. This will affect the texture rendering by properly handling gamma correction for this texture.e2e/case/primitive-cylinder.ts (1)
41-44: Correctly implemented sRGB color space configuration.The addition of the
paramsobject withisSRGBColorSpace: trueensures the texture is loaded with sRGB color space support, which aligns with the PR objectives. This is consistent with the implementation in other primitive examples.e2e/case/particleRenderer-dream.ts (5)
50-53: Correctly implemented sRGB color space configuration for texture loading.The addition of the
paramsobject withisSRGBColorSpace: trueensures the texture is loaded with sRGB color space support, which aligns with the PR objectives.
57-60: Correctly implemented sRGB color space configuration for texture loading.The addition of the
paramsobject withisSRGBColorSpace: trueis consistently applied to this texture asset.
64-67: Correctly implemented sRGB color space configuration for texture loading.The addition of the
paramsobject withisSRGBColorSpace: trueis consistently applied to this texture asset.
71-74: Correctly implemented sRGB color space configuration for texture loading.The addition of the
paramsobject withisSRGBColorSpace: trueis consistently applied to this texture asset.
50-74: Verify visual consistency in the particle rendering.All textures in the particle system are now using sRGB color space, which could affect their visual appearance. Since the PR mentions breaking changes affecting UI output, ensure that the visual appearance of these particle effects remains as expected.
e2e/case/primitive-cone.ts (1)
41-44: Correctly implemented sRGB color space configuration.The addition of the
paramsobject withisSRGBColorSpace: trueensures the texture is loaded with sRGB color space support, which aligns with the PR objectives. This is consistent with the implementation in other primitive examples.e2e/case/particleRenderer-textureSheetAnimation.ts (1)
48-51: Added support for sRGB color space in texture loadingThe
isSRGBColorSpace: trueparameter has been added to the texture loading configuration, which aligns with the PR objective of adding support for sRGB color space. This ensures the texture is processed with the correct color space during rendering.e2e/case/texture-sRGB-KTX2.ts (1)
29-31: Potential missing sRGB parameter for KTX2 texture loadingThis file demonstrates KTX2 textures but doesn't specify
isSRGBColorSpacein the params object, unlike other texture loading examples in this PR. According to the PR objectives, users must specifyisSRGBColorSpacein the parameters of the texture loader.Please verify if KTX2 textures need the
isSRGBColorSpaceparameter or if they handle sRGB differently. If they do need it, consider adding:url: "https://mdn.alipayobjects.com/oasis_be/afts/img/A*XmHPTYGREAIAAAAAAAAAAAAAekp5AQ/original/original.ktx2", - type: AssetType.KTX2 + type: AssetType.KTX2, + params: { + isSRGBColorSpace: true + }e2e/case/project-loader.ts (3)
7-7: Updated import source for registerIncludesThe import for
registerIncludeshas been changed from@galacean/engine-toolkitto@galacean/engine-shader-shaderlab, suggesting a reorganization of code modules.
20-20: Updated project URLThe URL for loading the project has been updated, likely to a newer version that incorporates sRGB textures or other changes related to this PR.
25-26: Repositioned updateForE2E callThe
updateForE2E(engine)call has been moved to after retrieving the camera component, which may improve the testing sequence.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/rhi-webgl/src/GLTexture.ts (1)
679-715: Comprehensive texture validation with fallback logic.This
_validate()method thoroughly checks format capabilities, downgrades mipmap usage for non-power-of-two or unsupported sRGB auto-generation scenarios, and throws errors where appropriate. Overall, it looks robust.
- Potential extension checks: For sRGB usage in WebGL1, consider verifying the
EXT_sRGBextension here as well for completeness.- Refactoring: You could unify these fallback checks with
_supportTextureFormat()/_supportRenderBufferDepthFormat()for more cohesive validation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/loader/src/gltf/GLTFUtils.ts(1 hunks)packages/rhi-webgl/src/GLTexture.ts(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/loader/src/gltf/GLTFUtils.ts
🧰 Additional context used
🧬 Code Definitions (1)
packages/rhi-webgl/src/GLTexture.ts (4)
packages/core/src/texture/Texture.ts (2)
isSRGBColorSpace(41-43)format(48-50)packages/rhi-webgl/src/WebGLGraphicDevice.ts (3)
gl(124-126)isWebGL2(112-114)WebGLGraphicDevice(77-569)packages/rhi-webgl/src/type.ts (1)
TextureFormatDetail(46-54)packages/rhi-webgl/src/GLCapability.ts (1)
rhi(75-77)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: e2e (22.x)
🔇 Additional comments (8)
packages/rhi-webgl/src/GLTexture.ts (8)
31-31: Good introduction of theisSRGBColorSpaceparameter.This makes the sRGB logic explicit and aligns with the PR's goal to support sRGB color space.
645-645: Nice adoption ofreadFormatin pixel reads.Extracting
readFormatfrom_formatDetailensures more accurate readbacks, especially for sRGB or compressed textures. This is a clean approach and improves maintainability.
670-677: Efficient use of a single read framebuffer.Creating (or reusing) a single
_readFrameBufferat runtime avoids unnecessary framebuffer objects, which is a good optimization. Ensure that the framebuffer is properly cleaned up if needed (for instance, on context loss).
666-666:✅ Verification successful
Use of
readFormat ?? baseFormatingl.readPixelsis correct.Falling back to
baseFormatifreadFormatis undefined is a safe measure. Verify that no further adjustments are required for specialized formats or extensions.
🏁 Script executed:
#!/bin/bash # Description: Search for references to readPixels usage in the code to ensure consistent fallback usage. rg -A4 "readPixels"Length of output: 2859
Verification: Fallback Logic is Consistent and Correct
The use of
readFormat ?? baseFormatingl.readPixelsin packages/rhi-webgl/src/GLTexture.ts is consistent with the overall approach used elsewhere in the codebase. Our search confirms that while other modules like GLTexture2DArray.ts directly usebaseFormat, relying onreadFormatwhen specified and falling back tobaseFormatotherwise provides a safe and uniform mechanism. No additional adjustments appear necessary for specialized formats or extensions at this time.
38-40:❓ Verification inconclusive
Ensure compatibility with sRGB in WebGL1.
When
isSRGBColorSpaceis true under WebGL1, the code setsgl.SRGB8. However, sRGB texture support in WebGL1 typically requires an extension (EXT_sRGB). Please confirm that an appropriate extension check is implemented or provide a fallback.
🏁 Script executed:
#!/bin/bash # Description: Verify presence of EXT_sRGB or any fallback logic in the codebase. rg -A6 "EXT_sRGB|sRGB.*extension"Length of output: 1468
Verify sRGB Extension Handling in WebGL1 Context
We noticed that in
packages/rhi-webgl/src/GLTexture.tsthe code usesgl.SRGB8whenisSRGBColorSpaceis true, even for WebGL1. Since WebGL1 requires theEXT_sRGBextension for sRGB texture support, please confirm that one of the following is true:
- A global extension check for
EXT_sRGBis implemented (for example, during context initialization) so that any usage ofgl.SRGB8is safe.- A specific fallback is incorporated in this texture creation logic when running under WebGL1 without extension support.
In our search, we identified that fallback logic for sRGB is present in shader files (e.g., in
packages/core/src/base/Constant.tsand shader common files). However, it is unclear if these global measures are applied toGLTexture.ts. Please verify the integration between these components or add an explicit check/fallback here if needed.
46-48:❓ Verification inconclusive
Double-check sRGB usage for RGBA under WebGL1.
Similar to the R8G8B8 format, using
gl.SRGB8_ALPHA8in WebGL1 may require the EXT_sRGB extension. Ensure you check and gracefully handle scenarios where this extension is unavailable.
🏁 Script executed:
#!/bin/bash # Description: Verify presence of sRGB extension usage or fallback for RGBA sRGB in the codebase. rg -A6 "SRGB8_ALPHA8"Length of output: 5936
sRGB Extension Handling in WebGL1
Please verify that the usage of
gl.SRGB8_ALPHA8inpackages/rhi-webgl/src/GLTexture.tsis correctly gated for WebGL1 contexts. In WebGL1, usinggl.SRGB8_ALPHA8requires the EXT_sRGB extension to be available. If that extension isn’t enabled, you must ensure there is a graceful fallback (for example, reverting to usinggl.RGBA) so that the texture configuration doesn’t lead to runtime errors.Key points to check:
- Confirm that the EXT_sRGB extension is requested and enabled before the sRGB constants are used in WebGL1.
- If the extension isn’t available, ensure that the code falls back to a supported format rather than directly using
gl.SRGB8_ALPHA8.internalFormat: isSRGBColorSpace ? gl.SRGB8_ALPHA8 : isWebGL2 ? gl.RGBA8 : gl.RGBA, baseFormat: isSRGBColorSpace ? (isWebGL2 ? gl.RGBA : gl.SRGB8_ALPHA8) : gl.RGBA, readFormat: gl.RGBA,Review the texture setup path to ensure that these conditions are met.
143-145:❓ Verification inconclusive
Confirm ETC2/ASTC sRGB extension support.
These blocks properly introduce sRGB variants for ETC2 and ASTC formats. Please verify that the hardware and relevant extensions (e.g., WEBGL_compressed_texture_etc, WEBGL_compressed_texture_astc) are available at runtime, or else add suitable fallback code.
Also applies to: 155-157, 182-184
🏁 Script executed:
#!/bin/bash # Description: Check for ETC2/ASTC extension usage or fallback paths. rg -A6 "ETC2|ASTC"Length of output: 80006
Review Verification – Confirmed Runtime Extension Checks for ETC2/ASTC sRGB
After examining the relevant code paths and test files, we can confirm that the sRGB variants for ETC2 and ASTC are selected based on the
isSRGBColorSpaceflag inpackages/rhi-webgl/src/GLTexture.ts(lines 143–145 and similar cases). In addition, our runtime capability checks inpackages/rhi-webgl/src/GLCapability.tsinvoke calls likethis.canIUse(GLCapabilityType.etc)for ETC2 andthis.canIUse(GLCapabilityType.astc)for ASTC formats, ensuring that hardware support for these extensions is verified. Test files (e.g., those in the loader and texture test suites) also exercise these code paths.Action Items:
- Ensure that the current runtime detection (via the GLCapability framework) is sufficient for graceful degradation on hardware that lacks support.
- If there’s a possibility that the relevant extensions (e.g.,
WEBGL_compressed_texture_etcorWEBGL_compressed_texture_astc) might not be available in some target environments, consider implementing an explicit fallback path or clear error messaging for that scenario.Please verify on actual target hardware that the extension checks behave as expected, and update the fallback strategy if needed.
117-119:❓ Verification inconclusive
Validate BC1/BC3/BC7 sRGB format support.
These lines correctly add sRGB variants for BC1/BC3/BC7 compressed formats. However, ensure that the GPU and WebGL extensions are checked before using these compressed sRGB formats. If the hardware or extension is not supported, consider fallback logic or graceful degradation.
Also applies to: 124-126, 131-133
🏁 Script executed:
#!/bin/bash # Description: Check for GPU extension checks for BC1/BC3/BC7 sRGB usage. rg -A6 "S3TC|BPTC|EXT_texture_compression_"Length of output: 6269
GPU/WebGL Extension Validation for Compressed sRGB Formats
The updated code correctly selects sRGB variants for BC1, BC3, and BC7. However, please ensure that—prior to using these sRGB internal formats—the code explicitly verifies that the GPU/WebGL extensions (e.g., for WEBGL_compressed_texture_s3tc_srgb and related formats) are supported. If the required extensions are unavailable, implement fallback logic or graceful degradation to the non-sRGB (standard) formats.
Please review and confirm that these extension checks are enforced consistently across the following sections in this file:
- Lines 117–119 (BC1)
- Lines 124–126 (BC3)
- Lines 131–133 (BC7)
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
packages/core/src/Background.ts (2)
109-113:⚠️ Potential issueConcerning use of @ts-ignore and undocumented Color property
The implementation uses
@ts-ignoreand relies on an undocumented_onValueChangedproperty of the Color class. This approach could be fragile if the Color class implementation changes.Consider implementing a more robust approach that doesn't rely on undocumented features:
- this._solidColor.toLinear(this._linearSolidColor); - // @ts-ignore - this.solidColor._onValueChanged = () => { - this.solidColor.toLinear(this._linearSolidColor); - }; + this._solidColor.toLinear(this._linearSolidColor); + // Rely on the setter to update _linearSolidColor instead of using _onValueChangedAnd update the setter as suggested in the previous comment.
45-53: 🛠️ Refactor suggestionSetter doesn't update _linearSolidColor
The solidColor setter copies the new value but doesn't update _linearSolidColor. This inconsistent behavior relies on the _onValueChanged hook instead of direct synchronization in the setter itself.
set solidColor(value: Color) { if (value !== this._solidColor) { this._solidColor.copyFrom(value); + this._solidColor.toLinear(this._linearSolidColor); } }
🧹 Nitpick comments (1)
packages/rhi-webgl/src/GLTexture.ts (1)
436-453: _supportSRGB() logic covers key formats.This helper enumerates multiple sRGB-compatible formats. For completeness, consider adding a comment clarifying that other ASTC block sizes are not yet included in the sRGB detection.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/core/src/Background.ts(2 hunks)packages/loader/src/Texture2DLoader.ts(2 hunks)packages/rhi-webgl/src/GLTexture.ts(9 hunks)packages/rhi-webgl/src/GLTexture2D.ts(2 hunks)packages/rhi-webgl/src/GLTexture2DArray.ts(2 hunks)packages/rhi-webgl/src/GLTextureCube.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/loader/src/Texture2DLoader.ts
- packages/rhi-webgl/src/GLTextureCube.ts
🧰 Additional context used
🧬 Code Definitions (2)
packages/rhi-webgl/src/GLTexture2DArray.ts (3)
packages/rhi-webgl/src/GLTexture.ts (1)
GLTexture(19-770)packages/core/src/texture/Texture.ts (5)
format(48-50)isSRGBColorSpace(41-43)mipmapCount(104-106)width(55-57)height(62-64)packages/core/src/texture/Texture2DArray.ts (2)
length(17-19)Texture2DArray(11-220)
packages/rhi-webgl/src/GLTexture2D.ts (3)
packages/rhi-webgl/src/WebGLGraphicDevice.ts (1)
isWebGL2(112-114)packages/rhi-webgl/src/GLTexture.ts (1)
GLTexture(19-770)packages/core/src/texture/Texture.ts (2)
format(48-50)isSRGBColorSpace(41-43)
🔇 Additional comments (11)
packages/core/src/Background.ts (2)
34-35: The _linearSolidColor field supports sRGB color space handlingThis new internal field stores the linearized version of the solid color, correctly implementing the sRGB to linear color space conversion needed for proper rendering.
37-37: Good encapsulation of the solidColor propertyConverting the public property to a private field with getter/setter pattern is a good practice for maintaining better control over property updates.
packages/rhi-webgl/src/GLTexture2DArray.ts (2)
16-17: Validation call looks good.Invoking
_validatehere is a sound approach for consolidating all critical checks (e.g., format support, sRGB handling) in one place.
18-21: No concerns on the constructor updates.Destructuring the properties and binding the texture before retrieving format details is straightforward and consistent with typical WebGL patterns.
packages/rhi-webgl/src/GLTexture2D.ts (2)
15-15: Centralized validation is appropriate.Calling
this._validate(texture2D, rhi)simplifies the constructor by consolidating checks for format, sRGB, etc.
17-19: Good usage of sRGB-aware format detail.Retrieving format details with
isSRGBColorSpaceparameter ensures the correct internal and base formats for sRGB support.packages/rhi-webgl/src/GLTexture.ts (5)
31-31: Parameter addition supports sRGB color space.Having an explicit
isSRGBColorSpaceparameter provides flexibility for different color spaces.
38-40: Potential mismatch in base format for WebGL1.When
isSRGBColorSpaceis true and!isWebGL2, settingbaseFormattogl.SRGB8is questionable becausegl.SRGB8is typically an internal format rather than a valid base format in WebGL1. Verify that the required sRGB extension is present or handle fallback gracefully.
46-48: Same caution about base format for RGBA sRGB in WebGL1.For
R8G8B8A8in WebGL1, usinggl.SRGB8_ALPHA8as the base format may not behave as expected without extensions. Please ensure proper fallback if not supported.
117-119: Appropriate usage of compressed sRGB internal formats. Mapping compressed sRGB variants (e.g., DXT1, DXT5, BC7, ETC2, ASTC) looks correct. Confirm that the platform has the relevant extensions for these compressed sRGB formats.
Also applies to: 124-126, 131-133, 143-145, 155-157, 182-184
702-708: Creation of read frame buffer is fine.Lazily creating and caching
_readFrameBufferis a reasonable approach.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/rhi-webgl/src/GLTexture.ts (1)
585-596: Inconsistency between comment and code for sRGB mipmap supportThe comment states "Auto-generating mipmaps for sRGB textures is only supported in [WebGL2 + RGBA]" but the condition checks for
format === TextureFormat.R8G8B8A8. This inconsistency could confuse developers.// Auto-generating mipmaps for sRGB textures is only supported in [WebGL2 + RGBA] - // Auto-generating mipmaps for sRGB textures is only supported in [WebGL2 + RGBA] + // Auto-generating mipmaps for sRGB textures is only supported in [WebGL2 + R8G8B8A8]
🧹 Nitpick comments (2)
packages/rhi-webgl/src/GLTexture.ts (2)
29-47: Support for sRGB color space in _getFormatDetail looks good, but needs additional documentation.The method signature change to include
isSRGBColorSpaceparameter in this critical method is well-implemented. The implementation correctly selects different internal formats based on the color space.However, there's no documentation that explains this new parameter. Consider updating the method's JSDoc comments to describe this parameter.
/** * Get more texture info from TextureFormat. * * @internal + * @param format - The texture format + * @param isSRGBColorSpace - Whether the texture uses sRGB color space + * @param gl - The WebGL rendering context + * @param isWebGL2 - Whether using WebGL2 */
696-697: Good addition of readFormat to handle sRGB correctlyThe modification to use
readFormat ?? baseFormatis a good approach to handle the differences between reading formats in WebGL1 with sRGB textures. However, the comment could be more descriptive.- // Base format is different from read format in webgl1.0 with sRGB + // When using sRGB in WebGL1, we need to use a different format for reading pixels + // than the base format used for texture creation
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/rhi-webgl/src/GLTexture.ts(9 hunks)packages/rhi-webgl/src/GLTexture2DArray.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/rhi-webgl/src/GLTexture2DArray.ts
🧰 Additional context used
🧬 Code Definitions (1)
packages/rhi-webgl/src/GLTexture.ts (3)
packages/core/src/texture/Texture.ts (4)
isSRGBColorSpace(41-43)format(48-50)width(55-57)height(62-64)packages/rhi-webgl/src/type.ts (1)
TextureFormatDetail(46-54)packages/core/src/texture/RenderTarget.ts (2)
width(42-44)height(49-51)
🔇 Additional comments (2)
packages/rhi-webgl/src/GLTexture.ts (2)
436-453: Good implementation of sRGB support checkThe new
_supportSRGBmethod provides a clear way to determine which texture formats support sRGB color space. This is a well-structured addition that will help prevent errors.
702-708: Improved implementation of _getReadFrameBufferThe refactored method is cleaner and more maintainable.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
packages/rhi-webgl/src/GLTexture.ts (2)
38-41: Update baseFormat handling for sRGB in WebGL1.The current logic:
baseFormat: isSRGBColorSpace ? (isWebGL2 ? gl.RGB : gl.SRGB8) : gl.RGB,forces the use of
gl.SRGB8in WebGL1 when sRGB is enabled—even though this constant isn't available by default without the proper extension.Consider updating the logic to check for the necessary extension support:
baseFormat: isSRGBColorSpace ? (isWebGL2 ? gl.RGB - : gl.SRGB8) + : (rhi.canIUse(GLCapabilityType.sRGBTexture) ? gl.SRGB8 : gl.RGB)) : gl.RGB,This ensures that in WebGL1, if the sRGB extension isn't available, the code safely falls back to using
gl.RGB.
710-739: 🛠️ Refactor suggestionAdd WebGL1 sRGB extension validation in the _validate method.
The validation method is well-implemented, but should also check for sRGB extension availability in WebGL1 environments.
// Validate sRGB format // @ts-ignore if (texture._isSRGBColorSpace && !GLTexture._supportSRGB(format)) { Logger.warn("Only support sRGB color space in RGB8 or RGBA8 or some compressed texture format"); // @ts-ignore texture._isSRGBColorSpace = false; } + +// Validate sRGB support in WebGL1 +if (texture._isSRGBColorSpace && !isWebGL2 && !rhi.canIUse(GLCapabilityType.sRGBTexture)) { + Logger.warn("sRGB textures require WebGL2 or EXT_sRGB extension in WebGL1"); + texture._isSRGBColorSpace = false; +}Also, consider using a safer approach for accessing the internal properties:
-// @ts-ignore -texture._isSRGBColorSpace = false; +(texture as any)._isSRGBColorSpace = false;
🧹 Nitpick comments (3)
packages/core/src/BasicResources.ts (3)
244-244: Consider wrapping switch case declarations in blocksThe variable declaration within the switch case could be accessed by other switch clauses if the code is modified in the future, potentially causing unexpected behavior.
case TextureType.Texture2D: - const texture2D = new Texture2D(engine, 1, 1, format, false, isSRGBColorSpace); - texture2D.setPixelBuffer(pixel); - texture = texture2D; - break; + { + const texture2D = new Texture2D(engine, 1, 1, format, false, isSRGBColorSpace); + texture2D.setPixelBuffer(pixel); + texture = texture2D; + break; + }🧰 Tools
🪛 Biome (1.9.4)
[error] 244-244: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
249-249: Consider wrapping switch case declarations in blocksSimilar to the previous comment, wrap the
texture2DArraydeclaration in a block to ensure it's properly scoped to this case.case TextureType.Texture2DArray: - const texture2DArray = new Texture2DArray(engine, 1, 1, 1, format, false, isSRGBColorSpace); - texture2DArray.setPixelBuffer(0, pixel); - texture = texture2DArray; - break; + { + const texture2DArray = new Texture2DArray(engine, 1, 1, 1, format, false, isSRGBColorSpace); + texture2DArray.setPixelBuffer(0, pixel); + texture = texture2DArray; + break; + }🧰 Tools
🪛 Biome (1.9.4)
[error] 249-249: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
254-254: Consider wrapping switch case declarations in blocksAs with the previous cases, wrap the
textureCubedeclaration in a block to ensure proper scoping.case TextureType.TextureCube: - const textureCube = new TextureCube(engine, 1, format, false, isSRGBColorSpace); - for (let i = 0; i < 6; i++) { - textureCube.setPixelBuffer(TextureCubeFace.PositiveX + i, pixel); - } - texture = textureCube; - break; + { + const textureCube = new TextureCube(engine, 1, format, false, isSRGBColorSpace); + for (let i = 0; i < 6; i++) { + textureCube.setPixelBuffer(TextureCubeFace.PositiveX + i, pixel); + } + texture = textureCube; + break; + }🧰 Tools
🪛 Biome (1.9.4)
[error] 254-254: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/core/src/BasicResources.ts(3 hunks)packages/rhi-webgl/src/GLTexture.ts(9 hunks)packages/rhi-webgl/src/GLTexture2DArray.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/rhi-webgl/src/GLTexture2DArray.ts
🧰 Additional context used
🧬 Code Definitions (2)
packages/rhi-webgl/src/GLTexture.ts (4)
packages/core/src/texture/Texture.ts (2)
isSRGBColorSpace(41-43)format(48-50)packages/rhi-webgl/src/WebGLGraphicDevice.ts (3)
gl(124-126)isWebGL2(112-114)WebGLGraphicDevice(77-569)packages/rhi-webgl/src/type.ts (1)
TextureFormatDetail(46-54)packages/rhi-webgl/src/GLCapability.ts (1)
rhi(75-77)
packages/core/src/BasicResources.ts (3)
packages/core/src/texture/Texture2D.ts (1)
Texture2D(12-173)packages/core/src/texture/Texture2DArray.ts (1)
Texture2DArray(11-220)packages/core/src/texture/TextureCube.ts (1)
TextureCube(12-193)
🪛 Biome (1.9.4)
packages/core/src/BasicResources.ts
[error] 244-244: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 249-249: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 254-254: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: e2e (22.x)
🔇 Additional comments (14)
packages/core/src/BasicResources.ts (8)
110-110: New material property for sRGB color space supportThe addition of
blitScreenMaterialenhances the engine's capabilities by providing a dedicated material for screen blitting operations in sRGB color space, aligning with the PR's objective of supporting sRGB color space.
145-151: Initialization of new blitScreenMaterial looks goodThe initialization follows the same pattern as
blitMaterialwith proper settings for depth state. Using the "blit-screen" shader specifically designed for sRGB operations is appropriate.
159-165: LGTM: Proper sRGB parameter for whiteTexture2DThe addition of the sRGB color space parameter (set to true) for the white texture is consistent with the changes across the codebase and ensures proper gamma correction for this commonly used texture.
166-172: LGTM: Proper sRGB parameter for whiteTextureCubeSetting the sRGB color space parameter to true for the white cube texture is appropriate as it ensures consistent color handling across all basic textures.
176-182: LGTM: Proper sRGB parameter for whiteTexture2DArrayThe texture array correctly receives the sRGB color space flag set to true, consistent with other white textures.
185-191: Good choice: setting isSRGBColorSpace=false for uint textureSetting the sRGB color space parameter to false for the uint texture is the correct choice since uint textures store numerical data rather than color information and should not undergo gamma correction.
233-239: Method signature update for sRGB supportThe
_create1x1Texturemethod signature has been appropriately updated to include theisSRGBColorSpaceparameter, enabling control over gamma correction for different texture types.
166-191:❓ Verification inconclusive
Verify consistent usage of sRGB parameters across the codebase
The changes look good, but it's important to ensure that other parts of the codebase have been updated to handle the sRGB color space parameter correctly. This will help prevent any inconsistencies in color rendering.
🏁 Script executed:
#!/bin/bash # Check for any other texture creation without sRGB parameter echo "Checking for potential missing sRGB parameters in texture creation:" rg "new Texture2D\(.*\)" --no-filename | grep -v "isSRGBColorSpace" || echo "All Texture2D creations have sRGB parameter" rg "new TextureCube\(.*\)" --no-filename | grep -v "isSRGBColorSpace" || echo "All TextureCube creations have sRGB parameter" rg "new Texture2DArray\(.*\)" --no-filename | grep -v "isSRGBColorSpace" || echo "All Texture2DArray creations have sRGB parameter" # Look for any other places where texture formats might need sRGB awareness echo -e "\nChecking for potential color space conversion in shaders:" rg "gammaToLinear|linearToGamma" --no-filenameLength of output: 12136
Action Required: Verify Consistent sRGB Handling Across Texture Creations
The updates in packages/core/src/BasicResources.ts (lines 166–191) correctly use the
_create1x1Texturehelper with explicit sRGB flags. However, our repository-wide search uncovered numerous direct instantiations (e.g.,new Texture2Dandnew TextureCube) where the sRGB parameter isn’t explicitly referenced. Please review these instances to ensure they either use the appropriate sRGB flag or rely on intended defaults. This manual verification will help prevent any inadvertent inconsistencies in color rendering across the engine.
- BasicResources.ts: Confirmed proper usage via
_create1x1Texture.- Other texture instantiations: Several cases (as shown by our search) omit an explicit sRGB parameter. Please check if these should be updated or explicitly documented in the default behavior.
packages/rhi-webgl/src/GLTexture.ts (6)
31-34: LGTM! Method signature update to support sRGB color space.The method signature update is necessary for implementing sRGB color space support throughout the texture system.
117-119: LGTM! Comprehensive sRGB support for compressed texture formats.The implementation correctly adds sRGB variants for all supported compressed texture formats (BC1, BC3, BC7, ETC2_RGB, ETC2_RGBA8, ASTC_4x4).
Also applies to: 124-126, 131-133, 143-145, 155-157, 182-184
436-453: LGTM! Well-implemented texture format check for sRGB support.The
_supportSRGBmethod provides a clear and comprehensive list of texture formats that support sRGB color space.
585-596: LGTM! Proper mipmap generation handling for sRGB textures.This code correctly prevents automatic mipmap generation for sRGB textures except in WebGL2 with R8G8B8A8 format, which is the only fully supported case.
675-675: LGTM! Improved pixel buffer reading with format-specific handling.The update correctly uses the new
readFormatproperty to ensure accurate pixel reading, especially important for sRGB textures.Also applies to: 697-697
702-708: LGTM! Good refactoring of framebuffer creation logic.Extracting the framebuffer creation into a separate method improves code organization and reusability.
| internalFormat: isSRGBColorSpace ? gl.SRGB8_ALPHA8 : isWebGL2 ? gl.RGBA8 : gl.RGBA, | ||
| baseFormat: isSRGBColorSpace ? (isWebGL2 ? gl.RGBA : gl.SRGB8_ALPHA8) : gl.RGBA, | ||
| readFormat: gl.RGBA, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update baseFormat handling for sRGB in WebGL1 for RGBA format.
Similar to the RGB format issue, the RGBA format logic needs to be updated to handle WebGL1 environments properly by checking for extension support:
baseFormat: isSRGBColorSpace
? (isWebGL2
? gl.RGBA
- : gl.SRGB8_ALPHA8)
+ : (rhi.canIUse(GLCapabilityType.sRGBTexture) ? gl.SRGB8_ALPHA8 : gl.RGBA))
: gl.RGBA,📝 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.
| internalFormat: isSRGBColorSpace ? gl.SRGB8_ALPHA8 : isWebGL2 ? gl.RGBA8 : gl.RGBA, | |
| baseFormat: isSRGBColorSpace ? (isWebGL2 ? gl.RGBA : gl.SRGB8_ALPHA8) : gl.RGBA, | |
| readFormat: gl.RGBA, | |
| internalFormat: isSRGBColorSpace ? gl.SRGB8_ALPHA8 : isWebGL2 ? gl.RGBA8 : gl.RGBA, | |
| baseFormat: isSRGBColorSpace | |
| ? (isWebGL2 | |
| ? gl.RGBA | |
| : (rhi.canIUse(GLCapabilityType.sRGBTexture) ? gl.SRGB8_ALPHA8 : gl.RGBA)) | |
| : gl.RGBA, | |
| readFormat: gl.RGBA, |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/core/src/RenderPipeline/RenderContext.ts (2)
69-79: Add JSDoc documentation for the new method.This new method is implementing an important feature for sRGB color space support, but it lacks JSDoc documentation to explain its purpose, parameters, and expected behavior. Adding proper documentation would help users understand when and how to use this method.
+/** + * Sets the current render target for rendering operations. + * @param destination - The render target to set, or null for the default framebuffer + * @param viewport - The viewport dimensions for rendering + * @param mipLevel - Optional mip level to render to + * @param faceIndex - Optional cube face index when rendering to a cubemap + * @internal + */ setRenderTarget(destination: RenderTarget | null, viewport: Vector4, mipLevel?: number, faceIndex?: TextureCubeFace) {
70-72: Add null checks for safety.The method assumes that
this.camera,this.camera.engine, andengine._hardwareRendererare not null. Consider adding null checks to prevent potential runtime errors.setRenderTarget(destination: RenderTarget | null, viewport: Vector4, mipLevel?: number, faceIndex?: TextureCubeFace) { const engine = this.camera.engine; + if (!engine) { + console.warn("Cannot set render target: camera engine is null"); + return; + } const rhi = engine._hardwareRenderer; + if (!rhi) { + console.warn("Cannot set render target: hardware renderer is null"); + return; + } rhi.activeRenderTarget(destination, viewport, this.flipProjection, mipLevel, faceIndex);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (10)
packages/core/src/postProcess/shaders/Bloom/BloomBlurH.glslis excluded by!**/*.glslpackages/core/src/postProcess/shaders/Bloom/BloomBlurV.glslis excluded by!**/*.glslpackages/core/src/postProcess/shaders/Bloom/BloomPrefilter.glslis excluded by!**/*.glslpackages/core/src/postProcess/shaders/Bloom/BloomUpsample.glslis excluded by!**/*.glslpackages/core/src/postProcess/shaders/Filtering.glslis excluded by!**/*.glslpackages/core/src/postProcess/shaders/UberPost.glslis excluded by!**/*.glslpackages/core/src/shaderlib/common.glslis excluded by!**/*.glslpackages/core/src/shaderlib/extra/Blit.fs.glslis excluded by!**/*.glslpackages/core/src/shaderlib/extra/BlitScreen.fs.glslis excluded by!**/*.glslpackages/shader-shaderlab/src/shaders/Common.glslis excluded by!**/*.glsl
📒 Files selected for processing (1)
packages/core/src/RenderPipeline/RenderContext.ts(2 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
packages/core/src/RenderPipeline/RenderContext.ts (3)
packages/core/src/texture/RenderTarget.ts (1)
RenderTarget(11-245)packages/core/src/Camera.ts (2)
viewport(253-255)viewport(257-261)packages/core/src/Engine.ts (1)
Engine(51-697)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: e2e (22.x)
🔇 Additional comments (2)
packages/core/src/RenderPipeline/RenderContext.ts (2)
74-78: LGTM! Proper sRGB macro management.The code correctly manages the
_outputSRGBCorrectMacrobased on whether we're rendering to a custom target or back to the default framebuffer. This approach aligns with the PR objective of supporting sRGB color space.
79-79:✅ Verification successful
Check if this line should be retained.
There's a previous review comment from GuoLei1990 suggesting to delete this line (the closing brace). However, this would make the code invalid. Consider if there might have been a different intention or if the comment was referring to a different version of the code.
🏁 Script executed:
#!/bin/bash # Check if there are other similar comments or issues with braces in the PR git diff | grep -E '[\{\}]' | wc -lLength of output: 39
Action: Confirm Retention of Closing Brace
After verifying the diff (which shows no modifications to bracket usage), it’s clear that the closing brace in question is essential for maintaining valid syntax. The previous comment from GuoLei1990 likely pertained to an older or different version of the code. No change is needed here—the line is correctly retained.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
packages/rhi-webgl/src/GLTexture2DArray.ts (2)
16-16: Explicitly specifyingtruefor WebGL2.Passing
trueto_getFormatDetailclarifies that the target is WebGL2. For code clarity, consider using a descriptive boolean property (e.g.,this._isWebGL2) to reinforce the meaning at call sites.- this._formatDetail = GLTexture._getFormatDetail(format, isSRGBColorSpace, this._gl, true); + const isWebGL2 = true; + this._formatDetail = GLTexture._getFormatDetail(format, isSRGBColorSpace, this._gl, isWebGL2);
118-139: Validate sRGB fallbacks and format support.
- The
_validatemethod properly ensures WebGL2 usage, checks for unsupported formats, and downgrades the sRGB flag if not supported.- Consider adding or expanding tests to verify that sRGB fallback and error handling (e.g., unsupported WebGL1.0 usage) occur as expected in real scenarios.
Overall, this improves runtime stability by catching invalid configurations early.
packages/loader/src/TextureCubeLoader.ts (1)
28-45: Handling optional sRGB and mipmap parameters.Using
TextureUtils.supportGenerateMipmapsWithCorrection()with potentially undefinedisSRGBColorSpaceis safe, as falsy inputs effectively skip sRGB checks. However, consider defaulting to a boolean (e.g.,true) to mirror the engine’s typical defaults and improve clarity.- const { format, anisoLevel, wrapModeU, wrapModeV, filterMode, isSRGBColorSpace, mipmap } = item.params ?? {}; + const { + format, + anisoLevel, + wrapModeU, + wrapModeV, + filterMode, + isSRGBColorSpace = true, + mipmap = true + } = item.params ?? {};packages/core/src/texture/TextureUtils.ts (2)
4-7: Consider refactoring out static-only class.
Static analysis reports that classes containing only static methods can be replaced with simple functions. Converting this class to a set of named functions (or a namespace-like object) can simplify the code.-export class TextureUtils { +// Example refactor into named functions or an object literal +// textureUtils.ts +function supportGenerateMipmapsWithCorrection(...) { ... } +function supportSRGB(...) { ... } +function supportMipmaps(...) { ... } +function supportGenerateMipmaps(...) { ... } // Then export them: -export class TextureUtils { ... } +export { supportGenerateMipmapsWithCorrection, supportSRGB, supportMipmaps, supportGenerateMipmaps };
54-66: Consider extension-based approach for sRGB mipmap generation.
Currently, the code disallows auto mipmap generation for sRGB unless it’s[WebGL2 + R8G8B8A8]. If you wish to support compressed sRGB or additional sRGB formats in the future, you might handle extension availability checks here as well.packages/rhi-webgl/src/GLTexture.ts (1)
565-575: Auto mipmap generation for sRGB with condition checks.
The code logs a warning if auto-generating mipmaps for sRGB is not supported. This is appropriate, but consider providing a fallback or partial fallback if feasible.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
packages/core/src/texture/TextureUtils.ts(1 hunks)packages/core/src/texture/index.ts(1 hunks)packages/loader/src/Texture2DLoader.ts(3 hunks)packages/loader/src/TextureCubeLoader.ts(2 hunks)packages/rhi-webgl/src/GLTexture.ts(8 hunks)packages/rhi-webgl/src/GLTexture2DArray.ts(3 hunks)tests/src/core/texture/Texture2D.test.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/src/core/texture/Texture2D.test.ts
- packages/loader/src/Texture2DLoader.ts
🧰 Additional context used
🧬 Code Graph Analysis (3)
packages/loader/src/TextureCubeLoader.ts (5)
packages/rhi-webgl/src/WebGLGraphicDevice.ts (1)
isWebGL2(112-114)packages/core/src/texture/TextureUtils.ts (1)
TextureUtils(7-67)packages/core/src/texture/Texture.ts (2)
format(48-50)isSRGBColorSpace(41-43)packages/core/src/texture/TextureCube.ts (1)
TextureCube(12-193)packages/rhi-webgl/src/GLTexture.ts (4)
anisoLevel(493-498)filterMode(467-488)wrapModeU(451-454)wrapModeV(459-462)
packages/rhi-webgl/src/GLTexture.ts (2)
packages/core/src/texture/TextureUtils.ts (1)
TextureUtils(7-67)packages/math/src/MathUtil.ts (1)
MathUtil(4-71)
packages/rhi-webgl/src/GLTexture2DArray.ts (3)
packages/rhi-webgl/src/GLTexture.ts (1)
GLTexture(21-746)packages/core/src/texture/Texture2DArray.ts (1)
Texture2DArray(11-220)packages/core/src/texture/TextureUtils.ts (1)
TextureUtils(7-67)
🪛 Biome (1.9.4)
packages/core/src/texture/TextureUtils.ts
[error] 7-67: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: e2e (22.x)
🔇 Additional comments (21)
packages/core/src/texture/index.ts (1)
13-13: Re-exportingTextureUtilsis beneficial.This addition makes the utility functions universally accessible within the module. However, please verify that the corresponding unit tests adequately cover the newly exposed functionality to ensure ongoing stability.
packages/rhi-webgl/src/GLTexture2DArray.ts (3)
1-1: ImportingTextureUtils.The newly added import is valid since it's referenced in the
_validate()method. No issues here.
12-12: Centralizing validation logic.Invoking
_validate(texture2DArray, rhi)is a clear improvement for readability and maintainability, as it consolidates checks into a single method. Ensure this method is fully covered by tests for various scenarios (e.g., different formats, sRGB toggles, WebGL2 check).
14-14: Potential usage note onisSRGBColorSpace.Destructuring
isSRGBColorSpaceafter_validate()allows retrieving an updated value if_validatemodifies_isSRGBColorSpace. This is correct in principle, but confirm that the fallback tofalsein_validate()is accurately reflected here for dependent logic (e.g.,_getFormatDetail).packages/loader/src/TextureCubeLoader.ts (2)
10-10: ImportingTextureUtils.Newly importing
TextureUtilsaligns with usage below for sRGB and mipmap corrections. Looks appropriate.
47-57: Conditional mipmap generation.Delaying
texture.generateMipmaps()until after setting all faces is correct, ensuring complete face data. No issues here.packages/core/src/texture/TextureUtils.ts (4)
1-3: Imports LGTM.
These import statements cleanly bring in needed modules with no apparent issues.
8-32: Clarify overwriting of the isSRGBColorSpace flag.
WithinsupportGenerateMipmapsWithCorrection, the method adjusts theisSRGBColorSpaceparameter whenformatdoes not support sRGB. This may be surprising to the caller if they expect the flag to remain as originally provided. Consider returning a separate flag or clarifying documentation to highlight this side effect.
34-48: sRGB format checks are consistent.
The switch statement properly identifies formats capable of sRGB. No issues here.
50-52: Mipmap support logic is straightforward.
Checking for WebGL2 or power-of-two dimensions is a sensible approach.packages/rhi-webgl/src/GLTexture.ts (11)
11-11: Import of TextureUtils looks good.
This aligns with the new utility methods for mipmap checks.
14-14: Import of MathUtil is consistent.
Used for power-of-two checks and no apparent issues.
35-36: Check fallback for WebGL1 sRGB on R8G8B8.
WhenisSRGBColorSpaceis true,baseFormatfalls togl.SRGB8in WebGL1, which may not be available without an extension. If you want to adopt a graceful fallback, consider verifying extension availability here as well.
43-45: R8G8B8A8 sRGB support is well-defined.
Only minor concern would be to confirm extension checks for WebGL1, but overall logic is correct for WebGL2.
114-130: Ensure BC1, BC3, BC7 sRGB extension availability.
These lines correctly map compressed formats to their sRGB counterparts. Just ensure the code later checks for extension availability (e.g., S3TC) before using them. Otherwise, fallback logic or error messaging may be needed.
140-142: Confirm ETC2/ASTC sRGB extension coverage.
These toggles for ETC2_RGB, ETC2_RGBA8, and ASTC_4x4 are correct. Verify corresponding extension checks or fallback logic in_supportTextureFormat.Also applies to: 152-154, 179-181
653-653: No issues with readFormat usage.
The introduced logic for reading pixel data withreadFormat ?? baseFormatis appropriate for sRGB or other format mismatches in WebGL1.
680-686: Read frame buffer creation is straightforward.
Creates a single shared_readFrameBufferif it doesn’t exist. This is a sensible approach.
688-719: Validation logic is comprehensive.
The_validatemethod properly checks for format, sRGB support, and NPOT texture restrictions in WebGL1. Implementation seems correct.
727-727: Wrap mode fallback is appropriate.
For NPOT textures, forcing clamp to edge in WebGL1 is consistent with specifications.
28-28:✅ Verification successful
Verify usage of isSRGBColorSpace in call sites.
_getFormatDetailnow requiresisSRGBColorSpace. Ensure all calls in the codebase pass the correct parameter or handle any missing arguments.
🏁 Script executed:
#!/bin/bash # Description: Find all invocations of _getFormatDetail without the new isSRGBColorSpace parameter. rg "_getFormatDetail\(" -A 2 -B 2Length of output: 1788
All
_getFormatDetailinvocations correctly pass theisSRGBColorSpaceparameter.
- Verified in
packages/rhi-webgl/src/GLTextureCube.ts,packages/rhi-webgl/src/GLTexture2DArray.ts, andpackages/rhi-webgl/src/GLTexture2D.tsthat the new parameter is included in every call.- No call sites were found missing the argument, so the update appears complete.
Please check if the PR fulfills these requirements
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Support sRGB color space as #2563
What is the current behavior? (You can also link to an open issue here)
use 'gammaToLinear' in shader
What is the new behavior (if this is a feature change)?
use sRGB color space and support
gammaToLinearby GPU.Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)
isSRGBColorSpacein texture loader's paramsglTF loaderis determined by the order in which the materials are parsed. For example, if the same texture is used as both abaseTextureand anormalTexture, thebaseTexturewill be parsed first, so the texture will be considered to be insRGB space.Other information:
Summary by CodeRabbit
New Features
Refinements