Support sRGB color space #2593
Support sRGB color space #2593
Conversation
WalkthroughThis pull request introduces enhanced support for sRGB color space across texture and rendering pipelines. It adds new parameters (e.g., Changes
Sequence Diagram(s)sequenceDiagram
participant Loader
participant Engine
participant HWRenderer as HardwareRenderer
participant Texture as Texture2D
Loader->>Engine: Request texture load with isSRGBColorSpace flag
Engine->>HWRenderer: Check sRGB capability (canIUse(sRGB))
alt sRGB Supported
Engine->>Texture: Instantiate Texture2D(isSRGBColorSpace=true, ...)
else Not Supported
Engine->>Texture: Instantiate Texture2D with _noSRGBSupportMacro enabled
end
Texture-->>Loader: Return texture instance
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 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.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev/1.5 #2593 +/- ##
=========================================
Coverage 68.65% 68.65%
=========================================
Files 964 965 +1
Lines 101035 101293 +258
Branches 8679 8709 +30
=========================================
+ Hits 69365 69545 +180
- Misses 31408 31485 +77
- Partials 262 263 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
e2e/case/.mockForE2E.ts (1)
43-43: Updated texture initialization to support sRGB color spaceThe
Texture2Dconstructor now includes theisSRGBColorSpaceparameter set totrue, which aligns with the PR objective of adding sRGB color space support. This ensures that the render target texture used for screenshots will correctly handle colors in the sRGB space.However, the three
undefinedparameters between the dimensions and the color space flag make the code less readable. Consider using named parameters or adding a comment to explain the purpose of these undefined values.- const renderColorTexture = new Texture2D(engine, width, height, undefined, undefined, undefined, true); + // Parameters: engine, width, height, format, mipmap, arraySize, isSRGBColorSpace + const renderColorTexture = new Texture2D(engine, width, height, undefined, undefined, undefined, true);packages/core/src/Background.ts (1)
110-116: Add explicit documentation about color spacesThere's no clear documentation in the code about which color space each color property represents (sRGB vs linear).
Consider adding comments to clarify the color spaces:
/** @internal */ + /** Linear color space representation of the solid color */ _linearSolidColor: Color; private _solidColor: Color = new Color(0.25, 0.25, 0.25, 1.0); /** * Background solid color. * @defaultValue `new Color(0.25, 0.25, 0.25, 1.0)` * @remarks When `mode` is `BackgroundMode.SolidColor`, the property will take effects. + * @remarks This color is in sRGB color space. For linear space calculations, use _linearSolidColor. */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
packages/core/src/shaderlib/pbr/btdf.glslis excluded by!**/*.glslpackages/shader-shaderlab/src/shaders/shadingPBR/BTDF.glslis excluded by!**/*.glsl
📒 Files selected for processing (5)
e2e/case/.mockForE2E.ts(1 hunks)e2e/case/material-shaderReplacement.ts(2 hunks)e2e/case/particleRenderer-dream.ts(2 hunks)packages/core/src/Background.ts(3 hunks)packages/core/src/RenderPipeline/BasicRenderPipeline.ts(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- e2e/case/particleRenderer-dream.ts
- packages/core/src/RenderPipeline/BasicRenderPipeline.ts
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: e2e (22.x)
🔇 Additional comments (4)
packages/core/src/Background.ts (3)
34-36: New internal field for linear color space representationThis new
_linearSolidColorfield stores the linear color space representation of the solid color, which is needed for the sRGB color space support. This is a good approach to maintain both representations.
37-54: Good encapsulation of solidColor with getter/setterConverting
solidColorfrom a direct property to a getter/setter pattern is a good practice. This encapsulation allows for controlled modifications to the property and enables the synchronization of the linear color representation when changes occur.
99-99: Proper cleanup in destroy methodAdding cleanup for the new
_linearSolidColorfield in the destroy method is important to prevent memory leaks. Good implementation.e2e/case/material-shaderReplacement.ts (1)
63-67:❓ Verification inconclusive
Correct implementation of sRGB color space support for texture loading
The addition of the
isSRGBColorSpace: trueparameter in the texture loading configuration is aligned with the PR objectives for supporting sRGB color space. This ensures that the UV check texture is properly interpreted in the sRGB color space.Since this is part of a breaking change mentioned in the PR objectives, it would be helpful to verify that this parameter is consistently used across all texture loading code paths in the codebase.
🏁 Script executed:
#!/bin/bash # Search for texture loading instances to ensure consistent usage of isSRGBColorSpace rg "type:\s*AssetType\.Texture2D" --type ts -A 5Length of output: 28834
Update: Verified Texture Loading sRGB Configuration
The addition of the
isSRGBColorSpace: trueparameter ine2e/case/material-shaderReplacement.ts(lines 63–67) is correctly implemented to support sRGB texture loading. A repository-wide search confirms that most texture loading code paths—especially in our e2e tests (e.g., inmaterial-blinn-phong.ts,primitive-cuboid.ts, andspriteMask-customStencil.ts)—include this parameter where needed.
- In files like
material-blinn-phong.ts, one instance correctly setsparams: { isSRGBColorSpace: true }, while a second texture loading block omits it. Please verify that omissions in such cases are intentional (i.e., the texture in question does not require sRGB conversion).- Overall, the changes align with the PR objectives regarding sRGB support.
Please review the noted instance(s) to ensure consistent behavior across all texture loading code paths.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
packages/core/src/Background.ts (2)
111-115:⚠️ 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._linearSolidColor = this._solidColor.toLinear(this.solidColor.clone()); - // @ts-ignore - this.solidColor._onValueChanged = () => { - this.solidColor.toLinear(this._linearSolidColor); - }; + this._linearSolidColor = new Color(); + this._solidColor.toLinear(this._linearSolidColor);And ensure that the setter for
solidColorupdates_linearSolidColoras mentioned in the previous comment.
49-53:⚠️ Potential issueMissing update to _linearSolidColor in the solidColor setter
The setter for
solidColorproperly updates_solidColorwhen a new value is set, but it fails to update_linearSolidColor. This creates an inconsistency with the constructor's behavior which sets up synchronization between these two values.set solidColor(value: Color) { if (value !== this._solidColor) { this._solidColor.copyFrom(value); + this._solidColor.toLinear(this._linearSolidColor); } }
🧹 Nitpick comments (2)
packages/core/src/Background.ts (2)
34-36: Add documentation for _linearSolidColorThis new field is essential for sRGB color space support, but lacks documentation explaining its purpose and usage. While it's marked as
@internal, additional comments would help other developers understand how it relates to the sRGB implementation./** @internal */ + /** + * Stores the linear color space representation of the background solid color. + * This is used for rendering when working with sRGB color space. + */ _linearSolidColor: Color;
110-115: Potential reference issue with solidColor in constructorThe constructor uses
this.solidColorin some places andthis._solidColorin others, which could lead to confusion. SincesolidColoris now a getter/setter, using the private field directly would be more appropriate in this context.- this._linearSolidColor = this._solidColor.toLinear(this.solidColor.clone()); + this._linearSolidColor = new Color(); + this._solidColor.toLinear(this._linearSolidColor);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
packages/core/src/postProcess/shaders/Filtering.glslis excluded by!**/*.glsl
📒 Files selected for processing (1)
packages/core/src/Background.ts(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: e2e (22.x)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/core/src/Background.ts (3)
49-53: Ensure the solidColor setter handles null and undefined values.Unlike the
texturesetter (lines 63-71), thesolidColorsetter doesn't check ifvalueis null or undefined. This could lead to errors if null is passed. Also note that this code isn't covered by tests.set solidColor(value: Color) { - if (value !== this._solidColor) { + if (value && value !== this._solidColor) { this._solidColor.copyFrom(value); } }🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 50-53: packages/core/src/Background.ts#L50-L53
Added lines #L50 - L53 were not covered by tests
92-99: Consider cleaning up _solidColor and _linearSolidColor in the destroy method.The
destroymethod cleans up other properties but doesn't address_solidColorand_linearSolidColor, which could lead to memory leaks.destroy(): void { this.texture = null; this._mesh._addReferCount(-1); this._mesh = null; this._material._addReferCount(-1); this._material = null; + this._solidColor = null; + this._linearSolidColor = null; this.sky.destroy(); }
108-109: Initialize _linearSolidColor more directlyFor clearer code, consider initializing
_linearSolidColorin a more straightforward way.- this._linearSolidColor = this._solidColor.toLinear(this.solidColor.clone()); + this._linearSolidColor = new Color(); + this._solidColor.toLinear(this._linearSolidColor);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/core/src/Background.ts(2 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
packages/core/src/Background.ts
[warning] 50-53: packages/core/src/Background.ts#L50-L53
Added lines #L50 - L53 were not covered by tests
[warning] 112-113: packages/core/src/Background.ts#L112-L113
Added lines #L112 - L113 were not covered by tests
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: e2e (22.x)
🔇 Additional comments (1)
packages/core/src/Background.ts (1)
110-113: Concerning use of @ts-ignore and undocumented Color propertyThe 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. Also note that this code isn't covered by tests according to the code coverage report.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 112-113: packages/core/src/Background.ts#L112-L113
Added lines #L112 - L113 were not covered by tests
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/rhi-webgl/src/GLTexture.ts (1)
679-715: Comprehensive texture validation with important sRGB compatibility checks.The new
_validatemethod provides robust validation for texture formats, dimensions, and mipmap settings. It properly:
- Verifies texture format support
- Handles non-power-of-2 texture limitations in WebGL1
- Includes special validation for sRGB mipmaps, warning users when auto-generating mipmaps for sRGB textures in unsupported configurations
This centralized validation approach improves code maintainability and user experience by providing clear warnings for compatibility issues.
However, consider including more specific information in the warning messages about what texture is being affected to help with debugging.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
e2e/case/particleRenderer-force.ts(1 hunks)e2e/case/particleRenderer-shape-mesh.ts(1 hunks)e2e/config.ts(1 hunks)e2e/package.json(1 hunks)packages/rhi-webgl/src/GLTexture.ts(7 hunks)packages/rhi-webgl/src/type.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- e2e/package.json
- e2e/config.ts
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: e2e (22.x)
🔇 Additional comments (12)
packages/rhi-webgl/src/type.ts (2)
52-53: Clear documentation for new readFormat property.The addition of the
readFormatproperty in theTextureFormatDetailinterface is well-documented with a comment explaining its purpose for specific cases of thereadPixelsfunction.
114-118: Good addition of sRGB S3TC texture format constants.These new enum values appropriately extend support for sRGB variants of S3TC compressed texture formats, which is essential for the sRGB color space implementation. The hexadecimal values match the WebGL standard constants.
packages/rhi-webgl/src/GLTexture.ts (6)
31-31: Parameter addition for sRGB color space support.The new
isSRGBColorSpaceparameter in the_getFormatDetailmethod is a necessary addition to enable sRGB format selection based on the texture's color space.
38-41: Correctly implemented sRGB format selection for R8G8B8.The implementation properly handles different format selection paths based on the sRGB flag and WebGL version, with the additional
readFormatensuring correct pixel reading behavior.
46-49: Proper sRGB format selection for R8G8B8A8.Similar to the R8G8B8 case, the implementation correctly handles different format selections based on sRGB flag and WebGL version, with appropriate read format.
117-119: Comprehensive sRGB support for compressed texture formats.The implementation correctly selects between standard and sRGB variants of compressed texture formats (BC1, BC3, BC7, ETC2, ASTC) based on the
isSRGBColorSpaceflag, enabling proper color space representation for these formats.Also applies to: 124-126, 131-133, 143-145, 155-157, 182-184
645-645: Enhanced pixel reading with format-specific handling.The code now properly uses the new
readFormatproperty when reading pixels, falling back tobaseFormatwhen not specified, improving compatibility with various texture formats.Also applies to: 666-666
671-677: Optimized read framebuffer management.The implementation now lazily creates the read framebuffer only when needed, improving resource management.
e2e/case/particleRenderer-force.ts (4)
52-55: Addition of sRGB color space parameter is appropriateThe addition of
params: { isSRGBColorSpace: true }to the texture configuration aligns with the PR objectives to support sRGB color space. This enables proper gamma correction when displaying the particle textures.
59-62: Consistent application of sRGB color space parameterThe
isSRGBColorSpace: trueparameter is consistently applied to this texture resource, ensuring all textures within the particle system use the same color space configuration.
66-69: Properly configured sRGB parameter for particle textureThe third texture correctly implements the sRGB color space parameter, maintaining consistency with the rest of the texture configurations in this file.
73-76: sRGB color space configuration complete for all texturesAll textures in this particle system now properly specify the sRGB color space parameter, ensuring consistent color representation across the entire effect.
| format = TextureFormat.R8G8B8A8, | ||
| mipmap = true, | ||
| usage = TextureUsage.Static, | ||
| isSRGBColorSpace = false |
There was a problem hiding this comment.
-
Default should be
trueis better! -
Maybe
isSRGBColorSpaceshould before thanusage
| @@ -34,15 +36,9 @@ export class Texture2D extends Texture { | |||
| this._usage = usage; | |||
| this._format = format; | |||
| this._mipmapCount = this._getMipmapCount(); | |||
| this._isSRGBColorSpace = isSRGBColorSpace; | |||
There was a problem hiding this comment.
Why not do this in super class
| @@ -33,6 +34,11 @@ export abstract class Texture extends GraphicsResource { | |||
| private _depthCompareFunction: TextureDepthCompareFunction; | |||
| private _useDepthCompareMode: boolean = false; | |||
|
|
|||
| // Whether the texture is in the sRGB color space. | |||
There was a problem hiding this comment.
??? Your write comments like this?
| */ | ||
| constructor( | ||
| engine: Engine, | ||
| width: number, | ||
| height: number, | ||
| length: number, | ||
| format: TextureFormat = TextureFormat.R8G8B8A8, | ||
| mipmap: boolean = true | ||
| mipmap: boolean = true, | ||
| isSRGBColorSpace = false |
| super(engine); | ||
|
|
||
| this._mipmap = mipmap; | ||
| this._width = size; | ||
| this._height = size; | ||
| this._format = format; | ||
| this._mipmapCount = this._getMipmapCount(); | ||
| this._isSRGBColorSpace = isSRGBColorSpace; |
| Depth32Stencil8 = 34, | ||
| Depth32Stencil8 = 33, | ||
| /** Render to stencil buffer. */ | ||
| Stencil = 34, |
There was a problem hiding this comment.
Why? Modifying the value of a bit operation may cause deserialization errors
| @@ -71,4 +79,6 @@ export interface Texture2DParams { | |||
| filterMode: TextureFilterMode; | |||
| /** Anisotropic level for texture. */ | |||
| anisoLevel: number; | |||
| /** Whether to use sRGB color space, default is false */ | |||
| width, | ||
| undefined, | ||
| undefined, | ||
| item.params?.isSRGBColorSpace ?? false |
There was a problem hiding this comment.
Why not same with Texture2D？
| @@ -350,7 +350,7 @@ export class GLTFUtils { | |||
| /** | |||
| * Load image buffer | |||
| */ | |||
| static loadImageBuffer(imageBuffer: ArrayBuffer, type: string): Promise<HTMLImageElement> { | |||
| static loadImageBuffer(imageBuffer: ArrayBuffer | TypedArray, type: string): Promise<HTMLImageElement> { | |||
| internalFormat: isWebGL2 ? gl.RGB8 : gl.RGB, | ||
| baseFormat: gl.RGB, | ||
| internalFormat: isSRGBColorSpace ? gl.SRGB8 : isWebGL2 ? gl.RGB8 : gl.RGB, | ||
| baseFormat: isWebGL2 ? gl.RGB : isSRGBColorSpace ? gl.SRGB8 : gl.RGB, |
There was a problem hiding this comment.
Why not unify the judgment logic？
| @@ -647,11 +662,58 @@ export class GLTexture implements IPlatformTexture { | |||
| } else { | |||
| gl.framebufferTexture2D(gl.FRAMEBUFFER, gl.COLOR_ATTACHMENT0, gl.TEXTURE_2D, this._glTexture, mipLevel); | |||
| } | |||
| gl.readPixels(x, y, width, height, baseFormat, dataType, out); | |||
|
|
|||
| gl.readPixels(x, y, width, height, readFormat ?? baseFormat, dataType, out); | |||
| /** @ts-ignore */ | ||
| texture2D._mipmapCount = texture2D._getMipmapCount(); | ||
| } | ||
| this._validate(texture2D, rhi); |
| @@ -112,6 +111,12 @@ export enum GLCompressedTextureInternalFormat { | |||
| RGBA_S3TC_DXT3_EXT = 0x83f2, | |||
| RGBA_S3TC_DXT5_EXT = 0x83f3, | |||
|
|
|||
| // s3tc + WEBGL_compressed_texture_s3tc_srgb | |||
|
|
||
|
|
||
| // Compatible with devices that do not even support EXT_sRGB in WebGL1.0. | ||
| vec4 texture2D_SRGB(sampler2D tex, vec2 uv) { |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
e2e/case/particleRenderer-dream.ts (1)
44-62: Consider explicitly specifying sRGB color space for texturesAccording to the PR objectives, users must specify
isSRGBColorSpacein the parameters of the texture loader. However, this test doesn't explicitly set this parameter when loading textures. While it's likely using the default value (which seems to betruebased on the PR summary), it would be clearer to explicitly specify this for better test readability and to demonstrate the new functionality.Consider updating the texture loading to explicitly specify the sRGB color space:
.load([ { url: "https://mdn.alipayobjects.com/huamei_b4l2if/afts/img/A*JPsCSK5LtYkAAAAAAAAAAAAADil6AQ/original", - type: AssetType.Texture2D + type: AssetType.Texture2D, + isSRGBColorSpace: true }, // Apply similar changes to other texture loadspackages/core/src/texture/TextureCube.ts (1)
19-29: Consider documenting the breaking changes in a code comment.While the PR objectives mention this introduces breaking changes affecting text color, sprite color, and UI output, it might be helpful to add a note in the code documentation about these potential effects. This would help users understand the implications when upgrading.
packages/loader/src/EnvLoader.ts (1)
28-28: Appropriate color space selection for environment textures.Setting
isSRGBColorSpacetofalsefor environment textures is correct, as environment maps typically store linear HDR data rather than gamma-corrected sRGB data. This will ensure proper color space handling during rendering.Consider adding a brief comment explaining why sRGB is set to false for environment textures:
- texture ||= new TextureCube(engine, size, undefined, undefined, false); + // Environment maps store linear data, not sRGB + texture ||= new TextureCube(engine, size, undefined, undefined, false);packages/loader/src/HDRLoader.ts (1)
96-96: Correct color space handling for HDR textures.Setting
isSRGBColorSpacetofalsefor HDR textures is appropriate, as HDR data represents linear light values and should not be treated as sRGB. This ensures the engine will correctly process the HDR data without unwanted gamma conversions.Consider adding a brief comment explaining why sRGB is set to false for HDR textures:
- texture ||= new TextureCube(engine, cubeSize, undefined, undefined, false); + // HDR data represents linear light values, not sRGB + texture ||= new TextureCube(engine, cubeSize, undefined, undefined, false);tests/src/core/SpriteMask.test.ts (1)
30-30: Test adaption for the new sRGB color space parameter.The tests have been appropriately updated to work with the new Texture2D constructor signature, explicitly setting
isSRGBColorSpacetofalsefor all texture instances. This ensures consistent test behavior without gamma correction.Since you've set
isSRGBColorSpacetofalsein all tests (overriding the defaulttrue), consider:
- Adding a comment explaining the rationale for using linear color space in tests
- Adding a new test case that specifically verifies behavior when
isSRGBColorSpaceistrue(the default) to ensure both paths are testedit("should correctly handle sRGB color space", () => { const rootEntity = scene.getRootEntity(); const spriteMask = rootEntity.addComponent(SpriteMask); // Test with default sRGB = true const texture = new Texture2D(engine, 100, 100); expect(texture.isSRGBColorSpace).to.eq(true); // Additional verification of sRGB behavior });Also applies to: 46-46, 89-89, 111-111, 154-154, 166-166, 174-174
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
e2e/case/material-blinn-phong.ts(1 hunks)e2e/case/material-shaderReplacement.ts(1 hunks)e2e/case/particleRenderer-dream.ts(1 hunks)e2e/case/spriteMask-customStencil.ts(1 hunks)packages/core/src/Engine.ts(2 hunks)packages/core/src/material/utils/PrefilteredDFG.ts(1 hunks)packages/core/src/mesh/BlendShapeManager.ts(1 hunks)packages/core/src/mesh/SkinnedMeshRenderer.ts(1 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/loader/src/EnvLoader.ts(1 hunks)packages/loader/src/HDRLoader.ts(1 hunks)packages/loader/src/KTXLoader.ts(1 hunks)packages/loader/src/Texture2DLoader.ts(2 hunks)packages/loader/src/TextureCubeLoader.ts(1 hunks)tests/src/core/SpriteMask.test.ts(6 hunks)tests/src/core/texture/Texture2D.test.ts(2 hunks)tests/src/loader/GLTFLoader.test.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- e2e/case/material-shaderReplacement.ts
- packages/core/src/texture/Texture2DArray.ts
- packages/loader/src/Texture2DLoader.ts
- packages/loader/src/TextureCubeLoader.ts
- packages/core/src/texture/Texture2D.ts
🧰 Additional context used
🧬 Code Definitions (8)
packages/loader/src/KTXLoader.ts (2)
packages/core/src/texture/Texture2D.ts (1)
Texture2D(12-181)packages/core/src/Engine.ts (1)
resourceManager(167-169)
packages/core/src/Engine.ts (1)
packages/core/src/texture/Texture2D.ts (1)
Texture2D(12-181)
packages/core/src/material/utils/PrefilteredDFG.ts (1)
packages/core/src/texture/Texture2D.ts (1)
Texture2D(12-181)
packages/core/src/texture/TextureCube.ts (1)
packages/core/src/texture/Texture.ts (2)
format(45-47)isSRGBColorSpace(38-40)
packages/loader/src/EnvLoader.ts (1)
packages/core/src/texture/TextureCube.ts (1)
TextureCube(12-201)
packages/core/src/mesh/SkinnedMeshRenderer.ts (1)
packages/core/src/texture/Texture2D.ts (1)
Texture2D(12-181)
tests/src/core/texture/Texture2D.test.ts (1)
packages/core/src/texture/Texture2D.ts (1)
Texture2D(12-181)
tests/src/core/SpriteMask.test.ts (3)
packages/core/src/2d/sprite/Sprite.ts (3)
texture(45-47)texture(49-57)Sprite(13-418)packages/core/src/texture/Texture2D.ts (1)
Texture2D(12-181)packages/core/src/2d/sprite/SpriteMask.ts (1)
SpriteMask(22-371)
🪛 GitHub Check: codecov/patch
packages/core/src/Engine.ts
[warning] 262-263: packages/core/src/Engine.ts#L262-L263
Added lines #L262 - L263 were not covered by tests
e2e/case/material-blinn-phong.ts
[warning] 47-50: e2e/case/material-blinn-phong.ts#L47-L50
Added lines #L47 - L50 were not covered by tests
e2e/case/particleRenderer-dream.ts
[warning] 27-27: e2e/case/particleRenderer-dream.ts#L27
Added line #L27 was not covered by tests
e2e/case/spriteMask-customStencil.ts
[warning] 47-50: e2e/case/spriteMask-customStencil.ts#L47-L50
Added lines #L47 - L50 were not covered by tests
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: e2e (22.x)
🔇 Additional comments (22)
e2e/case/particleRenderer-dream.ts (3)
26-27: Verify the impact of removing WebGL mode specificationThe PR has removed the explicit WebGL1 mode setting, which might be related to the sRGB color space support. This change allows the engine to potentially use WebGL2 which has better native sRGB support, but it could also cause different rendering behavior.
Since this PR introduces breaking changes affecting colors, we should verify that this test still produces the expected visual output with the sRGB color space now being the default.
Has the visual output of this test been compared before and after the sRGB color space implementation to ensure consistency or account for intentional differences?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 27-27: e2e/case/particleRenderer-dream.ts#L27
Added line #L27 was not covered by tests
104-106: Verify color values with sRGB color spaceWith the introduction of sRGB color space support, the color values used in the particle system (defined as RGB with values between 0-1) may be interpreted differently. Ensure these color values still produce the intended visual effect with the new color space handling.
It would be valuable to document whether these colors were chosen with sRGB in mind or if adjustments are needed for the new color space support.
Also applies to: 177-179, 298-300
27-27: Regarding the static analysis warning about test coverageThe static analysis tool flagged line 27 as not covered by tests. However, this is likely a false positive since this is an E2E test file. The engine creation is being tested through the subsequent operations in this file.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 27-27: e2e/case/particleRenderer-dream.ts#L27
Added line #L27 was not covered by testspackages/core/src/Engine.ts (2)
54-55: LGTM - Good implementation of the ShaderMacro for sRGB support detectionThis macro addition follows the same pattern as the existing
_noDepthTextureMacroand will be used to conditionally compile shader code when sRGB support is not available.
256-256: Correctly setting depth texture color space to non-sRGBThe modification adds the
falseparameter forisSRGBColorSpace, which is appropriate since depth textures store distance information, not color data that would benefit from sRGB encoding.e2e/case/spriteMask-customStencil.ts (1)
47-50: Correctly configuring mask texture with isSRGBColorSpace: false.Setting
isSRGBColorSpaceto false for the mask texture is the right approach since mask textures contain data values rather than visual colors, and should not undergo sRGB/linear conversion.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 47-50: e2e/case/spriteMask-customStencil.ts#L47-L50
Added lines #L47 - L50 were not covered by testse2e/case/material-blinn-phong.ts (1)
47-50: Properly configuring normal map texture with isSRGBColorSpace: false.Setting
isSRGBColorSpaceto false for this texture is correct since it's used as a normal map (as seen in line 72 where it's assigned tomaterial.normalTexture). Normal maps contain directional data that should not undergo sRGB/linear conversion.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 47-50: e2e/case/material-blinn-phong.ts#L47-L50
Added lines #L47 - L50 were not covered by testspackages/loader/src/KTXLoader.ts (1)
26-26: Setting KTX textures to non-sRGB by default.The addition of
falseas theisSRGBColorSpaceparameter is appropriate for many KTX texture use cases that contain non-color data (e.g., normal maps, roughness, metallic maps). However, this could affect color textures loaded via KTX.Users who load color textures via KTX may need to manually set them to sRGB color space after loading. Consider documenting this behavior change in the release notes.
packages/core/src/mesh/BlendShapeManager.ts (1)
345-346: Correctly setting vertex data texture to non-sRGB.Setting
isSRGBColorSpaceto false for the blend shape texture array is appropriate since this texture stores vertex position/normal/tangent delta data rather than color information. These values should not undergo sRGB/linear conversion.packages/core/src/texture/TextureCube.ts (2)
19-21: Good addition of the sRGB color space parameter with appropriate documentation.The addition of the
isSRGBColorSpaceparameter with a default value oftruealigns well with the PR objective to support sRGB color space. This provides users with explicit control over color space handling for cube textures.
29-29: Properly initializing the new sRGB flag.The initialization of the
_isSRGBColorSpacemember variable ensures the class properly tracks the color space setting.packages/core/src/material/utils/PrefilteredDFG.ts (1)
13-21: Updated texture constructor with explicit color space settingThe
Texture2Dconstructor call has been modified to include two additional parameters: anundefinedvalue for theusageparameter andfalsefor theisSRGBColorSpaceparameter. This explicitly specifies that the prefiltered DFG lookup texture should use linear color space rather than sRGB.This is the right approach as DFG lookup textures contain numerical data (not visual colors) that should not undergo sRGB/linear conversion.
packages/core/src/mesh/SkinnedMeshRenderer.ts (1)
174-182: Updated joint texture constructor with explicit color space settingThe
Texture2Dconstructor call has been modified to include two additional parameters: anundefinedvalue for theusageparameter andfalsefor theisSRGBColorSpaceparameter. This explicitly specifies that the joint texture should use linear color space rather than sRGB.This is the correct approach since joint textures contain numerical transformation data that should not undergo sRGB/linear conversion, which would corrupt the skeletal animation.
tests/src/core/texture/Texture2D.test.ts (3)
29-30: Updated constructor with explicit color space parameterThe test case now includes additional parameters to match the updated
Texture2Dconstructor signature.
34-35: Updated unsupported format test with explicit color space parameterThe test case now includes additional parameters to match the updated
Texture2Dconstructor signature.
116-129: Added sRGB color space testsNew test suite that verifies default color space behavior and mipmap handling with sRGB textures.
The test checks three important behaviors:
- Confirms that textures use sRGB color space by default
- Verifies explicit sRGB setting works
- Validates that RGB textures with sRGB enabled have their mipmap count downgraded to 1 (likely a limitation)
tests/src/loader/GLTFLoader.test.ts (6)
80-94: Added additional textures for material testingThree new textures have been added to the test data to better test different texture roles in PBR materials.
This change supports testing different color space settings for various texture types in materials.
206-207: Updated texture reference for metallicRoughness textureChanged the texture index from 0 to 3 to reference a different test texture.
This allows testing different color space settings for this specific texture type.
213-214: Updated texture reference for normal textureChanged the texture index from 0 to 1 to reference a different test texture.
This allows testing different color space settings for normal maps.
217-218: Updated texture reference for occlusion textureChanged the texture index from 0 to 2 to reference a different test texture.
This allows testing different color space settings for occlusion maps.
441-442: Updated texture count assertionThe expected number of textures has been increased from 1 to 4 to match the expanded test data.
463-466: Added sRGB color space verification testsNew assertions to verify that different texture types use the appropriate color space:
- Base color texture uses sRGB color space
- Technical textures (roughness/metallic, normal, occlusion) use linear color space
This follows best practices in PBR rendering where color data should use sRGB while technical data that represents physical properties should use linear color space to avoid distortion.
| @@ -71,4 +79,6 @@ export interface Texture2DParams { | |||
| filterMode: TextureFilterMode; | |||
| /** Anisotropic level for texture. */ | |||
| anisoLevel: number; | |||
| /** Whether to use sRGB color space, default is true. */ | |||
There was a problem hiding this comment.
/** Whether to use sRGB color space or linear color space. @DefaultValue true */
| @@ -17,13 +17,13 @@ WebGLEngine.create({ canvas: "canvas", shaderLab }).then((engine) => { | |||
| engine.resourceManager | |||
| .load({ | |||
| type: AssetType.Project, | |||
| url: "https://mdn.alipayobjects.com/oasis_be/afts/file/A*o15SSopTBh0AAAAAAAAAAAAADkp5AQ/project.json" | |||
| url: "https://mdn.alipayobjects.com/oasis_be/afts/file/A*CbucQ5pF7wEAAAAAAAAAAAAAekp5AQ/project.json" | |||
There was a problem hiding this comment.
You should add a Project with a custom Shader
| @@ -94,6 +105,12 @@ export class Background { | |||
| constructor(private _engine: Engine) { | |||
| this._initMesh(_engine); | |||
| this._initMaterial(_engine); | |||
|
|
|||
| this._linearSolidColor = this._solidColor.toLinear(this.solidColor.clone()); | |||
There was a problem hiding this comment.
Inefficient and unreadable code：
/** @internal */
_linearSolidColor: Color = new Color();
this._solidColor.toLinear(this._linearSolidColor);
| @@ -38,8 +31,27 @@ export class Background { | |||
| /** @internal */ | |||
| _material: Material; | |||
|
|
|||
| /** @internal */ | |||
| _linearSolidColor: Color; | |||
There was a problem hiding this comment.
Only once per frame, so there may be no need to cache
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
Refactor
Tests