Texture support R8 and R8G8 Format and supportsTextureFormat
#2612
Conversation
WalkthroughThe changes expand texture handling throughout the codebase. A new texture entry "R8G8" has been introduced, along with new enum values. Core modules now include a static method to check texture format support, and multiple texture classes have been updated to accept an sRGB flag during instantiation. RHI/WebGL modules incorporate updated format detail functions, validation routines, and enhanced pixel storage alignment. An end-to-end test case has also been added for the new texture format. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Texture
participant SystemInfo
participant GLTexture
participant WebGL
Client->>Texture: Instantiate(texture parameters,<br>including isSRGBColorSpace)
Texture->>SystemInfo: supportsTextureFormat(engine, format)
SystemInfo-->>Texture: Return support status
Texture->>GLTexture: _getFormatDetail(format, isSRGBColorSpace, gl, isWebGL2)
GLTexture-->>Texture: Return format details
GLTexture->>GLTexture: _validate(texture, rhi)
GLTexture->>WebGL: Set pixelStorei(unpackAlignment)
WebGL-->>GLTexture: Acknowledge settings
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (5)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (4)
🪧 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 (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev/1.5 #2612 +/- ##
===========================================
- Coverage 68.59% 68.55% -0.05%
===========================================
Files 966 967 +1
Lines 101443 101465 +22
Branches 8714 8715 +1
===========================================
- Hits 69585 69557 -28
- Misses 31595 31643 +48
- Partials 263 265 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
packages/core/src/RenderPipeline/PipelineUtils.ts (1)
67-115: 🛠️ Refactor suggestionRename the local variable to avoid overshadowing the
needDepthTextureparameter.Within this block,
needDepthTextureis first used as a boolean parameter, but then declared again as aTexture2D | null. This overshadowing can cause confusion and potential bugs. Consider renaming it (e.g.,newDepthTexture) to maintain clarity:- const needDepthTexture = depthFormat + const newDepthTexture = depthFormat
🧹 Nitpick comments (22)
e2e/case/project-loader.ts (1)
25-26: Reordered updateForE2E callThe call to
updateForE2E(engine)has been moved to after retrieving the camera entity. This ensures that the engine is updated after all the necessary components are loaded and available.Consider adding a comment explaining why this reordering was necessary, especially since it might affect the test execution timing.
packages/math/src/Color.ts (1)
362-375: Added deprecated methods for backward compatibilityThe code properly maintains backward compatibility by adding deprecated versions of the old methods that call the new ones. This is a good practice that allows existing code to continue working while encouraging migration to the new terminology.
This is a well-implemented deprecation pattern. Consider adding a version number or timeline for when these deprecated methods will be removed in future releases to help users plan their migration.
packages/core/src/EngineSettings.ts (1)
4-4: Consider using a type alias for empty interfaceThe interface is now empty after removing the colorSpace property. According to the static analysis, an empty interface is equivalent to
{}.- export interface EngineSettings {} + export type EngineSettings = {};🧰 Tools
🪛 Biome (1.9.4)
[error] 4-4: An empty interface is equivalent to {}.
Safe fix: Use a type alias instead.
(lint/suspicious/noEmptyInterface)
packages/rhi-webgl/src/GLTexture2DArray.ts (1)
119-130: Consider avoiding @ts-ignore for accessing private propertiesThe validation method correctly handles sRGB format support checking, but it uses
@ts-ignoreto access what appears to be private properties of the texture object. Consider making these properties accessible through proper accessor methods or refactoring the design to avoid suppressing TypeScript type checking.- // @ts-ignore - const isSRGBColorSpace = texture._isSRGBColorSpace; + const isSRGBColorSpace = texture.isSRGBColorSpace; // Assuming a getter exists or is added- // @ts-ignore - texture._isSRGBColorSpace = false; + texture.setIsSRGBColorSpace(false); // Assuming a setter method exists or is addedpackages/rhi-webgl/src/GLRenderTarget.ts (1)
53-55: Added validation for sRGB color space with R8G8B8 format.This validation ensures that when using sRGB color space, only the R8G8B8A8 format is supported, not R8G8B8, which is a critical check for proper texture handling.
The error message could be more informative by explaining why R8G8B8 isn't supported with sRGB:
- throw new Error(`If you want to use sRGB color space, only R8G8B8A8 format is supported in RenderTarget`); + throw new Error(`R8G8B8 format doesn't support sRGB color space due to lack of alpha channel. Please use R8G8B8A8 format instead.`);packages/loader/src/KTXLoader.ts (1)
26-26: Added sRGB configuration to texture creationThe additional
falseparameter added to theTexture2Dconstructor configures the texture to not use sRGB color space by default. This is consistent with the changes in other files for standardizing color space handling.Consider using a named parameter or adding a comment to clarify that this parameter controls sRGB color space handling:
- const texture = new Texture2D(resourceManager.engine, width, height, engineFormat, mipmap, false); + const texture = new Texture2D(resourceManager.engine, width, height, engineFormat, mipmap, false /* isSRGBColorSpace */);packages/loader/src/EnvLoader.ts (1)
28-28: Updated TextureCube constructor call with additional parametersThe change adds parameters to the
TextureCubeconstructor, including afalsevalue for sRGB color space configuration, which aligns with the PR objective of improving texture format support.To improve code readability, consider using named parameters or adding a comment explaining what each parameter represents:
- texture ||= new TextureCube(engine, size, undefined, undefined, false); + texture ||= new TextureCube(engine, size, undefined /* format */, undefined /* mipmap */, false /* isSRGBColorSpace */);packages/core/src/particle/modules/shape/MeshShape.ts (4)
14-16: Availability of normal attribute.By design, this shape requires a mesh with valid normals. If the mesh lacks normal data, usage of this shape throws an error. Ensure that this requirement is clearly documented for end users.
57-76: Random index usage.Selecting a random vertex index throughout the mesh is correct for distributing particles. However, to avoid test flakiness, consider using a fixed seed for unit tests or mocking the random function to ensure reproducibility.
97-105: Use of string literal in throw.Use of
throwwith string literals is less conventional than throwing anErrorobject. Consider wrapping it in anErrorto provide a more detailed stack trace and standard error handling.- throw `Mesh must have ${vertexElement.attribute} attribute.`; + throw new Error(`Mesh must have ${vertexElement.attribute} attribute.`);
168-170: Test coverage suggestion.The
_cloneTomethod and the rest ofMeshShapewould benefit from dedicated tests (both for typical usage and edge cases). This helps ensure reliability when the shape is cloned or replaced at runtime.Would you like help creating corresponding unit tests for this new shape class?
packages/core/src/BasicResources.ts (1)
244-245: Consider wrapping switch case declarations in blocksThe texture declarations in each switch case should be wrapped in blocks to prevent potential scope issues between cases.
case TextureType.Texture2D: { const texture2D = new Texture2D(engine, 1, 1, format, false, isSRGBColorSpace); texture2D.setPixelBuffer(pixel); texture = texture2D; break; } case TextureType.Texture2DArray: { const texture2DArray = new Texture2DArray(engine, 1, 1, 1, format, false, isSRGBColorSpace); texture2DArray.setPixelBuffer(0, pixel); texture = texture2DArray; break; } 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; }Also applies to: 249-250, 254-255
🧰 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)
e2e/case/particleRenderer-force.ts (1)
280-353:createSparksParticle: Particle curves look reasonable.
- The curve definitions for force (e.g.,
forceX.curveMaxwith multipleCurveKeys) allow dynamic sparks movement.- Suggest adding specialized tests if not already covered for multi-key curves.
packages/rhi-webgl/src/GLTexture.ts (1)
26-142: Validate new texture formats (R8,R8G8) and sRGB logic.
The addition ofR8andR8G8formats aligns with the PR objectives to support these texture formats in WebGL. The internal format, base format, and unpack alignment are set correctly for WebGL2 only. The dynamic handling ofisSRGBColorSpacein other cases also appears consistent.Consider adding a warning or fallback path if
R8orR8G8are attempted in a WebGL1 context for clarity.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 56-57: packages/rhi-webgl/src/GLTexture.ts#L56-L57
Added lines #L56 - L57 were not covered by tests
[warning] 64-65: packages/rhi-webgl/src/GLTexture.ts#L64-L65
Added lines #L64 - L65 were not covered by tests
[warning] 72-73: packages/rhi-webgl/src/GLTexture.ts#L72-L73
Added lines #L72 - L73 were not covered by tests
[warning] 80-81: packages/rhi-webgl/src/GLTexture.ts#L80-L81
Added lines #L80 - L81 were not covered by tests
[warning] 88-89: packages/rhi-webgl/src/GLTexture.ts#L88-L89
Added lines #L88 - L89 were not covered by tests
[warning] 96-97: packages/rhi-webgl/src/GLTexture.ts#L96-L97
Added lines #L96 - L97 were not covered by tests
[warning] 104-106: packages/rhi-webgl/src/GLTexture.ts#L104-L106
Added lines #L104 - L106 were not covered by tests
[warning] 109-114: packages/rhi-webgl/src/GLTexture.ts#L109-L114
Added lines #L109 - L114 were not covered by tests
[warning] 127-133: packages/rhi-webgl/src/GLTexture.ts#L127-L133
Added lines #L127 - L133 were not covered by tests
[warning] 136-141: packages/rhi-webgl/src/GLTexture.ts#L136-L141
Added lines #L136 - L141 were not covered by testspackages/core/src/texture/TextureUtils.ts (1)
1-3: Static-only class usage:
All methods are static. As noted by the lint hint, you could switch to standalone functions if you prefer a lighter simplification. For now, this structure is acceptable.Consider converting
TextureUtilsinto a utility module of pure functions.packages/loader/src/gltf/parser/GLTFTextureParser.ts (1)
117-139: _isSRGBColorSpace method logic
You check multiple texture references (emissive, base color, sheen color, etc.) to decide if a texture needs sRGB color space. This is a thoughtful approach to glTF PBR definitions.Consider short-circuiting once a match is found for marginal performance gains—though likely negligible here.
packages/loader/src/ktx2/KTX2Loader.ts (4)
45-63: Validate new_capabilityMapentries and consider fallback logic.Defining the
_capabilityMapfor both linear and sRGB is beneficial. However, ensure that fallback logic gracefully handles unknown or unsupported formats when the GPU doesn’t have a matching capability.
93-98: Add additional error handling or logging if the parsed data is malformed.This code now returns an object containing
ktx2Container,engine,result, andtargetFormat. Consider preemptively validating thatktx2Containerandresultcontain consistent width/height/faces data, and log or throw if corrupted.
145-151: Consider clarifying the power-of-two checks for PVRTC fallback.The logic to fall back to
RGBA8if width/height are not powers of two (or if width ≠ height) is correct for PVRTC, but consider adding comments or inline documentation so future maintainers understand the requirement.🧰 Tools
🪛 Biome (1.9.4)
[error] 147-147: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
164-168: Address static lint warnings by referencing the class name instead ofthis.The lint tool warns that using
thisin a static context can be confusing. You can resolve this by replacingthis._detectSupportedFormatcalls and references tothis._capabilityMapwithKTX2Loader._detectSupportedFormatorKTX2Loader._capabilityMapto be more explicit.- const targetFormat = this._detectSupportedFormat(renderer, priorityFormats, isSRGB); + const targetFormat = KTX2Loader._detectSupportedFormat(renderer, priorityFormats, isSRGB);Also applies to: 172-172
packages/core/src/particle/modules/VelocityOverLifetimeModule.ts (2)
41-42: Ensure that ignoring clone for macros is intentional.Marking
_velocityMacroand_randomModeMacroas@ignoreCloneis typically correct because they are set dynamically. Just verify there’s no need to preserve their state across clones.
135-139: Initialize local variables together for clarity.You declared
velocityMacroandisRandomModeMacroseparately, but both default to null. Combining them might reduce repetition.
🛑 Comments failed to post (3)
e2e/case/texture-sRGB-KTX2.ts (1)
27-35: 🛠️ Refactor suggestion
Add error handling for texture loading
There's no error handling for the texture loading process. If the texture fails to load, the test will fail without a clear indication of what went wrong.
engine.resourceManager .load<Texture2D>({ url: "https://mdn.alipayobjects.com/oasis_be/afts/img/A*XmHPTYGREAIAAAAAAAAAAAAAekp5AQ/original/original.ktx2", type: AssetType.KTX2 }) .then((texture: Texture2D) => { const material = new UnlitMaterial(engine); material.baseTexture = texture; + }) + .catch((error) => { + console.error("Failed to load texture:", error); + // Consider adding a fallback or explicit test failure here });📝 Committable 8000 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.engine.resourceManager .load<Texture2D>({ url: "https://mdn.alipayobjects.com/oasis_be/afts/img/A*XmHPTYGREAIAAAAAAAAAAAAAekp5AQ/original/original.ktx2", type: AssetType.KTX2 }) .then((texture: Texture2D) => { const material = new UnlitMaterial(engine); material.baseTexture = texture; }) .catch((error) => { console.error("Failed to load texture:", error); // Consider adding a fallback or explicit test failure here });packages/core/src/texture/Texture.ts (1)
177-196: 💡 Verification agent
🧩 Analysis chain
Format validation and color space handling in constructor.
The updated constructor properly validates texture formats and initializes the color space property. The format validation is crucial for ensuring that only supported formats like R8 and R8G8 are used in appropriate contexts.
The format validation check throws an error when a format isn't supported, but this code path isn't covered by tests.
🏁 Script executed:
#!/bin/bash # Search for tests that verify texture format validation rg -l "TextureFormat.R8\s|TextureFormat.R8G8" --type ts --glob "*test*.ts"Length of output: 220
Action Required: Add Test Case for Unsupported Texture Formats
The constructor now correctly validates texture formats and sets the SRGB color space flag. However, although tests exist that reference supported formats (e.g., TextureFormat.R8 and TextureFormat.R8G8 in several test files), none of these tests explicitly cover the error path when an unsupported texture format is provided. Consider adding at least one unit test that verifies that an error is thrown when an unsupported texture format is used.
- File:
packages/core/src/texture/Texture.ts(Lines 177-196)- Test Suggestion: Create a new test case (or augment an existing one) to instantiate the texture with an invalid format and assert that the constructor throws the expected error.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 186-187: packages/core/src/texture/Texture.ts#L186-L187
Added lines #L186 - L187 were not covered by testspackages/rhi-webgl/src/GLTexture.ts (1)
565-574: 🛠️ Refactor suggestion
Coverage missing for new sRGB + mipmap constraint check.
Auto-generating mipmaps for sRGB textures is now restricted to [WebGL2 + R8G8B8A8], and the code logs a warning if unsupported. This code path is not test-covered based on static analysis hints.Add test cases to ensure this warning is triggered correctly under conditions that do not support sRGB mipmap generation.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/src/core/texture/Texture2D.test.ts(2 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
tests/src/core/texture/Texture2D.test.ts
[error] 119-119: Don't focus the test.
The 'only' method is often used for debugging or during implementation. It should be removed before deploying to production.
Consider removing 'only' to ensure all tests are executed.
Unsafe fix: Remove focus from test.
(lint/suspicious/noFocusedTests)
[error] 136-136: Don't focus the test.
The 'only' method is often used for debugging or during implementation. It should be removed before deploying to production.
Consider removing 'only' to ensure all tests are executed.
Unsafe fix: Remove focus from test.
(lint/suspicious/noFocusedTests)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: e2e (22.x)
🔇 Additional comments (2)
tests/src/core/texture/Texture2D.test.ts (2)
102-113: Great improvement to test case naming!Renaming the test from "读取成功" to "读取 R8G8B8A8 成功" improves clarity by specifying exactly which texture format is being tested. This change aligns well with the new format-specific test structure.
115-143: Excellent test coverage for new texture formatsThe addition of specific tests for R8 and R8G8 formats demonstrates thorough testing of the new functionality. The tests appropriately validate that both
setPixelBufferandgetPixelBufferoperations work correctly with these formats, which directly addresses the PR objectives.🧰 Tools
🪛 Biome (1.9.4)
[error] 119-119: Don't focus the test.
The 'only' method is often used for debugging or during implementation. It should be removed before deploying to production.
Consider removing 'only' to ensure all tests are executed.
Unsafe fix: Remove focus from test.(lint/suspicious/noFocusedTests)
[error] 136-136: Don't focus the test.
The 'only' method is often used for debugging or during implementation. It should be removed before deploying to production.
Consider removing 'only' to ensure all tests are executed.
Unsafe fix: Remove focus from test.(lint/suspicious/noFocusedTests)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/rhi-webgl/src/GLTextureCube.ts (1)
93-118: Consider adding UNPACK_ALIGNMENT to setImageSource method.The
setPixelBuffermethod now sets theUNPACK_ALIGNMENTparameter, but this setting is missing in thesetImageSourcemethod. For consistency across texture loading paths, consider adding the same alignment parameter here.setImageSource( face: TextureCubeFace, imageSource: TexImageSource, mipLevel: number, flipY: boolean, premultiplyAlpha: boolean, x: number, y: number ): void { const gl = this._gl; - const { baseFormat, dataType } = this._formatDetail; + const formatDetail = this._formatDetail; + const { baseFormat, dataType } = formatDetail; this._bind(); gl.pixelStorei(gl.UNPACK_FLIP_Y_WEBGL, +flipY); gl.pixelStorei(gl.UNPACK_PREMULTIPLY_ALPHA_WEBGL, +premultiplyAlpha); + gl.pixelStorei(gl.UNPACK_ALIGNMENT, formatDetail.unpackAlignment); gl.texSubImage2D( gl.TEXTURE_CUBE_MAP_POSITIVE_X + face, mipLevel, x || 0, y || 0, baseFormat, dataType, imageSource ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/rhi-webgl/src/GLTexture2DArray.ts(4 hunks)packages/rhi-webgl/src/GLTextureCube.ts(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/rhi-webgl/src/GLTexture2DArray.ts (1)
packages/rhi-webgl/src/WebGLGraphicDevice.ts (1)
gl(124-126)
🔇 Additional comments (7)
packages/rhi-webgl/src/GLTextureCube.ts (2)
37-38: Good refactoring for improved readability.Extracting
formatDetailto a local variable before destructuring is a good practice. It makes the code more readable and potentially more efficient by avoiding repeated property access tothis._formatDetail.
48-48: Important addition for proper texture handling.Setting the
UNPACK_ALIGNMENTparameter is crucial for correctly handling texture data, especially for the newR8andR8G8formats. This ensures proper memory alignment when uploading texture data to the GPU.packages/rhi-webgl/src/GLTexture2DArray.ts (5)
1-1: Good cleanup removing unused importRemoving the unused
TextureFormatimport aligns with best practices for keeping imports clean and reducing unused dependencies.
34-35: Improved variable handling for format detailsCreating a local variable for
_formatDetailbefore destructuring it improves code clarity and potentially reduces property access overhead.
44-44: Great addition of pixel alignment configurationAdding the
UNPACK_ALIGNMENTsetting is crucial for correctly handling the new texture formats. This ensures the pixel data is properly unpacked from memory before being uploaded to the GPU texture, which directly supports the PR objective of addingR8andR8G8format support.
122-133: Simplified validation looks correctThe validation method has been streamlined to focus only on sRGB validation, removing redundant WebGL2 and texture format checks. This aligns with the PR's goal of centralizing format support checks in
SystemInfo.supportsTextureFormat.The validation still properly handles the sRGB color space configuration and provides appropriate warnings when needed.
115-115:✅ Verification successful
Verify property name used for PACK_ALIGNMENT
The code sets
PACK_ALIGNMENTusingformatDetail.unpackAlignment. Confirm that using the "unpack" alignment value for packing operations is intentional and correct for all texture formats.
🏁 Script executed:
#!/bin/bash # Let's search the codebase to see if other files use the same pattern # or if there's a separate packAlignment property # Check if there's a different property for pack alignment rg -A 3 "packAlignment" --type ts echo "-----------------------------------------" # Check how PACK_ALIGNMENT is set in other files rg -A 2 "PACK_ALIGNMENT" --type tsLength of output: 7128
Final Verification: Using
unpackAlignmentfor both PACK and UNPACK is intentional
- The repository consistently uses the
unpackAlignmentproperty for configuring bothgl.UNPACK_ALIGNME 8000 NTandgl.PACK_ALIGNMENT(as seen in files such asGLTextureCube.ts,GLTexture.ts, andGLTexture2DArray.ts).- The type definition only provides an
unpackAlignmentproperty, with no separatepackAlignmentdefined.- This unified approach to alignment configuration confirms that using
formatDetail.unpackAlignmentwhen settinggl.PACK_ALIGNMENTis deliberate and correctly applied for all supported texture formats.
R8 and R8G8 FormatR8 and R8G8 Format and supportsTextureFormat
Please check if the PR fulfills these requirements
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
R8andR8G8Format to insteadAlpha8andLuminanceAlphain the future（Modern graphic API is not suggest this these format）SystemInfosupportsupportsTextureFormatgetPixelBufferand setsetPixelBufferwhen format is R8G8B8Summary by CodeRabbit
Summary by CodeRabbit
New Features
Tests