coderabbitai[bot]
commented
3 days ago Walkthrough
The changes in this pull request involve modifications to the BasicRenderPipeline, _PostProcessManager, and BasicResources classes. In BasicRenderPipeline.ts, the rendering logic is updated to include checks for internalColorTarget, affecting how the render target is cleared. The _PostProcessManager class sees the introduction of new blending properties and adjustments to the rendering logic based on the camera's clear flags. Additionally, BasicResources.ts enhances the blending functionality of the blitMaterial. These changes collectively refine the rendering and post-processing capabilities of the system.
Changes
| File Path | Change Summary |
|---|---|
packages/core/src/RenderPipeline/BasicRenderPipeline.ts |
Updated render and _drawRenderPass methods to include conditional checks for internalColorTarget, altering render target clearing behavior. |
packages/core/src/postProcess/PostProcessManager.ts |
Added imports for CameraClearFlags and expanded Shader imports. Introduced new blending properties in the uberMaterial and modified _render logic. |
packages/core/src/BasicResources.ts |
Added imports for BlendFactor and BlendOperation. Configured blending properties in the blitMaterial constructor. |
packages/core/src/RenderPipeline/PipelineUtils.ts |
Added import for CameraClearFlags and modified blitTexture method to adjust blend state based on camera's clear flags. |
Possibly related PRs
- #2414: This PR modifies the
BasicRenderPipelineclass, specifically therendermethod, which is directly related to the changes made in the main PR that also affects theBasicRenderPipelineclass and its rendering logic. - #2437: This PR addresses changes in the
RenderQueueandBasicRenderPipeline, focusing on therenderQueueType, which is relevant to the rendering behavior discussed in the main PR.
Suggested labels
Rendering, post processing
Suggested reviewers
- GuoLei1990
- Sway007
Poem
In the realm of render, colors gleam,
With blending magic, we weave a dream.
Clear flags dancing, targets in sight,
A pipeline of wonders, oh what a delight!
Hops of joy as pixels play,
In this vibrant world, we’ll forever stay! 🐇✨
📜 Recent review details
**Configuration used: CodeRabbit UI** **Review profile: CHILL**📥 Commits
Reviewing files that changed from the base of the PR and between e252e1b7e7643dd87d60925d407d3879d1873c0e and 26b906064c56320a6d3f079b9a2819aa28b60e55.📒 Files selected for processing (2)
* `packages/core/src/BasicResources.ts` (2 hunks) * `packages/core/src/RenderPipeline/PipelineUtils.ts` (2 hunks)🔇 Additional comments (4)
packages/core/src/BasicResources.ts (2)
`11-11`: **LGTM: Import additions are necessary for blend state configuration.** The new imports are correctly added and properly used in the blend state configuration below. --- `50-57`: **Verify the impact of always-enabled alpha blending.** The blend state configuration implements standard alpha blending, which is appropriate for handling transparent overlays and post-processing effects. However, consider the following: 1. The blend state is always enabled, which might impact performance in cases where blending isn't needed 2. The configuration is hardcoded, making it inflexible for different use cases Let's verify the usage context: Consider making the blend state configurable: ```diff 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)
`3-3`: **LGTM!** The import statement for `CameraClearFlags` is correctly placed and necessary for the new functionality. --- `198-202`: **Verify impact on existing render pipelines.** 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 successful
**No impact on existing render pipelines** After analyzing the codebase usage of `blitTexture`, the blend state modification is safe because: 1. All direct pipeline usages (BasicRenderPipeline, OpaqueTexturePass) don't rely on the default blend state as they provide their own materials. 2. Post-processing calls (PostProcessManager, BloomEffect) explicitly pass materials, bypassing the new blend state logic. 3. The only case where no material is provided is in BasicRenderPipeline's final blit, where the blend state modification actually helps ensure proper color clearing behavior. The change is well-contained and improves the default behavior without affecting existing implementations.🏁 Scripts executed
The following scripts were executed for the analysis: Script: ```shell #!/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: ```shell #!/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: 4814Thank 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?
codecov[bot]
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