Fix clear flag of internal render target #2444
Conversation
WalkthroughThe changes in this pull request involve modifications to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🔇 Additional comments (4)packages/core/src/BasicResources.ts (2)
The new imports are correctly added and properly used in the blend state configuration below.
The blend state configuration implements standard alpha blending, which is appropriate for handling transparent overlays and post-processing effects. However, consider the following:
Let's verify the usage context: Consider making the blend state configurable: const blendState = blitMaterial.renderState.blendState.targetBlendState;
+// Allow blend state to be configured through material properties
+const blendEnabled = material.getProperty("blendEnabled") ?? true;
+blendState.enabled = blendEnabled;
-blendState.enabled = true;
blendState.sourceColorBlendFactor = BlendFactor.SourceAlpha;
blendState.destinationColorBlendFactor = BlendFactor.OneMinusSourceAlpha;packages/core/src/RenderPipeline/PipelineUtils.ts (2)
The import statement for
The blend state modification logic is sound - disabling blending when the camera has color clear flags ensures proper clearing behavior. However, this change might affect existing render pipelines. Let's verify the usage of this utility across the codebase: ✅ Verification successfulNo impact on existing render pipelines After analyzing the codebase usage of
The change is well-contained and improves the default behavior without affecting existing implementations. 🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Description: Find all usages of blitTexture to assess impact
# Look for calls where material parameter is not provided
# Search for blitTexture calls
rg -A 3 'blitTexture\('
# Search for potential render pipeline implementations that might be affected
ast-grep --pattern 'class $_ extends BasicRenderPipeline {
$$$
}'
Length of output: 2890 Script: #!/bin/bash
# Let's analyze the actual usage patterns of blitTexture without material parameter
# Search for blitTexture calls with context to see material parameter
rg -B 2 -A 2 'blitTexture\([^)]*\)' --multiline
# Find any custom render pipeline implementations
ast-grep --pattern 'class $_ implements IRenderPipeline {
$$$
}'
Length of output: 4814 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.77% -0.02%
==========================================
Files 524 524
Lines 27487 27506 +19
Branches 4111 4113 +2
==========================================
+ Hits 19457 19467 +10
- Misses 6477 6483 +6
- Partials 1553 1556 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
![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) {
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
New Features
BasicResourcesfor better visual effects.Bug Fixes
Documentation