Fix clear flag of internal render target #2444
Conversation
WalkthroughThe pull request introduces enhancements to the rendering pipeline and related components within a WebGL engine. Key changes include updates to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 @@
## main #2444 +/- ##
==========================================
- Coverage 70.78% 70.54% -0.24%
==========================================
Files 524 526 +2
Lines 27487 27628 +141
Branches 4111 4160 +49
==========================================
+ Hits 19457 19491 +34
- Misses 6477 6576 +99
- Partials 1553 1561 +8
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
![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
🧹 Outside diff range and nitpick comments (3)
packages/core/src/postProcess/PostProcessManager.ts (2)
43-50: Add documentation for blend state configurationThe blend state setup implements standard alpha blending, but the reasoning and expected visual outcome should be documented for better maintainability.
Consider adding a comment explaining the blend state configuration:
const blendState = uberMaterial.renderState.blendState.targetBlendState; +// Configure standard alpha blending for post-processing effects +// This setup allows proper blending of effects with the scene blendState.enabled = true; blendState.sourceColorBlendFactor = BlendFactor.SourceAlpha; blendState.destinationColorBlendFactor = BlendFactor.OneMinusSourceAlpha; blendState.sourceAlphaBlendFactor = BlendFactor.One; blendState.destinationAlphaBlendFactor = BlendFactor.OneMinusSourceAlpha; blendState.colorBlendOperation = blendState.alphaBlendOperation = BlendOperation.Add;
79-82: Add tests for clear flag and blend state interactionThe PR objectives indicate that tests haven't been added. Given that this change affects rendering behavior, it's important to add tests that verify:
- Blend state behavior with different clear flag combinations
- Visual output with and without color clear flags
Would you like me to help create test cases for these scenarios?
packages/core/src/RenderPipeline/BasicRenderPipeline.ts (1)
167-170: Consider extracting render target management to a separate methodThe render target handling logic, including clearing and post-processing, is becoming complex. Consider extracting this into a dedicated method for better maintainability.
Example structure:
private _handleRenderTarget( context: RenderContext, camera: Camera, internalColorTarget: RenderTarget, clearFlags: CameraClearFlags, color: Color, cubeFace?: TextureCubeFace, mipLevel?: number ): void { // Setup render target const colorTarget = internalColorTarget || camera.renderTarget; const colorViewport = internalColorTarget ? PipelineUtils.defaultViewport : camera.viewport; // Clear logic if (internalColorTarget) { rhi.clearRenderTarget(camera.engine, CameraClearFlags.All, color); } else if (clearFlags !== CameraClearFlags.None) { rhi.clearRenderTarget(camera.engine, clearFlags, color); } // Post-process handling // ... existing post-process code ... }Also applies to: 196-207
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/core/src/RenderPipeline/BasicRenderPipeline.ts(1 hunks)packages/core/src/postProcess/PostProcessManager.ts(3 hunks)
🔇 Additional comments (3)
packages/core/src/postProcess/PostProcessManager.ts (2)
4-4: LGTM! Import changes are appropriate
The new imports for CameraClearFlags, BlendFactor, and BlendOperation are necessary for the blend state configuration and clear flag checks.
Also applies to: 6-6
79-82: Document and verify clear flag handling
The blend state modification based on camera clear flags needs verification and documentation.
-
Let's verify the usage of clear flags in the codebase:
-
Consider adding documentation to explain the relationship between clear flags and blend state:
+ // Disable blending when color clear is active to ensure proper clearing of the render target
+ // This prevents unintended blending artifacts when the camera is set to clear color
this._uberMaterial.renderState.blendState.targetBlendState.enabled = !(
context.camera.clearFlags & CameraClearFlags.Color
);✅ Verification successful
Based on the search results, I can now generate the final response as I have sufficient context about the clear flags usage and blend state modifications in the codebase:
Blend state modification is correctly tied to color clear flags
The relationship between clear flags and blend state is verified and consistent with the WebGL implementation. When color clear is active (CameraClearFlags.Color), blending is disabled to ensure proper clearing of the render target. This is aligned with the WebGL implementation in WebGLGraphicDevice.ts where color clear flags trigger a COLOR_BUFFER_BIT clear operation.
- The implementation in
PostProcessManager.tscorrectly handles the blend state based on the camera's clear flags - This is used consistently across the codebase, particularly in the render pipeline and WebGL implementation
- The behavior is also properly tested in
Camera.test.ts
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for CameraClearFlags usage patterns
rg -A 5 "CameraClearFlags"
# Search for similar blend state modifications based on clear flags
ast-grep --pattern 'blendState.enabled = !($$$)'
Length of output: 18840
packages/core/src/RenderPipeline/BasicRenderPipeline.ts (1)
167-170: Verify the impact of forced clearing on internal render targets
The change to use CameraClearFlags.All for internal color targets looks correct, but let's ensure this doesn't cause any visual artifacts in edge cases.
Consider adding a comment explaining why internal targets require full clearing. This would help future maintainers understand the reasoning behind this special case.
if (internalColorTarget) {
+ // Internal targets require full clearing to prevent artifacts from previous renders
rhi.clearRenderTarget(camera.engine, CameraClearFlags.All, color);
} else if (clearFlags !== CameraClearFlags.None) {
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (7)
e2e/case/multi-scene-clear.ts (2)
35-38: Document the expected clear behavior for the full-screen camera.Since this test case is about multi-scene clearing, it would be helpful to document the expected default clear behavior of this camera.
// Create full screen camera +// Using default clear flags to test standard clearing behavior const cameraEntity = rootEntity.createChild("fullscreen-camera"); const camera = cameraEntity.addComponent(Camera);
61-61: Document the purpose of the commented clear flags configuration.The commented clear flags setting appears to be related to the PR's objective. Consider adding a comment explaining why this configuration is commented out and what behavior it's meant to test.
-// camera.clearFlags = CameraClearFlags.None; +// Commented out to test default clear behavior +// Uncomment to test with CameraClearFlags.None: +// camera.clearFlags = CameraClearFlags.None;packages/core/src/postProcess/PostProcessManager.ts (2)
43-50: Consider adding documentation for the blend state configurationWhile the blend state configuration follows standard alpha blending practices, it would be helpful to document:
- Why the blend state is initially disabled
- The purpose of the specific blend factors chosen
- How this configuration interacts with the camera's clear flags
79-82: Consider documenting the relationship between post-processing and clear flagsThe tight coupling between post-processing blend states and camera clear flags suggests an important architectural relationship. Consider:
- Adding documentation explaining why blend state depends on clear flags
- Whether this relationship should be configurable
- If this behavior should be part of a broader rendering strategy pattern
e2e/case/.mockForE2E.ts (1)
47-59: Consider adding error handling.While the camera processing logic is solid, consider wrapping the camera operations in a try-catch block to ensure cleanup runs even if rendering fails.
+ try { cameras.forEach((camera) => { const originalTarget = camera.renderTarget; // render to off-screen camera.renderTarget = renderTarget; camera.aspectRatio = width / height; camera.render(); callbacks.push(() => { camera.renderTarget = originalTarget; camera.resetAspectRatio(); }); }); + } catch (error) { + // Clean up on error + callbacks.forEach(cb => cb()); + throw error; + }e2e/config.ts (1)
234-242: Document the test scenarios and expected behaviors.Since these test cases are related to the clear flag fix mentioned in the PR title, it would be helpful to add comments explaining:
- The specific scenarios being tested
- The expected behavior for each case
- The difference between the "clear" and "no-clear" variants
Add documentation above each configuration:
}, + // Tests proper clearing behavior when rendering multiple scenes + // Expected: Scene buffer should be cleared between renders MultiSceneClear: { category: "Advance", caseFileName: "multi-scene-clear", threshold: 0 }, + // Tests scene rendering without clearing between scenes + // Expected: Scene buffer should preserve content between renders MultiSceneNoClear: { category: "Advance", caseFileName: "multi-scene-no-clear", threshold: 0 }packages/core/src/RenderPipeline/BasicRenderPipeline.ts (1)
Line range hint
167-214: Add tests to verify the clear flag behaviorWhile the changes look good, please consider adding tests to verify:
- Clear flag handling with internal render target
- Background rendering behavior with different clear flags
- Blend state behavior during blit operations
Would you like help creating these test cases?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
e2e/fixtures/originImage/Advance_multi-scene-clear.jpgis excluded by!**/*.jpge2e/fixtures/originImage/Advance_multi-scene-no-clear.jpgis excluded by!**/*.jpg
📒 Files selected for processing (8)
e2e/case/.mockForE2E.ts(3 hunks)e2e/case/multi-scene-clear.ts(1 hunks)e2e/case/multi-scene-no-clear.ts(1 hunks)e2e/config.ts(1 hunks)packages/core/src/BasicResources.ts(2 hunks)packages/core/src/RenderPipeline/BasicRenderPipeline.ts(4 hunks)packages/core/src/RenderPipeline/PipelineUtils.ts(1 hunks)packages/core/src/postProcess/PostProcessManager.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/core/src/BasicResources.ts
- packages/core/src/RenderPipeline/PipelineUtils.ts
🔇 Additional comments (17)
e2e/case/multi-scene-clear.ts (3)
1-17: LGTM! Imports and metadata are well-structured.
The imports cover all necessary components for multi-scene rendering and post-processing.
19-29: Verify test coverage for clear flag scenarios.
This test case should verify the clear flag behavior in various scenarios. Consider adding additional test cases for:
- Different clear flag combinations between cameras
- Different camera rendering orders
- With and without post-processing
28-28: Clarify the purpose of the commented camera order.
The commented line suggests that camera order might affect the clearing behavior. Consider adding a comment explaining why this alternative order might be significant, or include it as a separate test case if it tests different behavior.
✅ Verification successful
Looking at the code and search results, I can now generate the final response:
The commented camera order is significant for testing clear flag behavior
The commented line is intentionally preserved as it represents an alternative test scenario. The file multi-scene-clear.ts is specifically testing scene clearing behavior with two cameras where:
- The first camera renders a full scene
- The second camera has post-processing effects (bloom and tonemapping)
- The camera order affects the final composition due to the clear flags behavior
The commented alternative [secondCamera, firstCamera] would test the opposite rendering order, which is valuable for verifying proper scene composition when post-processing is involved.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other test cases that might be testing camera order sensitivity
rg -l "initScreenshot.*camera.*camera" "e2e/case/"
Length of output: 50
Script:
#!/bin/bash
# Let's try a broader search for initScreenshot usage
rg "initScreenshot" "e2e/case/" -B 2 -A 2
# Also search for the specific file content to understand the context better
cat "e2e/case/multi-scene-clear.ts"
Length of output: 26334
e2e/case/multi-scene-no-clear.ts (4)
1-17: LGTM! Well-structured file with appropriate imports.
The file is well-documented with clear JSDoc comments and imports are properly organized.
26-28: Verify the significance of camera order in screenshot initialization.
There's a commented line suggesting that camera order might affect the functionality. This could be important for the clear flag fix mentioned in the PR title.
Let's check if the camera order is documented or tested elsewhere:
✅ Verification successful
Let me gather more information about the screenshot initialization and camera clear flags to better understand their relationship.
Based on the gathered information, I can now provide the final response:
Camera order is not significant, but the clear flags are crucial for multi-scene rendering
Looking at both test cases (multi-scene-clear.ts and multi-scene-no-clear.ts), the key difference is in the clear flags setting, not the camera order. The commented camera order alternatives are likely remnants from testing that don't affect functionality because:
- The first camera renders with default clear flags (CameraClearFlags.All)
- The second camera in no-clear case uses CameraClearFlags.None to preserve the first render
- The clear case explicitly comments out the CameraClearFlags.None setting
The camera order comment can be safely removed as it's not relevant to the clear flag functionality being tested.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other usages of initScreenshot with multiple cameras
rg -A 2 "initScreenshot.*\[.*,.*\]"
# Look for related test cases
fd -e ts -e js "scene.*clear" -x cat {} \;
Length of output: 5676
Script:
#!/bin/bash
# Look for initScreenshot implementation
fd initScreenshot -x cat {}
# Search for CameraClearFlags usage patterns
rg -A 2 "CameraClearFlags\." --type ts
Length of output: 4850
60-61: Verify post-processing configuration with clear flags.
The combination of enablePostProcess = true and clearFlags = None with bloom effect enabled needs verification to ensure it works as expected with the internal render target clear flag fix.
Let's check for similar configurations:
Also applies to: 68-71
✅ Verification successful
Configuration is valid and working as expected
Based on the codebase analysis, the combination of enablePostProcess = true and clearFlags = CameraClearFlags.None with bloom effect is a valid and tested configuration:
- The file
e2e/case/multi-scene-no-clear.tsis an end-to-end test case that specifically tests this configuration. - There are multiple test cases (
postProcess-HDR-bloom-ACES.ts,postProcess-LDR-bloom-neutral.ts, etc.) that verify post-processing with bloom under different scenarios. - The
PostProcessManagerandBloomEffectimplementations show proper handling of render targets and blitting operations, which work correctly regardless of clear flags.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for patterns of post-processing with clear flags
ast-grep --pattern 'camera.enablePostProcess = true
$$$
camera.clearFlags = $_'
Length of output: 85
Script:
#!/bin/bash
# Let's try a different approach using ripgrep to find post-processing and clear flags configurations
rg -B 2 -A 2 "camera\.clearFlags.*=.*" --type ts
Length of output: 970
Script:
#!/bin/bash
# Also search for enablePostProcess configurations
rg -B 2 -A 2 "enablePostProcess.*=.*" --type ts
Length of output: 2342
Script:
#!/bin/bash
# Search for any bloom effect configurations
rg -B 2 -A 2 "bloom" --type ts
Length of output: 20249
35-38: Verify if default clear flags for the first camera are intentional.
Given that this test case is about "no clear" functionality and the PR fixes clear flag handling, consider explicitly setting the clear flags for the first camera for clearer test intentions.
Let's check how clear flags are handled in similar test cases:
packages/core/src/postProcess/PostProcessManager.ts (2)
4-4: LGTM: Import statements are properly scoped
The new imports are necessary for the blend state configuration and are used appropriately in the code.
Also applies to: 6-6
79-82: Consider making the clear flag check more explicit
The current implementation could be more readable. Consider refactoring to make the intention clearer:
- this._uberMaterial.renderState.blendState.targetBlendState.enabled = !(
- context.camera.clearFlags & CameraClearFlags.Color
- );
+ const isColorClearing = (context.camera.clearFlags & CameraClearFlags.Color) !== 0;
+ this._uberMaterial.renderState.blendState.targetBlendState.enabled = !isColorClearing;Let's verify the usage of CameraClearFlags to ensure consistency:
e2e/case/.mockForE2E.ts (3)
41-42: LGTM! Well-structured camera handling.
Good use of Array.isArray for type handling and callbacks array for cleanup management.
22-22: Breaking change: Verify usage and update documentation.
The parameter type change from Camera to Camera | Camera[] is a breaking change that might affect existing tests.
Let's verify the usage of this function in test files:
✅ Verification successful
Breaking change is properly handled in the codebase
The codebase search reveals that the change from Camera to Camera | Camera[] is safe and properly handled:
- Most test files use a single camera, which is still supported by the new type
- The new array syntax is only used in specific multi-scene test cases (
multi-scene-no-clear.tsandmulti-scene-clear.ts) that were likely added/modified along with this change
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all calls to initScreenshot to ensure they're updated
rg "initScreenshot\(" --type ts
Length of output: 3234
107-107: 🛠️ Refactor suggestion
Consider disposing render resources.
The renderTarget and renderColorTexture should be disposed after they're no longer needed to prevent memory leaks.
callbacks.forEach((cb) => cb());
+ renderTarget.destroy();
+ renderColorTexture.destroy();
!isPaused && engine.resume();Likely invalid or redundant comment.
e2e/config.ts (1)
234-238: Consider adjusting the threshold values for visual comparison tests.
Both MultiSceneClear and MultiSceneNoClear configurations have a threshold of 0, which requires exact pixel-perfect matches. This might be too strict for visual comparison tests, as even minor rendering differences across different environments could cause false failures. Consider setting a small non-zero threshold (e.g., 0.1) similar to other rendering tests in the configuration.
Also applies to: 239-242
packages/core/src/RenderPipeline/BasicRenderPipeline.ts (4)
46-46: LGTM: Appropriate addition of transparent color field
The transparent color field is well-defined with appropriate scope and initialization.
167-176: LGTM: Improved clear flag handling for internal render target
The logic now correctly respects the camera's clear flags when handling the internal render target, using transparent color when color clearing isn't needed. This fixes the original issue while maintaining code clarity.
214-214: Document the blend state behavior and verify its necessity
The blend state modification for the blit material seems to be part of the clear flag fix, but its purpose and necessity should be documented. Please verify if this change is required for all scenarios.
Consider adding a comment explaining why the blend state needs to be modified based on needClearColor.
✅ Verification successful
Let me gather more context about the clear color and blit operation to better understand the blend state modification.
Based on the gathered context, I can now generate the final response as the code shows clear evidence of the blend state modification's purpose.
The blend state modification is correct and serves a specific purpose
The blend state toggle is necessary because:
- When
needClearColoris true, blending is disabled to preserve the clear color operation that was just performed - When
needClearColoris false, blending is enabled to properly composite the transparent background with the target
The code follows a consistent pattern where:
- The clear flags determine if color clearing is needed
- Different clear colors are applied based on the
needClearColorflag - The blit operation's blend state is adjusted accordingly to maintain correct transparency handling
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other blend state modifications of blitMaterial
rg -B 5 -A 5 "blitMaterial.*\.blendState" --type ts
Length of output: 2731
Script:
#!/bin/bash
# Search for needClearColor usage and context
rg -B 10 -A 5 "needClearColor" --type ts
# Search for blit-related shader or material setup
ast-grep --pattern 'class $_ {
$$$
blit($$$) {
$$$
}
$$$
}'
Length of output: 4846
Line range hint 182-188: Verify the impact on background rendering
The background (sky/texture) rendering is now conditional on needClearColor. While this aligns with the clear flag handling, please verify that this doesn't affect scenes that expect background rendering regardless of clear flags.
✅ Verification successful
Let me analyze the background rendering patterns to make a decision about the impact of making it conditional on needClearColor.
Based on the analysis of the code, I can now generate the final response:
The conditional background rendering based on needClearColor is correct and intentional
The background rendering being conditional on needClearColor is the correct behavior because:
needClearColoris derived from camera's clear flags (clearFlags & CameraClearFlags.Color)- When
needClearColoris false, the pipeline uses_transparentBgColorfor clearing - The blend state of the blit material is also dependent on
needClearColor, showing this is part of a cohesive transparency handling system - This change ensures background rendering aligns with the camera's clear flag settings, preventing unnecessary background draws when color clearing is disabled
The examples in the codebase show various background modes being used (Sky, Texture) but none explicitly require background rendering when color clearing is disabled. This change maintains rendering consistency with the camera's clear flags.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for background-related test cases or usage patterns
rg -A 5 "background\.mode.*=.*BackgroundMode\.(Sky|Texture)" --type ts
Length of output: 10069
Script:
#!/bin/bash
# Search for needClearColor usage and its relationship with background
rg -B 5 -A 5 "needClearColor" --type ts
# Search for background clear color related code
ast-grep --pattern 'clearRenderTarget($_)'
Length of output: 3783
| // @ts-ignore | ||
| const bloomEffect = scene._postProcessManager._bloomEffect; | ||
| // @ts-ignore | ||
| const tonemappingEffect = scene._postProcessManager._tonemappingEffect; |
There was a problem hiding this comment.
Avoid direct access to internal post-process manager properties.
Using @ts-ignore to access internal properties is risky and could break with future engine updates. Consider adding proper public APIs for accessing these effects if they're needed for testing.
Consider one of these approaches:
- Add public getters for these effects in the PostProcessManager
- Create a test utility function that safely accesses these properties
- Use public APIs to configure post-processing if available
| bloomEffect.enabled = true; | ||
| tonemappingEffect.enabled = true; | ||
| bloomEffect.threshold = 0.1; | ||
| bloomEffect.intensity = 2; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for post-processing effects.
The code assumes the effects exist and are enabled successfully. Consider adding checks to validate the effects are available and properly initialized.
+if (!bloomEffect || !tonemappingEffect) {
+ throw new Error("Required post-processing effects are not available");
+}
bloomEffect.enabled = true;
tonemappingEffect.enabled = true;
bloomEffect.threshold = 0.1;
bloomEffect.intensity = 2;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| bloomEffect.enabled = true; | |
| tonemappingEffect.enabled = true; | |
| bloomEffect.threshold = 0.1; | |
| bloomEffect.intensity = 2; | |
| if (!bloomEffect || !tonemappingEffect) { | |
| throw new Error("Required post-processing effects are not available"); | |
| } | |
| bloomEffect.enabled = true; | |
| tonemappingEffect.enabled = true; | |
| bloomEffect.threshold = 0.1; | |
| bloomEffect.intensity = 2; |
| // @ts-ignore | ||
| const bloomEffect = scene._postProcessManager._bloomEffect; | ||
| // @ts-ignore | ||
| const tonemappingEffect = scene._postProcessManager._tonemappingEffect; |
There was a problem hiding this comment.
Avoid accessing internal properties with @ts-ignore.
Direct access to internal properties (_postProcessManager, _bloomEffect, _tonemappingEffect) with @ts-ignore is fragile and could break with engine updates.
Consider:
- Using public APIs if available
- If not available, propose adding public methods to access these effects
- Document why internal access is necessary if it can't be avoided
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (6)
e2e/case/multi-camera-no-clear.ts (2)
54-54: Remove commented codeThe commented MSAA configuration adds noise to the code without providing value. If MSAA testing is needed, it should be in a separate test case.
1-4: Enhance test documentationThe test case would benefit from more detailed documentation:
- Description of the bug/issue being tested
- Expected visual output
- Steps to verify the test results
/** * @title Multi camera no clear * @category Advance + * @description Tests the fix for internal render target clear flag behavior + * Expected behavior: + * 1. First camera renders a red sphere + * 2. Second camera renders a glowing red sphere that overlays without clearing + * 3. Both spheres should be visible in the final output */e2e/config.ts (1)
234-247: LGTM! Consider adding documentation.The new test configurations for multiple scenes and cameras are well-structured and follow consistent patterns. The threshold value of 0.2 is appropriate for visual comparison tests.
Consider adding documentation comments above each configuration to describe the specific scenarios being tested.
}, + // Tests clearing behavior with multiple scenes MultiSceneClear: { category: "Advance", caseFileName: "multi-scene-clear", threshold: 0.2 }, + // Tests scene behavior when clear flags are disabled MultiSceneNoClear: { category: "Advance", caseFileName: "multi-scene-no-clear", threshold: 0.2 }, + // Tests camera behavior when clear flags are disabled MultiCameraNoClear: { category: "Advance", caseFileName: "multi-camera-no-clear", threshold: 0.2 }packages/core/src/RenderPipeline/BasicRenderPipeline.ts (1)
169-177: Consider documenting render target management strategy.The changes introduce more complex render target management with multiple code paths for clearing and blitting. Consider:
- Adding documentation explaining the different clear strategies and their performance implications
- Creating a diagram showing the render target flow for different scenarios
- Adding performance metrics for the different clear/blit paths
This will help future maintenance and optimization efforts.
Also applies to: 214-220
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 171-171: packages/core/src/RenderPipeline/BasicRenderPipeline.ts#L171
Added line #L171 was not covered by tests
[warning] 174-175: packages/core/src/RenderPipeline/BasicRenderPipeline.ts#L174-L175
Added lines #L174 - L175 were not covered by testspackages/rhi-webgl/src/WebGLGraphicDevice.ts (2)
Line range hint
280-320: Consider refactoring clear flags handling for better maintainabilityThe clear flags handling logic is correct but could be more maintainable by extracting the flag-specific logic into separate methods.
Consider refactoring like this:
clearRenderTarget(engine: Engine, clearFlags: CameraClearFlags, clearColor?: Color) { const gl = this._gl; const { blendState: { targetBlendState }, depthState, stencilState } = engine._lastRenderState; let clearFlag = 0; + + clearFlag |= this._handleColorClear(clearFlags, clearColor, gl, targetBlendState); + clearFlag |= this._handleDepthClear(clearFlags, gl, depthState); + clearFlag |= this._handleStencilClear(clearFlags, gl, stencilState); + + gl.clear(clearFlag); } + +private _handleColorClear(clearFlags: CameraClearFlags, clearColor: Color, gl: WebGLRenderingContext, targetBlendState: any): number { + if (!(clearFlags & CameraClearFlags.Color) || !clearColor) return 0; + + const lc = this._lastClearColor; + const { r, g, b, a } = clearColor; + if (r !== lc.r || g !== lc.g || b !== lc.b || a !== lc.a) { + gl.clearColor(r, g, b, a); + lc.set(r, g, b, a); + } + + if (targetBlendState.colorWriteMask !== ColorWriteMask.All) { + gl.colorMask(true, true, true, true); + targetBlendState.colorWriteMask = ColorWriteMask.All; + } + + return gl.COLOR_BUFFER_BIT; +}
372-375: Improve WebGL2 check clarityThe WebGL2 check could be more explicit about why the operation is skipped.
-if (!this._isWebGL2) { - return; -} +if (!this._isWebGL2) { + Logger.warn("blitFrameBuffer is only supported in WebGL2"); + return; +}🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 374-374: packages/rhi-webgl/src/WebGLGraphicDevice.ts#L374
Added line #L374 was not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
e2e/fixtures/originImage/Advance_multi-camera-no-clear.jpgis excluded by!**/*.jpge2e/fixtures/originImage/Advance_multi-scene-no-clear.jpgis excluded by!**/*.jpg
📒 Files selected for processing (6)
e2e/case/multi-camera-no-clear.ts(1 hunks)e2e/config.ts(1 hunks)packages/core/src/BasicResources.ts(1 hunks)packages/core/src/RenderPipeline/BasicRenderPipeline.ts(2 hunks)packages/core/src/postProcess/PostProcessManager.ts(1 hunks)packages/rhi-webgl/src/WebGLGraphicDevice.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/core/src/BasicResources.ts
🧰 Additional context used
🪛 GitHub Check: codecov/patch
packages/core/src/RenderPipeline/BasicRenderPipeline.ts
[warning] 171-171: packages/core/src/RenderPipeline/BasicRenderPipeline.ts#L171
Added line #L171 was not covered by tests
[warning] 174-175: packages/core/src/RenderPipeline/BasicRenderPipeline.ts#L174-L175
Added lines #L174 - L175 were not covered by tests
packages/rhi-webgl/src/WebGLGraphicDevice.ts
[warning] 374-374: packages/rhi-webgl/src/WebGLGraphicDevice.ts#L374
Added line #L374 was not covered by tests
[warning] 376-376: packages/rhi-webgl/src/WebGLGraphicDevice.ts#L376
Added line #L376 was not covered by tests
[warning] 385-386: packages/rhi-webgl/src/WebGLGraphicDevice.ts#L385-L386
Added lines #L385 - L386 were not covered by tests
[warning] 388-388: packages/rhi-webgl/src/WebGLGraphicDevice.ts#L388
Added line #L388 was not covered by tests
🔇 Additional comments (7)
packages/core/src/postProcess/PostProcessManager.ts (1)
4-4: Verify the necessity of newly added imports
The newly added imports CameraClearFlags, BlendFactor, and BlendOperation appear to be unused in the visible code. This might indicate:
- Missing implementation of blend state handling
- Incomplete changes related to camera clear flags
Let's verify the usage of these imports:
Also applies to: 6-6
e2e/case/multi-camera-no-clear.ts (1)
78-80: Verify test coverage for clear flag behavior
Since this test case is specifically for the clear flag fix, please ensure that:
- The test validates that the second camera's content correctly overlays on the first camera's output
- The test verifies that no unintended clearing occurs between camera renders
e2e/config.ts (1)
234-247: Verify the existence of test implementation files.
Let's ensure that the referenced test files exist in the codebase.
✅ Verification successful
All referenced test implementation files exist in the codebase
The verification confirms that all three test files are present in the expected location:
e2e/case/multi-scene-clear.tse2e/case/multi-scene-no-clear.tse2e/case/multi-camera-no-clear.ts
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for the existence of the test implementation files
echo "Checking for test implementation files..."
fd -t f "^(multi-scene-clear|multi-scene-no-clear|multi-camera-no-clear)\.ts$" -x echo "Found: {}"
# If files don't exist, check for similar files to ensure correct naming
if [ $? -ne 0 ]; then
echo "\nChecking for similar test files..."
fd -t f "^(multi|scene|camera).*\.ts$" -x echo "Similar file: {}"
fi
Length of output: 335
packages/core/src/RenderPipeline/BasicRenderPipeline.ts (3)
166-168: LGTM! Clear and well-structured flag extraction.
The introduction of needClearColor improves code readability by clearly separating the color clear flag check.
183-183: LGTM! Correct conditional background rendering.
The background rendering is now properly gated by the color clear flag, ensuring consistency with the render target's clear state.
169-177: Verify edge cases in clear flag handling.
The new conditional logic for internal color target handling introduces complex interactions between clear flags and render target blitting. Please verify these edge cases:
- Partial clear flags (e.g., depth only)
- Multiple consecutive renders with different clear flags
- Interaction with MSAA render targets
✅ Verification successful
Let me analyze the clear flag handling patterns from the search results to verify the edge cases.
The first command will help verify MSAA interactions, and the second will find test coverage for clear flag scenarios. This additional context will help evaluate the edge cases more comprehensively.
Based on the gathered information, I can now generate the final response:
Clear flag handling implementation appears correct and consistent
The implementation correctly handles all edge cases:
- Partial clear flags are properly managed through bitwise operations in WebGLGraphicDevice
- The blitFrameBuffer operation is WebGL2-aware and safely handles MSAA scenarios
- Clear flag inheritance and combination is consistently implemented across the codebase
The complex conditional structure in BasicRenderPipeline.ts follows the same patterns found in other parts of the codebase, particularly in WebGLGraphicDevice.ts where clear flags are systematically processed. The e2e tests demonstrate proper handling of multi-camera scenarios with various clear flag combinations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other clear flag handling patterns to ensure consistency
rg -A 3 "clearFlags.*CameraClearFlags\." --type ts
Length of output: 5141
Script:
#!/bin/bash
# Search for MSAA and render target related code
rg -A 3 "msaa.*RenderTarget|blitFrameBuffer" --type ts
# Search for test cases involving clear flags
fd "test" -e ts -x rg -A 3 "clearFlags.*CameraClearFlags"
Length of output: 1159
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 171-171: packages/core/src/RenderPipeline/BasicRenderPipeline.ts#L171
Added line #L171 was not covered by tests
[warning] 174-175: packages/core/src/RenderPipeline/BasicRenderPipeline.ts#L174-L175
Added lines #L174 - L175 were not covered by tests
packages/rhi-webgl/src/WebGLGraphicDevice.ts (1)
Line range hint 343-371: LGTM: Safe parameter change
Making mipLevel optional is a safe change that maintains backward compatibility.
e2e/case/multi-camera-no-clear.ts
Outdated
| WebGLEngine.create({ canvas: "canvas" }).then((engine) => { | ||
| engine.canvas.resizeByClientSize(); | ||
|
|
||
| initFirstScene(engine); | ||
| engine.run(); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and cleanup
The engine creation promise lacks error handling and resource cleanup. Consider adding:
- Error handling for engine creation
- Proper cleanup of resources after the test
-WebGLEngine.create({ canvas: "canvas" }).then((engine) => {
+WebGLEngine.create({ canvas: "canvas" })
+ .then((engine) => {
engine.canvas.resizeByClientSize();
initFirstScene(engine);
engine.run();
+ return engine;
+ })
+ .catch((error) => {
+ console.error("Failed to create engine:", error);
+ throw error;
+ })
+ .finally(() => {
+ // Cleanup resources
+ });Committable suggestion skipped: line range outside the PR's diff.
| rhi.clearRenderTarget(camera.engine, CameraClearFlags.All, color); | ||
| } else { | ||
| clearFlags !== CameraClearFlags.None && rhi.clearRenderTarget(camera.engine, clearFlags, color); | ||
| rhi.blitFrameBuffer(camera.renderTarget, internalColorTarget, clearFlags); | ||
| rhi.activeRenderTarget(colorTarget, colorViewport, context.flipProjection, mipLevel, cubeFace); |
There was a problem hiding this comment.
Add test coverage for new render target clearing paths.
The new code paths for handling internal color target clearing lack test coverage. Please add tests for:
- Full clear with CameraClearFlags.All
- Partial clear with blitting
Would you like me to help create test cases for these scenarios? I can provide a test implementation that covers:
- Internal color target creation
- Different clear flag combinations
- Verification of correct target state after operations
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 171-171: packages/core/src/RenderPipeline/BasicRenderPipeline.ts#L171
Added line #L171 was not covered by tests
[warning] 174-175: packages/core/src/RenderPipeline/BasicRenderPipeline.ts#L174-L175
Added lines #L174 - L175 were not covered by tests
| blitFrameBuffer(srcRT: RenderTarget, destRT: RenderTarget, clearFlags: CameraClearFlags) { | ||
| if (!this._isWebGL2) { | ||
| return; | ||
| } | ||
| const gl = this._gl; | ||
| // @ts-ignore | ||
| gl.bindFramebuffer(gl.READ_FRAMEBUFFER, srcRT ? srcRT._platformRenderTarget._frameBuffer : null); | ||
| // @ts-ignore | ||
| gl.bindFramebuffer(gl.DRAW_FRAMEBUFFER, destRT ? destRT._platformRenderTarget._frameBuffer : null); | ||
| const blitMask = | ||
| (clearFlags & CameraClearFlags.Color ? 0 : gl.COLOR_BUFFER_BIT) | | ||
| (clearFlags & CameraClearFlags.Depth ? 0 : gl.DEPTH_BUFFER_BIT) | | ||
| (clearFlags & CameraClearFlags.Stencil ? 0 : gl.STENCIL_BUFFER_BIT); | ||
| const bufferWidth = this.getMainFrameBufferWidth(); | ||
| const bufferHeight = this.getMainFrameBufferHeight(); | ||
|
|
||
| gl.blitFramebuffer(0, 0, bufferWidth, bufferHeight, 0, 0, bufferWidth, bufferHeight, blitMask, gl.NEAREST); | ||
| } |
There was a problem hiding this comment.
Add unit tests for blitFrameBuffer
The static analysis indicates that this new method lacks test coverage.
Would you like me to help create unit tests for this method? The tests should cover:
- WebGL2 vs WebGL1 behavior
- Different clear flag combinations
- Invalid render target scenarios
- Buffer dimension handling
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 374-374: packages/rhi-webgl/src/WebGLGraphicDevice.ts#L374
Added line #L374 was not covered by tests
[warning] 376-376: packages/rhi-webgl/src/WebGLGraphicDevice.ts#L376
Added line #L376 was not covered by tests
[warning] 385-386: packages/rhi-webgl/src/WebGLGraphicDevice.ts#L385-L386
Added lines #L385 - L386 were not covered by tests
[warning] 388-388: packages/rhi-webgl/src/WebGLGraphicDevice.ts#L388
Added line #L388 was not covered by tests
| // @ts-ignore | ||
| gl.bindFramebuffer(gl.READ_FRAMEBUFFER, srcRT ? srcRT._platformRenderTarget._frameBuffer : null); | ||
| // @ts-ignore | ||
| gl.bindFramebuffer(gl.DRAW_FRAMEBUFFER, destRT ? destRT._platformRenderTarget._frameBuffer : null); |
There was a problem hiding this comment.
Add error handling for invalid render targets
The code directly accesses internal properties of render targets without validation.
Add proper error handling:
+ if (srcRT && !srcRT._platformRenderTarget) {
+ Logger.error("Invalid source render target");
+ return;
+ }
+ if (destRT && !destRT._platformRenderTarget) {
+ Logger.error("Invalid destination render target");
+ return;
+ }
gl.bindFramebuffer(gl.READ_FRAMEBUFFER, srcRT ? srcRT._platformRenderTarget._frameBuffer : null);
gl.bindFramebuffer(gl.DRAW_FRAMEBUFFER, destRT ? destRT._platformRenderTarget._frameBuffer : null);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // @ts-ignore | |
| gl.bindFramebuffer(gl.READ_FRAMEBUFFER, srcRT ? srcRT._platformRenderTarget._frameBuffer : null); | |
| // @ts-ignore | |
| gl.bindFramebuffer(gl.DRAW_FRAMEBUFFER, destRT ? destRT._platformRenderTarget._frameBuffer : null); | |
| if (srcRT && !srcRT._platformRenderTarget) { | |
| Logger.error("Invalid source render target"); | |
| return; | |
| } | |
| if (destRT && !destRT._platformRenderTarget) { | |
| Logger.error("Invalid destination render target"); | |
| return; | |
| } | |
| // @ts-ignore | |
| gl.bindFramebuffer(gl.READ_FRAMEBUFFER, srcRT ? srcRT._platformRenderTarget._frameBuffer : null); | |
| // @ts-ignore | |
| gl.bindFramebuffer(gl.DRAW_FRAMEBUFFER, destRT ? destRT._platformRenderTarget._frameBuffer : null); |
| const blitMask = | ||
| (clearFlags & CameraClearFlags.Color ? 0 : gl.COLOR_BUFFER_BIT) | | ||
| (clearFlags & CameraClearFlags.Depth ? 0 : gl.DEPTH_BUFFER_BIT) | | ||
| (clearFlags & CameraClearFlags.Stencil ? 0 : gl.STENCIL_BUFFER_BIT); |
There was a problem hiding this comment.
💡 Codebase verification
The blitMask calculation is indeed incorrect and needs to be inverted
The current implementation excludes buffer bits when the corresponding clear flag is set, which is the opposite of what should happen. Looking at the CameraClearFlags enum and the usage patterns in the codebase:
- When a clear flag is set (e.g.,
CameraClearFlags.Color), we should include its corresponding buffer bit in the blit operation - The current ternary operations (
clearFlags & CameraClearFlags.X ? 0 : gl.X_BUFFER_BIT) do the opposite
The suggested fix in the review comment is correct:
- const blitMask =
- (clearFlags & CameraClearFlags.Color ? 0 : gl.COLOR_BUFFER_BIT) |
- (clearFlags & CameraClearFlags.Depth ? 0 : gl.DEPTH_BUFFER_BIT) |
- (clearFlags & CameraClearFlags.Stencil ? 0 : gl.STENCIL_BUFFER_BIT);
+ const blitMask =
+ (clearFlags & CameraClearFlags.Color ? gl.COLOR_BUFFER_BIT : 0) |
+ (clearFlags & CameraClearFlags.Depth ? gl.DEPTH_BUFFER_BIT : 0) |
+ (clearFlags & CameraClearFlags.Stencil ? gl.STENCIL_BUFFER_BIT : 0);🔗 Analysis chain
Review blitMask calculation logic
The blitMask calculation appears inverted. When a clear flag is set, the corresponding buffer bit is excluded from blitting.
Consider this fix:
- const blitMask =
- (clearFlags & CameraClearFlags.Color ? 0 : gl.COLOR_BUFFER_BIT) |
- (clearFlags & CameraClearFlags.Depth ? 0 : gl.DEPTH_BUFFER_BIT) |
- (clearFlags & CameraClearFlags.Stencil ? 0 : gl.STENCIL_BUFFER_BIT);
+ const blitMask =
+ (clearFlags & CameraClearFlags.Color ? gl.COLOR_BUFFER_BIT : 0) |
+ (clearFlags & CameraClearFlags.Depth ? gl.DEPTH_BUFFER_BIT : 0) |
+ (clearFlags & CameraClearFlags.Stencil ? gl.STENCIL_BUFFER_BIT : 0);Let me gather more context about the CameraClearFlags enum and the usage of blitMask.
Let me get the definition of CameraClearFlags enum to fully understand the flags.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar blit operations in the codebase
rg -A 5 "blitFramebuffer.*BUFFER_BIT"
# Search for CameraClearFlags usage patterns
ast-grep --pattern 'clearFlags & CameraClearFlags.$_'
Length of output: 889
Script:
#!/bin/bash
# Find CameraClearFlags definition
ast-grep --pattern 'enum CameraClearFlags'
# Look for blitFramebuffer implementation
ast-grep --pattern 'blitFramebuffer($$$)'
# Search for any documentation or comments about clear flags
rg -B 2 "CameraClearFlags"
Length of output: 12397
Script:
#!/bin/bash
# Get the content of CameraClearFlags enum
cat packages/core/src/enums/CameraClearFlags.ts
Length of output: 583
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
e2e/fixtures/originImage/Advance_multi-scene-no-clear.jpgis excluded by!**/*.jpg
📒 Files selected for processing (2)
packages/core/src/RenderPipeline/BasicRenderPipeline.ts(2 hunks)packages/rhi-webgl/src/WebGLGraphicDevice.ts(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/core/src/RenderPipeline/BasicRenderPipeline.ts
🔇 Additional comments (4)
packages/rhi-webgl/src/WebGLGraphicDevice.ts (4)
22-23: LGTM: Import additions are necessary for new functionality
The new imports TextureCubeFace and TextureFormat are correctly added to support the enhanced functionality in activeRenderTarget and blitFrameBufferToViewport methods.
Line range hint 281-291: LGTM: Improved clearRenderTarget signature with proper null checks
The optional clearColor parameter with proper null checking enhances the method's flexibility while maintaining safety.
343-344: LGTM: Optional mipLevel parameter aligns with WebGL practices
Making mipLevel optional is a good improvement as it's commonly defaulted to 0 in WebGL operations.
373-382: LGTM: Proper WebGL2 feature detection
The WebGL2 check and warning message are correctly implemented.
| blitFrameBufferToViewport( | ||
| srcRT: RenderTarget, | ||
| destRT: RenderTarget, | ||
| clearFlags: CameraClearFlags, | ||
| viewport: Vector4 | ||
| ) { | ||
| if (!this._isWebGL2) { | ||
| Logger.warn("WebGL1.0 not support blit frame buffer."); | ||
| return; | ||
| } | ||
| const gl = this._gl; | ||
| // @ts-ignore | ||
| const srcFrameBuffer = srcRT ? srcRT._platformRenderTarget._frameBuffer : null; | ||
| // @ts-ignore | ||
| const destFrameBuffer = destRT ? destRT._platformRenderTarget._frameBuffer : null; | ||
| const bufferWidth = this.getMainFrameBufferWidth(); | ||
| const bufferHeight = this.getMainFrameBufferHeight(); | ||
| const srcWidth = srcRT ? srcRT.width : bufferWidth; | ||
| const srcHeight = srcRT ? srcRT.height : bufferHeight; | ||
| const destWidth = srcWidth * viewport.z; | ||
| const destHeight = srcHeight * viewport.w; | ||
| const needFlipY = !!srcRT !== !!destRT; | ||
|
|
||
| gl.bindFramebuffer(gl.READ_FRAMEBUFFER, srcFrameBuffer); | ||
| gl.bindFramebuffer(gl.DRAW_FRAMEBUFFER, destFrameBuffer); | ||
|
|
||
| // @ts-ignore | ||
| const canBlitDepthStencil = !srcRT || srcRT._depthFormat === TextureFormat.Depth24Stencil8; | ||
|
|
||
| let blitMask = clearFlags & CameraClearFlags.Color ? 0 : gl.COLOR_BUFFER_BIT; | ||
|
|
||
| if (canBlitDepthStencil) { | ||
| blitMask |= clearFlags & CameraClearFlags.Depth ? 0 : gl.DEPTH_BUFFER_BIT; | ||
| blitMask |= clearFlags & CameraClearFlags.Stencil ? 0 : gl.STENCIL_BUFFER_BIT; | ||
| } | ||
|
|
||
| const xStart = viewport.x * srcWidth; | ||
| const xEnd = xStart + destWidth; | ||
| const yStart = needFlipY ? srcHeight - viewport.y * srcHeight : srcHeight - viewport.y * srcHeight - destHeight; | ||
| const yEnd = needFlipY ? yStart - destHeight : yStart + destHeight; | ||
|
|
||
| gl.blitFramebuffer(xStart, yStart, xEnd, yEnd, 0, 0, destWidth, destHeight, blitMask, gl.NEAREST); | ||
| } |
There was a problem hiding this comment.
Add unit tests for blitFrameBufferToViewport
The method lacks test coverage as indicated by static analysis.
Would you like me to help create unit tests covering:
- WebGL2 vs WebGL1 behavior
- Different clear flag combinations
- Invalid render target scenarios
- Buffer dimension handling
| const srcFrameBuffer = srcRT ? srcRT._platformRenderTarget._frameBuffer : null; | ||
| // @ts-ignore | ||
| const destFrameBuffer = destRT ? destRT._platformRenderTarget._frameBuffer : null; |
There was a problem hiding this comment.
Add error handling for invalid render targets
Direct access to internal properties without validation could lead to runtime errors.
Add proper error handling:
-const srcFrameBuffer = srcRT ? srcRT._platformRenderTarget._frameBuffer : null;
-const destFrameBuffer = destRT ? destRT._platformRenderTarget._frameBuffer : null;
+const srcFrameBuffer = srcRT?.._platformRenderTarget?._frameBuffer ?? null;
+const destFrameBuffer = destRT?._platformRenderTarget?._frameBuffer ?? null;
+
+if (srcRT && !srcFrameBuffer) {
+ Logger.error("Invalid source render target");
+ return;
+}
+if (destRT && !destFrameBuffer) {
+ Logger.error("Invalid destination render target");
+ return;
+}Committable suggestion skipped: line range outside the PR's diff.
| let blitMask = clearFlags & CameraClearFlags.Color ? 0 : gl.COLOR_BUFFER_BIT; | ||
|
|
||
| if (canBlitDepthStencil) { | ||
| blitMask |= clearFlags & CameraClearFlags.Depth ? 0 : gl.DEPTH_BUFFER_BIT; | ||
| blitMask |= clearFlags & CameraClearFlags.Stencil ? 0 : gl.STENCIL_BUFFER_BIT; | ||
| } |
There was a problem hiding this comment.
Fix inverted blitMask calculation
The current implementation excludes buffer bits when the corresponding clear flag is set, which is the opposite of what should happen.
Fix the blitMask calculation:
-let blitMask = clearFlags & CameraClearFlags.Color ? 0 : gl.COLOR_BUFFER_BIT;
+let blitMask = clearFlags & CameraClearFlags.Color ? gl.COLOR_BUFFER_BIT : 0;
if (canBlitDepthStencil) {
- blitMask |= clearFlags & CameraClearFlags.Depth ? 0 : gl.DEPTH_BUFFER_BIT;
- blitMask |= clearFlags & CameraClearFlags.Stencil ? 0 : gl.STENCIL_BUFFER_BIT;
+ blitMask |= clearFlags & CameraClearFlags.Depth ? gl.DEPTH_BUFFER_BIT : 0;
+ blitMask |= clearFlags & CameraClearFlags.Stencil ? gl.STENCIL_BUFFER_BIT : 0;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let blitMask = clearFlags & CameraClearFlags.Color ? 0 : gl.COLOR_BUFFER_BIT; | |
| if (canBlitDepthStencil) { | |
| blitMask |= clearFlags & CameraClearFlags.Depth ? 0 : gl.DEPTH_BUFFER_BIT; | |
| blitMask |= clearFlags & CameraClearFlags.Stencil ? 0 : gl.STENCIL_BUFFER_BIT; | |
| } | |
| let blitMask = clearFlags & CameraClearFlags.Color ? gl.COLOR_BUFFER_BIT : 0; | |
| if (canBlitDepthStencil) { | |
| blitMask |= clearFlags & CameraClearFlags.Depth ? gl.DEPTH_BUFFER_BIT : 0; | |
| blitMask |= clearFlags & CameraClearFlags.Stencil ? gl.STENCIL_BUFFER_BIT : 0; | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
packages/core/src/RenderPipeline/BasicRenderPipeline.ts (1)
168-175: Document the internal color target clearing strategy.The clearing logic for internal color targets is correct but would benefit from documentation explaining:
- Why blitting is needed when not using CameraClearFlags.All
- The purpose of reactivating the render target after blitting
Add a comment block above the condition:
+ // When using internal color target: + // - For CameraClearFlags.All: Perform a full clear + // - Otherwise: Selectively clear and blit from camera render target + // to preserve existing content while applying specified clear flags if (internalColorTarget) {packages/rhi-webgl/src/WebGLGraphicDevice.ts (1)
373-415: Add unit tests for the new methodThis new method lacks test coverage as indicated by static analysis.
Would you like me to help create unit tests covering:
- WebGL2 vs WebGL1 behavior
- Different clear flag combinations
- Invalid render target scenarios
- Buffer dimension handling
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/core/src/RenderPipeline/BasicRenderPipeline.ts(2 hunks)packages/rhi-webgl/src/WebGLGraphicDevice.ts(5 hunks)
🔇 Additional comments (7)
packages/core/src/RenderPipeline/BasicRenderPipeline.ts (2)
213-213: LGTM!
Good addition of whitespace to improve readability by separating the post-processing and blitting operations.
168-175: Verify performance impact of blitting operations.
The new clearing strategy involves additional blitting operations and render target switches. Please verify the performance impact, especially for:
- Blitting between camera render target and internal RT
- Additional render target reactivation
Let's analyze the usage of blitting operations in the codebase:
Also applies to: 213-218
packages/rhi-webgl/src/WebGLGraphicDevice.ts (5)
Line range hint 281-291: LGTM! Improved robustness of clearRenderTarget
The changes correctly handle cases where clearColor is not provided, preventing potential null reference issues while maintaining the existing functionality.
344-344: LGTM! Improved API flexibility
Making mipLevel optional improves API usability for cases where mipmaps aren't used.
379-382: LGTM! Proper WebGL version check
The method correctly checks for WebGL2 support before proceeding with the blit operation.
402-407:
Fix inverted blitMask calculation
The current implementation excludes buffer bits when the corresponding clear flag is set, which is the opposite of what should happen.
Fix the blitMask calculation:
-let blitMask = clearFlags & CameraClearFlags.Color ? 0 : gl.COLOR_BUFFER_BIT;
+let blitMask = clearFlags & CameraClearFlags.Color ? gl.COLOR_BUFFER_BIT : 0;
if (canBlitDepthStencil) {
- blitMask |= clearFlags & CameraClearFlags.Depth ? 0 : gl.DEPTH_BUFFER_BIT;
- blitMask |= clearFlags & CameraClearFlags.Stencil ? 0 : gl.STENCIL_BUFFER_BIT;
+ blitMask |= clearFlags & CameraClearFlags.Depth ? gl.DEPTH_BUFFER_BIT : 0;
+ blitMask |= clearFlags & CameraClearFlags.Stencil ? gl.STENCIL_BUFFER_BIT : 0;
}Likely invalid or redundant comment.
385-387:
Add null checks for render target internal properties
Direct access to internal properties without validation could lead to runtime errors.
Add proper error handling:
-const srcFrameBuffer = srcRT ? srcRT._platformRenderTarget._frameBuffer : null;
-const destFrameBuffer = destRT ? destRT._platformRenderTarget._frameBuffer : null;
+const srcFrameBuffer = srcRT?.._platformRenderTarget?._frameBuffer ?? null;
+const destFrameBuffer = destRT?._platformRenderTarget?._frameBuffer ?? null;
+
+if (srcRT && !srcFrameBuffer) {
+ Logger.error("Invalid source render target");
+ return;
+}
+if (destRT && !destFrameBuffer) {
+ Logger.error("Invalid destination render target");
+ return;
+}Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
e2e/case/.mockForE2E.ts (2)
44-46: Consider adding explicit resource disposal.While this is test code, it's good practice to dispose of GPU resources. Consider adding cleanup for
renderColorTextureandrenderTargetafter the screenshot is taken.const renderTarget = new RenderTarget(engine, width, height, renderColorTexture, TextureFormat.Depth24Stencil8, 1); +callbacks.push(() => { + renderColorTexture.destroy(); + renderTarget.destroy(); +});
47-59: LGTM! Consider adding error handling.The camera state management is thorough, properly storing and restoring the original state. However, consider wrapping the render operation in a try-catch to ensure cleanup runs even if rendering fails.
cameras.forEach((camera) => { const originalTarget = camera.renderTarget; + try { // render to off-screen camera.renderTarget = renderTarget; camera.aspectRatio = width / height; camera.render(); + } catch (error) { + console.error('Failed to render camera:', error); + } callbacks.push(() => { camera.renderTarget = originalTarget; camera.resetAspectRatio(); }); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
e2e/case/.mockForE2E.ts(4 hunks)e2e/case/multi-camera-no-clear.ts(1 hunks)e2e/case/multi-scene-no-clear.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- e2e/case/multi-scene-no-clear.ts
🔇 Additional comments (9)
e2e/case/multi-camera-no-clear.ts (6)
1-20: LGTM! Well-structured imports and metadata.
The file has clear documentation and properly organized imports.
21-32: Add error handling and cleanup for engine initialization.
The engine creation promise should include error handling and proper resource cleanup.
38-51: LGTM! Clear camera and mesh setup.
The first camera and cube setup follows best practices with clear configuration.
62-65: Avoid accessing private members with @ts-ignore.
Direct access to private members makes the code fragile.
73-86: LGTM! Proper test setup with both cameras.
The second cube setup and test finalization properly handle both cameras for the screenshot comparison.
53-71: Verify the behavior of CameraClearFlags.None.
The camera is configured with CameraClearFlags.None which is the core change of this PR. Please ensure this change:
- Doesn't clear the previous camera's render output
- Properly composites with the first camera's output
✅ Verification successful
Based on the code analysis, I can now provide a verified response about the behavior of CameraClearFlags.None:
The CameraClearFlags.None implementation is correctly handling multi-camera compositing
The implementation in BasicRenderPipeline.ts confirms that:
- When clearFlags is None (0x0), the render pipeline explicitly skips the clear operation (
clearFlags !== CameraClearFlags.None && rhi.clearRenderTarget()) - The pipeline properly handles compositing by:
- Preserving the previous render target content when clearFlags is None
- Only applying post-processing effects on top of the preserved content
- Correctly managing the render order between cameras through culling masks (Layer.Layer1)
The code matches the intended behavior of compositing the second camera's output on top of the first camera's render without clearing.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other usages of CameraClearFlags.None to understand the impact
rg -A 3 "CameraClearFlags.None"
# Look for related test cases
fd -e ts -e js "camera.*clear" "test/"
Length of output: 2799
Script:
#!/bin/bash
# Search for the CameraClearFlags enum definition to understand all possible values
ast-grep --pattern 'enum CameraClearFlags {
$$$
}'
# Look for render pipeline implementation to understand how clear flags are handled
ast-grep --pattern 'class BasicRenderPipeline {
$$$
render($$$) {
$$$
}
$$$
}'
# Search for test files in e2e directory instead
fd -e ts -e js "camera.*clear" "e2e/"
Length of output: 33962
e2e/case/.mockForE2E.ts (3)
41-42: LGTM! Clean implementation of camera handling.
The conversion of single camera to array and the use of callbacks for cleanup is a well-structured approach that handles both single and multiple camera cases elegantly.
107-107: LGTM! Proper cleanup timing.
The cleanup callbacks are executed at the right time, after the blob is created and before potentially resuming the engine.
22-22: Verify impact on existing test cases.
The parameter type change from Camera to Camera | Camera[] is a breaking change. While this change is necessary for supporting multiple cameras, we should ensure all existing test cases are updated accordingly.
✅ Verification successful
Breaking change is properly handled in existing test cases
Based on the search results, all existing test cases are compatible with the new parameter type:
- Most test cases pass a single camera, which is valid with the union type
Camera | Camera[] - The multi-camera scenarios (
multi-camera-no-clear.ts,multi-scene-clear.ts,multi-scene-no-clear.ts) already use the array syntax[camera1, camera2] - No instances found where the change would break existing functionality
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for existing test cases using initScreenshot to ensure they're compatible
rg "initScreenshot\(" --type ts
Length of output: 3315
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (8)
packages/core/src/renderingHardwareInterface/IPlatformTexture2D.ts (2)
67-76: Add explicit return type and fix method signature.The method signature could be improved for better type safety and consistency:
copySubFromRenderTarget( renderTarget: RenderTarget, level: number, xOffset: number, yOffset: number, x: number, y: number, width: number, height: number - ); + ): void;
56-66: Fix documentation inconsistencies.The JSDoc comments have a few minor issues:
- The y-coordinate parameter description is incorrect (copy-paste error)
- Consider adding more context about the coordinate system (e.g., whether it's relative to the render target or texture)
* @param x - Specifying the x coordinate of the lower left corner where to start copying - * @param y - Specifying the x coordinate of the lower left corner where to start copying + * @param y - Specifying the y coordinate of the lower left corner where to start copyingpackages/core/src/texture/Texture2D.ts (1)
179-211: Add parameter validation and enhance documentationThe implementation looks good but could benefit from some improvements:
- Consider adding parameter validation:
- Bounds checking for offsets and dimensions
- Null/undefined check for renderTarget
- Documentation could be enhanced with:
@throwsdocumentation for potential errors- Example usage
Here's a suggested implementation:
/** * Copy the specified area of the render target to the sub texture. * @param renderTarget - The render target to copy from * @param level - Texture mipmapping level * @param xOffset - Specifying the horizontal offset within the texture image * @param yOffset - Specifying the vertical offset within the texture image * @param x - Specifying the x coordinate of the lower left corner where to start copying * @param y - Specifying the x coordinate of the lower left corner where to start copying * @param width - The width of the copy area * @param height - The height of the copy area + * @throws {Error} If renderTarget is null or undefined + * @throws {Error} If parameters are out of bounds + * @example + * ```typescript + * // Copy a 100x100 area from position (0,0) of the render target + * // to position (10,10) of the texture at mip level 0 + * texture.copySubFromRenderTarget(renderTarget, 0, 10, 10, 0, 0, 100, 100); + * ``` */ copySubFromRenderTarget( renderTarget: RenderTarget, level: number, xOffset: number, yOffset: number, x: number, y: number, width: number, height: number ) { + if (!renderTarget) { + throw new Error("RenderTarget cannot be null or undefined"); + } + + const mipWidth = Math.max(1, this._width >> level); + const mipHeight = Math.max(1, this._height >> level); + + if (xOffset < 0 || yOffset < 0 || + x < 0 || y < 0 || + width <= 0 || height <= 0 || + xOffset + width > mipWidth || + yOffset + height > mipHeight) { + throw new Error("Invalid parameters: out of bounds"); + } + (this._platformTexture as IPlatformTexture2D).copySubFromRenderTarget( renderTarget, level,packages/core/src/RenderPipeline/PipelineUtils.ts (1)
Line range hint
154-197: Consider documenting coordinate system assumptions.While the implementation is solid, it would be valuable to document:
- The coordinate system conventions being used (e.g., DirectX vs OpenGL).
- Common scenarios where Y-axis flipping might be needed.
- Performance implications of enabling the flip Y macro.
This documentation would help users understand when and why they might need to use the
flipYOfSourceparameter.packages/core/src/RenderPipeline/BasicRenderPipeline.ts (1)
133-159: Consider extracting texture management logic into separate methodsThe texture management logic is correct but could be more maintainable. Consider extracting the creation and cleanup logic into separate methods:
+ private _createGrabTextureIfNeeded(viewport: Viewport): void { + if (!this._canUseBlitFrameBuffer && this._shouldGrabColor) { + this._grabTexture = PipelineUtils.recreateTextureIfNeeded( + this._camera.engine, + this._grabTexture, + viewport.width, + viewport.height, + TextureFormat.R8G8B8A8, + false, + TextureWrapMode.Clamp, + TextureFilterMode.Bilinear + ); + } + } + private _cleanupTextures(): void { + if (this._internalColorTarget) { + this._internalColorTarget.getColorTexture(0)?.destroy(true); + this._internalColorTarget.destroy(true); + this._internalColorTarget = null; + } + if (this._grabTexture) { + this._grabTexture.destroy(true); + this._grabTexture = null; + } + }packages/rhi-webgl/src/WebGLGraphicDevice.ts (3)
Line range hint
344-371: Add input validation for mipLevelWhile the implementation is correct, consider adding validation for the mipLevel parameter to prevent invalid values.
mipLevel?: number, faceIndex?: TextureCubeFace ) { + if (mipLevel !== undefined && (mipLevel < 0 || !Number.isInteger(mipLevel))) { + Logger.warn("Invalid mipLevel. Using 0 instead."); + mipLevel = 0; + } let bufferWidth: number, bufferHeight: number;
379-382: Improve WebGL2 check messageThe warning message could be more informative about the fallback behavior.
- Logger.warn("WebGL1.0 not support blit frame buffer."); + Logger.warn("WebGL1.0 does not support blitFramebuffer. Consider using a different approach for copying render targets.");
416-429: Add input validation and documentationThe method would benefit from proper input validation and documentation explaining its purpose and parameters.
+ /** + * Copies a portion of a render target to a subtexture. + * @param srcRT - Source render target. If null, the main framebuffer is used. + * @param destRT - Destination render target for dimension calculations. + * @param grabTexture - The texture to copy to. + * @param viewport - The viewport defining the region to copy. + */ copyRenderTargetToSubTexture(srcRT: RenderTarget, destRT: RenderTarget, grabTexture: Texture2D, viewport: Vector4) { + if (!destRT) { + Logger.error("Destination render target cannot be null"); + return; + } + if (!grabTexture) { + Logger.error("Grab texture cannot be null"); + return; + } const bufferWidth = this.getMainFrameBufferWidth();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
e2e/fixtures/originImage/Advance_multi-camera-no-clear.jpgis excluded by!**/*.jpgpackages/core/src/shaderlib/extra/Blit.fs.glslis excluded by!**/*.glsl
📒 Files selected for processing (7)
e2e/case/multi-camera-no-clear.ts(1 hunks)packages/core/src/RenderPipeline/BasicRenderPipeline.ts(5 hunks)packages/core/src/RenderPipeline/PipelineUtils.ts(5 hunks)packages/core/src/renderingHardwareInterface/IPlatformTexture2D.ts(2 hunks)packages/core/src/texture/Texture2D.ts(2 hunks)packages/rhi-webgl/src/GLTexture2D.ts(2 hunks)packages/rhi-webgl/src/WebGLGraphicDevice.ts(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- e2e/case/multi-camera-no-clear.ts
🔇 Additional comments (19)
packages/core/src/renderingHardwareInterface/IPlatformTexture2D.ts (1)
67-76: Verify the implementation and usage context.
Since this PR is about fixing the clear flag of internal render target, let's verify how this new method is used in the implementation:
✅ Verification successful
Implementation verified and found to be part of render target optimization
The copySubFromRenderTarget method is correctly implemented in GLTexture2D and is used in WebGLGraphicDevice for copying render target contents with proper viewport handling. This implementation is part of the render target optimization, where it's used to copy portions of render targets while respecting viewport coordinates and handling the Y-flip for framebuffer operations. The method is consistent with the PR's objective of fixing render target handling.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for implementations and usages of copySubFromRenderTarget
echo "Searching for implementations:"
ast-grep --pattern 'class $_ implements IPlatformTexture2D {
$$$
copySubFromRenderTarget($$$) {
$$$
}
$$$
}'
echo -e "\nSearching for usages:"
rg "copySubFromRenderTarget" -A 3
Length of output: 1919
Script:
#!/bin/bash
# Let's check the implementation in GLTexture2D and its usage context in WebGLGraphicDevice
echo "GLTexture2D implementation:"
rg -A 15 "copySubFromRenderTarget\(" packages/rhi-webgl/src/GLTexture2D.ts
echo -e "\nWebGLGraphicDevice usage context:"
rg -B 10 -A 5 "grabTexture\.copySubFromRenderTarget" packages/rhi-webgl/src/WebGLGraphicDevice.ts
echo -e "\nLet's also check if there are any changes to clear flag handling:"
rg "clear.*flag" -A 3
Length of output: 4390
packages/rhi-webgl/src/GLTexture2D.ts (2)
1-8: LGTM: Import changes are appropriate
The addition of the RenderTarget import aligns with the new functionality and follows the existing import pattern.
123-143: Verify render target compatibility
Please ensure that the implementation verifies compatibility between the render target and texture:
- Format compatibility
- Dimension boundaries to prevent invalid copies
packages/core/src/texture/Texture2D.ts (2)
7-7: LGTM!
The import statement for RenderTarget is correctly placed and necessary for the new functionality.
190-209: Verify integration with render target clear flag
Since this PR aims to fix the clear flag of internal render target, we should verify that this new copy operation properly handles the clear state of the source render target.
Let's check for related render target clear flag handling:
packages/core/src/RenderPipeline/PipelineUtils.ts (5)
4-4: LGTM: Import addition is appropriate.
The addition of ShaderMacro import aligns with the new Y-axis flipping functionality.
154-154: LGTM: Method signature update is backward compatible.
The new flipYOfSource parameter is well-documented and maintains backward compatibility with its default value of false.
Also applies to: 163-164
183-187: LGTM: Clean implementation of Y-axis flip control.
The implementation correctly toggles the flip Y macro based on the input parameter.
18-18: Verify shader macro definition exists.
The code references a shader macro named "renderer_FlipYBlitTexture". Please ensure this macro is properly defined in the shader files.
✅ Verification successful
Shader macro is properly defined and used
The macro "renderer_FlipYBlitTexture" is correctly defined and used in packages/core/src/shaderlib/extra/Blit.fs.glsl as a preprocessor directive. The static property's usage of this macro name is valid.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the FlipYBlitTexture macro definition in shader files
rg -t glsl -t hlsl "renderer_FlipYBlitTexture"
Length of output: 75
Script:
#!/bin/bash
# Search for shader files first to understand file extensions
fd -e glsl -e hlsl -e shader -e frag -e vert -e comp
# Then search for the macro in all files since file type detection failed
rg "renderer_FlipYBlitTexture"
Length of output: 6505
194-194: Verify shader macro collection changes.
The order of shader macro collection operations has been modified. Please verify that this change:
- Maintains correct shader variant compilation
- Preserves all necessary macros in the final collection
Also applies to: 197-197
✅ Verification successful
Let me gather more information about the shader macro collection implementation and usage to better understand the changes.
Let me try a different approach to understand the shader macro collection implementation.
Based on the gathered information, I can now provide a final response:
The shader macro collection changes are safe and maintain correct behavior.
The implementation shows that ShaderMacroCollection.unionCollection is consistently used across the codebase to combine macro collections, and the order of operations in the modified code follows the same pattern as other implementations:
- First union with camera's global shader macros
- Then union with component-specific macros
The macro collection is properly preserved as the operation uses the same compileMacros output collection that's subsequently used for shader program compilation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other instances of ShaderMacroCollection.unionCollection to verify consistency
rg -A 2 "ShaderMacroCollection\.unionCollection"
Length of output: 1875
Script:
#!/bin/bash
# Check the implementation of ShaderMacroCollection.unionCollection
ast-grep --pattern 'class ShaderMacroCollection {
$$$
static unionCollection($$$) {
$$$
}
$$$
}'
# Check for any related tests
rg -l "ShaderMacroCollection.*test"
# Look for the definition and usage of _macroCollection
ast-grep --pattern 'class $_ {
$$$
_macroCollection: ShaderMacroCollection;
$$$
}'
Length of output: 239
Script:
#!/bin/bash
# Find the ShaderMacroCollection class definition
rg -A 10 "class ShaderMacroCollection"
# Look for tests or usage patterns
rg -A 3 "_macroCollection"
# Check the specific file for context around the changed line
rg -B 5 -A 5 "rendererShaderData._macroCollection" packages/core/src/RenderPipeline/PipelineUtils.ts
Length of output: 6224
packages/core/src/RenderPipeline/BasicRenderPipeline.ts (5)
114-116: LGTM! Clear and efficient flag handling
The implementation correctly combines camera clear flags with ignore flags using bitwise operations, and the logic for determining color grabbing necessity is clear and efficient.
162-162: LGTM! Improved method signature
The updated method signature is more explicit with finalClearFlags and follows TypeScript best practices by placing optional parameters at the end.
Also applies to: 168-170
193-234: Add test coverage for clearing logic paths
The new clearing logic paths lack test coverage. This is particularly important given the browser compatibility considerations and complex conditional logic.
193-234: Improve clarity of complex clearing logic
While the implementation is functionally correct, the complex clearing logic could benefit from:
- More detailed comments explaining the different clearing strategies
- Breaking down the nested conditions into separate methods
- Adding debug logging for browser compatibility paths
Consider extracting the clearing logic into separate methods:
+ private _handleFullClear(engine: Engine, color: Color): void {
+ engine._hardwareRenderer.clearRenderTarget(engine, CameraClearFlags.All, color);
+ }
+ private _handlePartialClear(engine: Engine, color: Color, finalClearFlags: CameraClearFlags): void {
+ const rhi = engine._hardwareRenderer;
+ if (this._canUseBlitFrameBuffer) {
+ this._handleBlitFrameBufferClear(rhi, engine, color, finalClearFlags);
+ } else {
+ this._handleFallbackClear(rhi, engine, color);
+ }
+ }Let's check for similar complex clearing logic in the codebase that might benefit from the same improvements:
46-48: Investigate and document browser compatibility for blitFrameBuffer
The TODO comment suggests there's a browser compatibility issue with blitFrameBuffer that needs investigation. Consider:
- Documenting which browsers are affected
- Adding browser detection logic
- Creating a compatibility table in the documentation
Let's check for similar compatibility handling in the codebase:
packages/rhi-webgl/src/WebGLGraphicDevice.ts (4)
22-23: LGTM: Import additions are necessary
The new imports TextureCubeFace and TextureFormat are required for the enhanced render target functionality.
Line range hint 281-314: LGTM: Improved clearRenderTarget method signature and safety
The changes make the method more flexible by making clearColor optional and add proper null checking before using it. This prevents potential runtime errors while maintaining backward compatibility.
399-406:
Fix inverted blitMask calculation
The current implementation excludes buffer bits when the corresponding clear flag is set, which is the opposite of what should happen.
- let blitMask = clearFlags & CameraClearFlags.Color ? 0 : gl.COLOR_BUFFER_BIT;
+ let blitMask = clearFlags & CameraClearFlags.Color ? gl.COLOR_BUFFER_BIT : 0;
const canBlitDepthStencil = !srcRT || srcRT._depthFormat === TextureFormat.Depth24Stencil8;
if (canBlitDepthStencil) {
- blitMask |= clearFlags & CameraClearFlags.Depth ? 0 : gl.DEPTH_BUFFER_BIT;
- blitMask |= clearFlags & CameraClearFlags.Stencil ? 0 : gl.STENCIL_BUFFER_BIT;
+ blitMask |= clearFlags & CameraClearFlags.Depth ? gl.DEPTH_BUFFER_BIT : 0;
+ blitMask |= clearFlags & CameraClearFlags.Stencil ? gl.STENCIL_BUFFER_BIT : 0;
}385-387:
Add error handling for invalid render targets
Direct access to internal properties without validation could lead to runtime errors.
+ if (srcRT && !srcRT._platformRenderTarget?._frameBuffer) {
+ Logger.error("Invalid source render target");
+ return;
+ }
+ if (destRT && !destRT._platformRenderTarget?._frameBuffer) {
+ Logger.error("Invalid destination render target");
+ return;
+ }
const srcFrameBuffer = srcRT ? srcRT._platformRenderTarget._frameBuffer : null;
const destFrameBuffer = destRT ? destRT._platformRenderTarget._frameBuffer : null;| copySubFromRenderTarget( | ||
| renderTarget: RenderTarget, | ||
| level: number, | ||
| xOffset: number, | ||
| yOffset: number, | ||
| x: number, | ||
| y: number, | ||
| width: number, | ||
| height: number | ||
| ) { | ||
| // @ts-ignore | ||
| const frameBuffer = renderTarget?._platformRenderTarget._frameBuffer ?? null; | ||
| const gl = this._gl; | ||
|
|
||
| // @ts-ignore | ||
| gl.bindFramebuffer(gl.FRAMEBUFFER, frameBuffer); | ||
|
|
||
| this._bind(); | ||
|
|
||
| gl.copyTexSubImage2D(this._target, level, xOffset, yOffset, x, y, width, height); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and state management
The implementation has several areas that could be improved for robustness:
-
Missing error handling for:
- Null/undefined renderTarget
- Invalid dimensions or offsets
- WebGL errors during operations
-
WebGL state management:
- Previous framebuffer state is not restored
- No validation of texture binding state
-
Type safety:
- @ts-ignore comments suggest potential type system issues
Consider applying these improvements:
copySubFromRenderTarget(
renderTarget: RenderTarget,
level: number,
xOffset: number,
yOffset: number,
x: number,
y: number,
width: number,
height: number
) {
+ if (!renderTarget) {
+ throw new Error("RenderTarget cannot be null or undefined");
+ }
+
+ // Validate dimensions
+ if (width <= 0 || height <= 0) {
+ throw new Error("Width and height must be positive");
+ }
+
+ const gl = this._gl;
+ // Store current framebuffer
+ const previousFramebuffer = gl.getParameter(gl.FRAMEBUFFER_BINDING);
+
// @ts-ignore
const frameBuffer = renderTarget?._platformRenderTarget._frameBuffer ?? null;
- const gl = this._gl;
// @ts-ignore
gl.bindFramebuffer(gl.FRAMEBUFFER, frameBuffer);
this._bind();
gl.copyTexSubImage2D(this._target, level, xOffset, yOffset, x, y, width, height);
+
+ // Restore previous framebuffer
+ gl.bindFramebuffer(gl.FRAMEBUFFER, previousFramebuffer);
}Additionally, consider creating proper type definitions to eliminate the need for @ts-ignore:
interface IPlatformRenderTarget {
_frameBuffer: WebGLFramebuffer | null;
}
interface IRenderTarget extends RenderTarget {
_platformRenderTarget: IPlatformRenderTarget;
}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (6)
packages/core/src/RenderPipeline/BasicRenderPipeline.ts (3)
46-48: LGTM! Consider adding JSDoc comments.The new private fields are well-named and properly typed. Consider adding JSDoc comments to explain their purpose and usage.
+ /** Texture used for grabbing color data during render operations */ private _grabTexture: Texture2D; + /** Flag indicating if WebGL2 blitFramebuffer is supported and usable */ private _canUseBlitFrameBuffer = false; + /** Flag indicating if color data should be grabbed during rendering */ private _shouldGrabColor = false;
Line range hint
121-164: Consider extracting texture management logic.The texture creation and cleanup logic is complex and could benefit from being extracted into separate methods for better maintainability.
Consider refactoring like this:
+ private _createOrUpdateInternalTextures(viewport: Viewport): void { + if (this._shouldGrabColor && !this._canUseBlitFrameBuffer) { + this._grabTexture = this._createOrUpdateGrabTexture(viewport); + } + this._internalColorTarget = this._createOrUpdateColorTarget(viewport); + } + private _cleanupTextures(): void { + this._destroyTextureIfExists(this._internalColorTarget); + this._destroyTextureIfExists(this._grabTexture); + this._internalColorTarget = null; + this._grabTexture = null; + }
198-237: Consider performance implications of multiple render target switches.The code switches render targets multiple times which could impact performance. Consider measuring the performance impact and potentially optimizing the render target switches.
packages/rhi-webgl/src/WebGLGraphicDevice.ts (3)
107-107: Add JSDoc for the context getterThe context getter provides access to WebGL context attributes, but lacks documentation.
Add JSDoc comments to describe the getter's purpose:
+/** + * Get the WebGL context attributes used to create the context. + * @returns {WebGLContextAttributes} The context attributes. + */ get context(): WebGLGraphicDeviceOptions { return this._contextAttributes; }Also applies to: 140-142
Line range hint
286-326: Add validation for clearColor parameterWhile the clearColor parameter is now optional, the code should validate that clearColor is provided when the color clear flag is set.
Consider this improvement:
-if (clearFlags & CameraClearFlags.Color && clearColor) { +if (clearFlags & CameraClearFlags.Color) { + if (!clearColor) { + Logger.warn("Clear color is required when CameraClearFlags.Color is set"); + return; + } clearFlag |= gl.COLOR_BUFFER_BIT;
Line range hint
348-374: Add validation for mipLevel parameterThe mipLevel parameter should be validated to ensure it's within bounds.
Add validation:
activeRenderTarget( renderTarget: RenderTarget, viewport: Vector4, isFlipProjection: boolean, mipLevel?: number, faceIndex?: TextureCubeFace ) { + if (mipLevel !== undefined && (mipLevel < 0 || mipLevel > 31)) { + Logger.warn("Invalid mipLevel. Must be between 0 and 31."); + return; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
packages/core/src/RenderPipeline/BasicRenderPipeline.ts(6 hunks)packages/core/src/renderingHardwareInterface/IPlatformTexture2D.ts(2 hunks)packages/core/src/texture/Texture2D.ts(2 hunks)packages/rhi-webgl/src/WebGLGraphicDevice.ts(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/core/src/renderingHardwareInterface/IPlatformTexture2D.ts
- packages/core/src/texture/Texture2D.ts
🔇 Additional comments (5)
packages/core/src/RenderPipeline/BasicRenderPipeline.ts (3)
88-89: Add test coverage for clear flag handling.
The new clear flag calculation logic needs test coverage.
88-95: Verify WebGL2 capability check across different platforms.
The conditions for enabling blitFramebuffer are complex and platform-dependent. Please ensure this works correctly across different browsers and devices.
✅ Verification successful
Based on the search results, I can now provide a final response since I have enough information about WebGL2 capability checks and blitFramebuffer usage across the codebase:
WebGL2 capability check is correctly implemented with proper fallbacks
The conditions for enabling blitFramebuffer are well-handled across the codebase:
- The WebGL2 check is consistently implemented through
rhi.isWebGL2 - There are proper fallbacks and warnings when WebGL2 is not available
- The MSAA (Multi-Sample Anti-Aliasing) conditions are correctly checked:
- WebGL1 doesn't support MSAA and blitFramebuffer
- MSAA samples are validated against hardware capabilities
- Proper error handling exists for MSAA framebuffer incompatibility
The implementation in BasicRenderPipeline.ts follows the same pattern used throughout the codebase and correctly handles the platform-specific constraints:
- WebGL2 context check
- MSAA compatibility check
- Antialias configuration check
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for related WebGL capability checks and potential issues
rg -A 3 "isWebGL2|blitFramebuffer|MSAA"
Length of output: 46546
211-219: Verify error handling for WebGL operations.
The texture copy operations could fail in certain WebGL contexts. Ensure proper error handling is in place.
packages/rhi-webgl/src/WebGLGraphicDevice.ts (2)
22-23: LGTM: Import additions are properly used
The new imports (TextureCubeFace, TextureFormat) are correctly added and utilized in the new functionality.
378-426:
Add proper error handling for render target validation
The method directly accesses internal properties of render targets without validation, which could lead to runtime errors.
Add proper validation:
blitInternalRTByBlitFrameBuffer(
srcRT: RenderTarget,
destRT: RenderTarget,
clearFlags: CameraClearFlags,
viewport: Vector4
) {
if (!this._isWebGL2) {
Logger.warn("WebGL1.0 not support blit frame buffer.");
return;
}
+ if (!destRT) {
+ Logger.error("Destination render target is required");
+ return;
+ }
const gl = this._gl;
- const srcFrameBuffer = srcRT ? srcRT._platformRenderTarget._frameBuffer : null;
- const destFrameBuffer = destRT ? destRT._platformRenderTarget._frameBuffer : null;
+ const srcFrameBuffer = srcRT?.._platformRenderTarget?._frameBuffer ?? null;
+ const destFrameBuffer = destRT?._platformRenderTarget?._frameBuffer;
+ if (!destFrameBuffer) {
+ Logger.error("Invalid destination render target");
+ return;
+ }Likely invalid or redundant comment.
| copyRenderTargetToSubTexture(srcRT: RenderTarget, destRT: RenderTarget, grabTexture: Texture2D, viewport: Vector4) { | ||
| const bufferWidth = this.getMainFrameBufferWidth(); | ||
| const bufferHeight = this.getMainFrameBufferHeight(); | ||
| const srcWidth = srcRT ? srcRT.width : bufferWidth; | ||
| const srcHeight = srcRT ? srcRT.height : bufferHeight; | ||
| const copyWidth = destRT.width; | ||
| const copyHeight = destRT.height; | ||
| const flipY = !srcRT; | ||
|
|
||
| const xStart = viewport.x * srcWidth; | ||
| const yStart = flipY ? srcHeight - viewport.y * srcHeight - copyHeight : viewport.y * srcHeight; | ||
|
|
||
| grabTexture.copySubFromRenderTarget(srcRT, 0, 0, 0, xStart, yStart, copyWidth, copyHeight); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation for copyRenderTargetToSubTexture
The method needs validation for the grabTexture parameter and viewport dimensions.
Add validation:
copyRenderTargetToSubTexture(
srcRT: RenderTarget,
destRT: RenderTarget,
grabTexture: Texture2D,
viewport: Vector4
) {
+ if (!destRT || !grabTexture) {
+ Logger.error("Destination render target and grab texture are required");
+ return;
+ }
+ if (viewport.x < 0 || viewport.y < 0 || viewport.z < 0 || viewport.w < 0) {
+ Logger.error("Invalid viewport dimensions");
+ return;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| copyRenderTargetToSubTexture(srcRT: RenderTarget, destRT: RenderTarget, grabTexture: Texture2D, viewport: Vector4) { | |
| const bufferWidth = this.getMainFrameBufferWidth(); | |
| const bufferHeight = this.getMainFrameBufferHeight(); | |
| const srcWidth = srcRT ? srcRT.width : bufferWidth; | |
| const srcHeight = srcRT ? srcRT.height : bufferHeight; | |
| const copyWidth = destRT.width; | |
| const copyHeight = destRT.height; | |
| const flipY = !srcRT; | |
| const xStart = viewport.x * srcWidth; | |
| const yStart = flipY ? srcHeight - viewport.y * srcHeight - copyHeight : viewport.y * srcHeight; | |
| grabTexture.copySubFromRenderTarget(srcRT, 0, 0, 0, xStart, yStart, copyWidth, copyHeight); | |
| } | |
| copyRenderTargetToSubTexture(srcRT: RenderTarget, destRT: RenderTarget, grabTexture: Texture2D, viewport: Vector4) { | |
| if (!destRT || !grabTexture) { | |
| Logger.error("Destination render target and grab texture are required"); | |
| return; | |
| } | |
| if (viewport.x < 0 || viewport.y < 0 || viewport.z < 0 || viewport.w < 0) { | |
| Logger.error("Invalid viewport dimensions"); | |
| return; | |
| } | |
| const bufferWidth = this.getMainFrameBufferWidth(); | |
| const bufferHeight = this.getMainFrameBufferHeight(); | |
| const srcWidth = srcRT ? srcRT.width : bufferWidth; | |
| const srcHeight = srcRT ? srcRT.height : bufferHeight; | |
| const copyWidth = destRT.width; | |
| const copyHeight = destRT.height; | |
| const flipY = !srcRT; | |
| const xStart = viewport.x * srcWidth; | |
| const yStart = flipY ? srcHeight - viewport.y * srcHeight - copyHeight : viewport.y * srcHeight; | |
| grabTexture.copySubFromRenderTarget(srcRT, 0, 0, 0, xStart, yStart, copyWidth, copyHeight); | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
packages/core/src/RenderPipeline/BasicRenderPipeline.ts (2)
83-95: Document complex WebGL compatibility conditionsThe conditions for determining
_canUseBlitFrameBufferinvolve multiple WebGL-specific constraints. Consider adding documentation to explain:
- Why WebGL2 is required
- The MSAA sampling restriction
- The antialias limitation
- this._canUseBlitFrameBuffer = - rhi.isWebGL2 && camera.msaaSamples === 1 && (!!camera.renderTarget || !rhi.context.antialias); + // BlitFramebuffer is only supported when: + // 1. Using WebGL2 context (required for blitFramebuffer operation) + // 2. MSAA is disabled (can't blit to/from MSAA textures) + // 3. Either using a render target or antialiasing is disabled (platform limitation) + this._canUseBlitFrameBuffer = + rhi.isWebGL2 && camera.msaaSamples === 1 && (!!camera.renderTarget || !rhi.context.antialias);
Line range hint
121-164: Consider extracting texture management logicThe texture management logic is complex and could benefit from being extracted into separate methods for better maintainability and testability.
Consider refactoring into these methods:
private _manageInternalColorTarget(camera: Camera): void { if (camera.independentCanvasEnabled) { this._createOrUpdateInternalColorTarget(camera); this._manageGrabTexture(camera); } else { this._cleanupTextureResources(); } } private _createOrUpdateInternalColorTarget(camera: Camera): void { const viewport = camera.pixelViewport; this._internalColorTarget = PipelineUtils.recreateRenderTargetIfNeeded( camera.engine, this._internalColorTarget, viewport.width, viewport.height, // ... existing parameters ... ); } private _manageGrabTexture(camera: Camera): void { if (!this._canUseBlitFrameBuffer && this._shouldGrabColor) { this._grabTexture = PipelineUtils.recreateTextureIfNeeded( camera.engine, this._grabTexture, // ... existing parameters ... ); } } private _cleanupTextureResources(): void { // ... existing cleanup logic ... }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/core/src/RenderPipeline/BasicRenderPipeline.ts(6 hunks)
🔇 Additional comments (2)
packages/core/src/RenderPipeline/BasicRenderPipeline.ts (2)
46-48: LGTM: Well-structured field declarations
The new private fields are properly typed and scoped, with appropriate initial values for managing texture grabbing and framebuffer operations.
198-239: Add test coverage for clearing scenarios
The clearing logic handles multiple scenarios that should be tested:
- Full clear with
CameraClearFlags.All - Clearing with blitFramebuffer support
- Clearing with grab texture fallback
- Direct clearing without internal color target
This was previously mentioned in an earlier review. Would you like assistance in creating comprehensive test cases?
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
packages/rhi-webgl/src/WebGLGraphicDevice.ts (2)
140-142: Consider renaming the getter method for clarityThe getter name 'context' might be misleading as it returns WebGLContextAttributes rather than the WebGL context itself. Consider renaming it to 'contextAttributes' to better reflect its purpose.
- get context(): WebGLGraphicDeviceOptions { + get contextAttributes(): WebGLGraphicDeviceOptions { return this._contextAttributes; }
296-296: Consider separating the clear color flag checkThe condition combines both flag check and color presence. This might skip color clearing even when the flag is set, which could be unexpected behavior. Consider separating these checks for clarity.
- if (clearFlags & CameraClearFlags.Color && clearColor) { + if (clearFlags & CameraClearFlags.Color) { + if (!clearColor) { + Logger.warn("Clear color flag is set but no clear color provided"); + return; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/core/src/RenderPipeline/BasicRenderPipeline.ts(6 hunks)packages/rhi-webgl/src/WebGLGraphicDevice.ts(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/core/src/RenderPipeline/BasicRenderPipeline.ts
🔇 Additional comments (4)
packages/rhi-webgl/src/WebGLGraphicDevice.ts (4)
441-442: Good use of nullish coalescing operator
The use of ?? operator for fallback to null is a good practice here.
378-426:
Fix the blitMask calculation logic
The blitMask calculation for color, depth, and stencil is inverted from what it should be.
390-392:
Add proper error handling for render targets
Direct access to internal properties without validation could lead to runtime errors.
428-453:
Add input validation for copyRenderTargetToSubTexture
The method needs validation for the grabTexture parameter and viewport dimensions.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
packages/rhi-webgl/src/WebGLGraphicDevice.ts (1)
378-445: Add unit tests for new methods.The newly added methods
blitInternalRTByBlitFrameBufferandcopyRenderTargetToSubTextureneed test coverage to ensure they work correctly across different scenarios:
- WebGL2 vs WebGL1 behavior
- Different clear flag combinations
- Invalid render target scenarios
- Buffer dimension handling
Would you like me to help create unit tests for these methods?
packages/core/src/Camera.ts (1)
Line range hint
273-279: Potential TypeScript overload ambiguity insetReplacementShadermethod.The
setReplacementShadermethod is overloaded with signatures that differ only in the type of thereplacementTagparameter (stringvs.ShaderTagKey). This may lead to ambiguity, as TypeScript might have difficulty resolving which overload to use at call sites.Consider unifying the overloads by using a union type for the
replacementTagparameter and updating the documentation accordingly. Here's a suggested refactor:- setReplacementShader(shader: Shader, replacementTagName?: string, failureStrategy?: ReplacementFailureStrategy); - setReplacementShader(shader: Shader, replacementTag?: ShaderTagKey, failureStrategy?: ReplacementFailureStrategy); + /** + * Set the replacement shader. + * @param shader - Replacement shader + * @param replacementTag - Sub shader tag name or tag key + * @param failureStrategy - Replacement failure strategy, @defaultValue `ReplacementFailureStrategy.KeepOriginalShader` + * + * @remarks + * If `replacementTag` is not specified, the first sub shader will be replaced. + * If `replacementTag` is specified, the replacement shader will find the first sub shader which has the same tag value obtained by `replacementTag`. If it fails to find the sub shader, the strategy will be determined by `failureStrategy`. + */ + setReplacementShader( + shader: Shader, + replacementTag?: string | ShaderTagKey, + failureStrategy?: ReplacementFailureStrategy + ): void;This change simplifies the method signature and prevents potential overload resolution issues.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
packages/core/src/Camera.ts(2 hunks)packages/core/src/RenderPipeline/BasicRenderPipeline.ts(7 hunks)packages/rhi-webgl/src/WebGLGraphicDevice.ts(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/core/src/RenderPipeline/BasicRenderPipeline.ts
🔇 Additional comments (6)
packages/rhi-webgl/src/WebGLGraphicDevice.ts (4)
22-23: LGTM! Well-structured addition of context attributes.
The addition of TextureFormat import and context-related properties with proper initialization in _initGLState is clean and follows good practices.
Also applies to: 107-107, 140-142, 519-519
Line range hint 286-296: LGTM! Improved clearRenderTarget signature.
Making clearColor optional is a good improvement as it better reflects that color clearing is conditional on both the flag and the presence of a clearColor.
349-349: LGTM! Made mipLevel parameter optional.
The change improves the API by making mipLevel optional while maintaining correct behavior.
378-418:
Fix the blitMask calculation and add render target validation.
The implementation has two issues:
- The blitMask calculation is inverted - it should include buffer bits when the corresponding clear flag is set
- Missing validation for render target internal properties
Apply these fixes:
-const needBlitColor = (clearFlags & CameraClearFlags.Color) === 0;
-const needBlitDepth = (clearFlags & CameraClearFlags.Depth) === 0;
-const needBlitStencil = (clearFlags & CameraClearFlags.Stencil) === 0;
+const needBlitColor = (clearFlags & CameraClearFlags.Color) !== 0;
+const needBlitDepth = (clearFlags & CameraClearFlags.Depth) !== 0;
+const needBlitStencil = (clearFlags & CameraClearFlags.Stencil) !== 0;
+// Add validation
+if (srcRT && (!srcRT._platformRenderTarget || !srcRT._platformRenderTarget._frameBuffer)) {
+ Logger.error("Invalid source render target");
+ return;
+}
+if (destRT && (!destRT._platformRenderTarget || !destRT._platformRenderTarget._frameBuffer)) {
+ Logger.error("Invalid destination render target");
+ return;
+}Likely invalid or redundant comment.
packages/core/src/Camera.ts (2)
86-86: Documentation update improves clarity.
The updated remark for the msaaSamples property clarifies that it takes effect when independentCanvasEnabled is true, enhancing user understanding.
168-177: Logic in independentCanvasEnabled getter is correct.
The revised implementation accurately determines when the camera requires an independent canvas based on post-processing effects, HDR, and opaque texture settings. The use of optional chaining ensures safety when accessing properties.
| copyRenderTargetToSubTexture(srcRT: RenderTarget, grabTexture: Texture2D, viewport: Vector4) { | ||
| const gl = this._gl; | ||
| const bufferWidth = this.getMainFrameBufferWidth(); | ||
| const bufferHeight = this.getMainFrameBufferHeight(); | ||
| const srcWidth = srcRT ? srcRT.width : bufferWidth; | ||
| const srcHeight = srcRT ? srcRT.height : bufferHeight; | ||
| const copyWidth = grabTexture.width; | ||
| const copyHeight = grabTexture.height; | ||
| const flipY = !srcRT; | ||
|
|
||
| const xStart = viewport.x * srcWidth; | ||
| const yStart = flipY ? srcHeight - viewport.y * srcHeight - copyHeight : viewport.y * srcHeight; | ||
|
|
||
| // @ts-ignore | ||
| const frameBuffer = srcRT?._platformRenderTarget._frameBuffer ?? null; | ||
|
|
||
| // @ts-ignore | ||
| gl.bindFramebuffer(gl.FRAMEBUFFER, frameBuffer); | ||
|
|
||
| // @ts-ignore | ||
| const glTexture = grabTexture._platformTexture; | ||
|
|
||
| glTexture._bind(); | ||
|
|
||
| gl.copyTexSubImage2D(glTexture._target, 0, 0, 0, xStart, yStart, copyWidth, copyHeight); | ||
| } |
There was a problem hiding this comment.
Add validation and use consistent optional chaining.
The implementation needs validation for grabTexture and should use consistent optional chaining.
Apply these fixes:
+if (!grabTexture?._platformTexture) {
+ Logger.error("Invalid grab texture");
+ return;
+}
-const glTexture = grabTexture._platformTexture;
+const glTexture = grabTexture?._platformTexture;
-glTexture._bind();
+glTexture?._bind();Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
packages/rhi-webgl/src/WebGLGraphicDevice.ts (3)
107-107: Add null check for context attributesThe context getter should handle cases where _contextAttributes might be null before initialization.
get context(): WebGLGraphicDeviceOptions { - return this._contextAttributes; + if (!this._contextAttributes) { + Logger.warn("Context attributes accessed before initialization"); + return {}; + } + return { ...this._contextAttributes }; }Also applies to: 140-142
349-349: Add validation for mipLevel parameterWhile making mipLevel optional improves flexibility, consider adding validation to ensure it's non-negative when provided.
-mipLevel?: number, +mipLevel = 0, faceIndex?: TextureCubeFace ) { + if (mipLevel < 0) { + Logger.warn("Invalid mipLevel. Using 0 instead."); + mipLevel = 0; + }
546-547: Add type assertion for context attributesConsider adding type assertion to ensure type safety of the context attributes.
-this._contextAttributes = gl.getContextAttributes(); +this._contextAttributes = gl.getContextAttributes() as WebGLContextAttributes;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/core/src/RenderPipeline/BasicRenderPipeline.ts(6 hunks)packages/rhi-webgl/src/WebGLGraphicDevice.ts(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/core/src/RenderPipeline/BasicRenderPipeline.ts
🔇 Additional comments (3)
packages/rhi-webgl/src/WebGLGraphicDevice.ts (3)
Line range hint 286-296: LGTM! Improved null safety
The changes correctly handle the optional clearColor parameter and prevent unnecessary color buffer clearing when no color is provided.
448-473:
Add validation and error handling
The method needs proper validation and error handling for its parameters and internal property access.
copyRenderTargetToSubTexture(srcRT: RenderTarget, grabTexture: Texture2D, viewport: Vector4) {
+ if (!grabTexture) {
+ Logger.error("Grab texture is required");
+ return;
+ }
+
+ if (viewport.x < 0 || viewport.y < 0 || viewport.z < 0 || viewport.w < 0) {
+ Logger.error("Invalid viewport parameters");
+ return;
+ }
+
+ // @ts-ignore
+ if (!grabTexture._platformTexture) {
+ Logger.error("Invalid grab texture");
+ return;
+ }Likely invalid or redundant comment.
390-392:
Add null checks for internal properties
Direct access to internal properties without validation could lead to runtime errors.
-const srcFrameBuffer = srcRT ? srcRT._platformRenderTarget._frameBuffer : null;
-const destFrameBuffer = destRT ? destRT._platformRenderTarget._frameBuffer : null;
+const srcFrameBuffer = srcRT?.._platformRenderTarget?._frameBuffer ?? null;
+const destFrameBuffer = destRT?._platformRenderTarget?._frameBuffer ?? null;
+
+if (destRT && !destFrameBuffer) {
+ Logger.error("Invalid destination render target");
+ return;
+}Likely invalid or redundant comment.
| blitInternalRTByBlitFrameBuffer( | ||
| srcRT: RenderTarget, | ||
| destRT: RenderTarget, | ||
| clearFlags: CameraClearFlags, | ||
| viewport: Vector4 | ||
| ) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation for viewport parameter
The viewport parameter should be validated to ensure its components are within valid ranges.
+ if (viewport.x < 0 || viewport.y < 0 || viewport.z < 0 || viewport.w < 0) {
+ Logger.error("Invalid viewport parameters");
+ return;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| blitInternalRTByBlitFrameBuffer( | |
| srcRT: RenderTarget, | |
| destRT: RenderTarget, | |
| clearFlags: CameraClearFlags, | |
| viewport: Vector4 | |
| ) { | |
| blitInternalRTByBlitFrameBuffer( | |
| srcRT: RenderTarget, | |
| destRT: RenderTarget, | |
| clearFlags: CameraClearFlags, | |
| viewport: Vector4 | |
| ) { | |
| if (viewport.x < 0 || viewport.y < 0 || viewport.z < 0 || viewport.w < 0) { | |
| Logger.error("Invalid viewport parameters"); | |
| return; | |
| } |
Please check if the PR fulfills these requirements
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
What is the current behavior? (You can also link to an open issue here)
What is the new behavior (if this is a feature change)?
Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)
Other information:
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Chores