Fix background render error when render to RT #2414
Conversation
zhuxudong
changed the title
WalkthroughThe changes in this pull request involve modifications to the Changes
Possibly related PRs
Suggested labels
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 #2414 +/- ##
==========================================
- Coverage 69.48% 69.46% -0.03%
==========================================
Files 522 522
Lines 27363 27365 +2
Branches 4097 4097
==========================================
- Hits 19013 19008 -5
- Misses 6849 6853 +4
- Partials 1501 1504 +3
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: 1
🧹 Outside diff range and nitpick comments (1)
packages/core/src/RenderPipeline/BasicRenderPipeline.ts (1)
320-320: LGTM! Consider documenting the camera shader data dependency.The addition of camera shader data upload is crucial for correct y-axis orientation in render targets. Consider adding a code comment explaining this dependency.
program.uploadAll(program.materialUniformBlock, material.shaderData); + // Upload camera shader data for correct y-axis orientation in render targets program.uploadAll(program.cameraUniformBlock, camera.shaderData);🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 320-320: packages/core/src/RenderPipeline/BasicRenderPipeline.ts#L320
Added line #L320 was not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/core/src/RenderPipeline/BasicRenderPipeline.ts(3 hunks)packages/core/src/shaderlib/extra/background-texture.vs.glsl(1 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
packages/core/src/RenderPipeline/BasicRenderPipeline.ts
[warning] 178-178: packages/core/src/RenderPipeline/BasicRenderPipeline.ts#L178
Added line #L178 was not covered by tests
[warning] 320-320: packages/core/src/RenderPipeline/BasicRenderPipeline.ts#L320
Added line #L320 was not covered by tests
🔇 Additional comments (3)
packages/core/src/shaderlib/extra/background-texture.vs.glsl (2)
4-4: LGTM! Well-chosen uniform for handling projection parameters.
The addition of camera_ProjectionParams follows standard practice for handling coordinate system differences between various render targets.
8-8: LGTM! Correct approach for Y-axis adjustment.
The multiplication of gl_Position.y with camera_ProjectionParams.x is the standard way to handle Y-axis flipping when rendering to different render targets.
Let's verify the camera parameter integration:
✅ Verification successful
Let me gather more information about how camera_ProjectionParams is set and used in the render context.
Let me check one final aspect - how the projection params are calculated and set.
Verified: Y-axis flipping implementation is correct and properly integrated
The verification confirms that:
- The
camera_ProjectionParamsuniform is properly declared in shader files - The projection params are correctly set in RenderContext with the x component being -1 or 1 based on
flipProjectionflag - The shader correctly applies this to flip the Y coordinate when needed
This matches the standard approach for Y-axis flipping in render targets.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the camera parameter is properly set in the pipeline
# Expected: Find the corresponding uniform setup in the render pipeline
# Search for camera_ProjectionParams setup in the pipeline
rg -A 5 "camera_ProjectionParams" packages/core/src/
# Search for _drawBackgroundTexture to verify camera parameter usage
ast-grep --pattern $'_drawBackgroundTexture($$$, camera$$$)'
Length of output: 2495
Script:
#!/bin/bash
# Search for where camera_ProjectionParams is set in RenderContext
rg -A 10 "_cameraProjectionProperty" packages/core/src/RenderPipeline/RenderContext.ts
# Search for background texture rendering implementation
ast-grep --pattern 'drawBackgroundTexture($$$) {
$$$
}'
Length of output: 815
Script:
#!/bin/bash
# Search for where projectionParams is set/calculated in RenderContext
rg -B 5 -A 10 "projectionParams" packages/core/src/RenderPipeline/RenderContext.ts
# Also check Camera class for any relevant projection param setup
rg -A 5 "ProjectionParams" packages/core/src/Camera.ts
Length of output: 1280
packages/core/src/RenderPipeline/BasicRenderPipeline.ts (1)
178-178: LGTM! Verify RT rendering behavior.
The addition of the camera parameter is correct and aligns with the need to handle projection parameters for render target output.
Let's verify the background texture rendering behavior with render targets:
✅ Verification successful
Based on the evidence gathered from the codebase:
- The background texture shader is properly initialized and used in the Background class
- The camera parameter addition in
_drawBackgroundTexturealigns with render target handling - The RenderContext shows evidence of Y-axis flipping matrices for render targets
- Multiple examples demonstrate proper render target and camera projection handling
✅ Camera parameter addition is correct for RT background rendering
The change to add the camera parameter to _drawBackgroundTexture is verified as the correct approach for handling background texture Y-axis rendering with render targets. The codebase shows consistent patterns of camera projection parameter handling for render targets, and the Background class properly initializes the shader that would consume these parameters.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other background texture render target usages
rg -g '!*.test.*' -g '!*.spec.*' "renderTarget.*background.*texture" -A 5
Length of output: 75
Script:
#!/bin/bash
# Let's try different search patterns to find render target and background texture usages
rg -g '!*.test.*' -g '!*.spec.*' "renderTarget.*background" -A 5
# Also search for background texture rendering methods
rg -g '!*.test.*' -g '!*.spec.*' "_drawBackgroundTexture" -A 5
# Look for render target setup with background
ast-grep --pattern 'renderTarget = $$$; background = $$$;'
# Search for background texture related tests
rg -g '*.test.*' -g '*.spec.*' "background.*texture.*renderTarget"
Length of output: 1315
Script:
#!/bin/bash
# Let's examine the background texture shader and material setup
ast-grep --pattern 'class Background {
$$$
}'
# Look for camera projection related code in shaders
rg -g '*.shader.*' -g '*.vert.*' -g '*.frag.*' "camera_ProjectionParams" -A 5
# Check for render target related camera setup
ast-grep --pattern 'camera.renderTarget = $$$'
# Look for any background texture flipping or y-axis related code
rg -g '!*.test.*' -g '!*.spec.*' "flipY|yAxis|textureY" -A 3
Length of output: 39745
Script:
#!/bin/bash
# Let's examine the background texture shader code and camera projection handling
rg -g '*.ts' "background-texture" -B 2 -A 5
# Look for camera projection matrix handling in render context
ast-grep --pattern 'class RenderContext {
$$$
_updateShaderData($$$) {
$$$
}
$$$
}'
# Check for render target specific camera handling
rg -g '*.ts' "renderTarget.*camera.*projection" -A 3
Length of output: 2359
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 178-178: packages/core/src/RenderPipeline/BasicRenderPipeline.ts#L178
Added line #L178 was not covered by tests
| @@ -175,7 +175,7 @@ | |||
| if (background.mode === BackgroundMode.Sky) { | |||
| background.sky._render(context); | |||
| } else if (background.mode === BackgroundMode.Texture && background.texture) { | |||
| this._drawBackgroundTexture(engine, background); | |||
| this._drawBackgroundTexture(engine, camera, background); | |||
There was a problem hiding this comment.
Add tests for background texture RT rendering.
The changes fix a y-axis rendering issue but lack test coverage. Consider adding tests to:
- Verify background texture rendering with render targets
- Ensure correct y-axis orientation
- Prevent future regressions
Would you like help creating test cases for:
- Background texture rendering with and without render targets
- Y-axis orientation verification
- Camera projection parameter integration
Also applies to: 320-320
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 178-178: packages/core/src/RenderPipeline/BasicRenderPipeline.ts#L178
Added line #L178 was not covered by tests
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/core/src/RenderPipeline/BasicRenderPipeline.ts (1)
Line range hint
303-321: Add documentation and validation for the y-axis fix.While the implementation looks correct, consider these improvements:
- Add a comment explaining how the camera shader data fixes the y-axis issue when rendering to RT
- Add validation before uploading camera shader data
Example documentation:
private _drawBackgroundTexture(camera: Camera, background: Background) { + // Upload camera projection parameters to fix y-axis orientation when rendering to RT + // This ensures correct vertical positioning of background texture in render target output const engine = camera.engine;Example validation:
program.uploadAll(program.materialUniformBlock, material.shaderData); + if (!camera.shaderData) { + console.warn('Camera shader data is required for correct background texture rendering'); + return; + } program.uploadAll(program.cameraUniformBlock, camera.shaderData);🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 304-304: packages/core/src/RenderPipeline/BasicRenderPipeline.ts#L304
Added line #L304 was not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/core/src/RenderPipeline/BasicRenderPipeline.ts(3 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
packages/core/src/RenderPipeline/BasicRenderPipeline.ts
[warning] 178-178: packages/core/src/RenderPipeline/BasicRenderPipeline.ts#L178
Added line #L178 was not covered by tests
[warning] 304-304: packages/core/src/RenderPipeline/BasicRenderPipeline.ts#L304
Added line #L304 was not covered by tests
[warning] 321-321: packages/core/src/RenderPipeline/BasicRenderPipeline.ts#L321
Added line #L321 was not covered by tests
🔇 Additional comments (1)
packages/core/src/RenderPipeline/BasicRenderPipeline.ts (1)
178-178: LGTM, but needs test coverage.
The parameter change from engine to camera is correct and aligns with the PR objective to fix y-axis rendering issues. However, this change lacks test coverage.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 178-178: packages/core/src/RenderPipeline/BasicRenderPipeline.ts#L178
Added line #L178 was not covered by tests
GuoLei1990
changed the title
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
Bug Fixes
Documentation