feat(graphic): add deferred pass#434
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughReplaces the legacy DefaultRenderPass and its creation with a G-buffer + Deferred render pass pipeline, updates BindGroup API to accept a name label, extends camera GPU buffer with CameraTransfer, and wires deferred textures, shader IDs, and bind-groups across initialization and GPU component setup. Changes
Sequence DiagramsequenceDiagram
participant Engine as Engine::Core
participant RG as RenderGraph
participant GBUF as GBuffer Pass
participant GBTEX as G-Buffer Textures
participant DEFER as Deferred Pass
participant LIGHTS as Lights Buffers
participant OUT as Frame Output
Engine->>RG: Build render graph (GBUFFER, DEFERRED)
RG->>GBUF: Execute GBuffer pass
GBUF->>GBTEX: Write albedo/normal/position/depth
RG->>DEFER: Execute Deferred pass (depends on GBuffer)
DEFER->>GBTEX: Read G-Buffer textures
DEFER->>LIGHTS: Read ambient & point-light buffers
DEFER->>OUT: Shade fragments and write final color
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/plugin/default-pipeline/src/plugin/PluginDefaultPipeline.cpp (1)
23-25: Outdated documentation: docstring references removed system.The docstring mentions
CreateDefaultRenderPipelinewhich has been removed from the setup systems. Update the documentation to reflect the current registration.📝 Suggested fix
- * The setup systems registered include: CreateDefaultRenderPipeline, Create3DGraph, - * CreateDefaultMaterial, CreateAmbientLight, and CreatePointLights. + * The setup systems registered include: Create3DGraph, CreateDefaultMaterial, + * CreateAmbientLight, and CreatePointLights.src/plugin/default-pipeline/src/system/GPUComponentManagement/OnMaterialCreation.cpp (1)
58-66: UseResource::GBUFFER_SHADER_IDinstead of hardcoded"DEFAULT_RENDER_PASS_SHADER"string.The material bind group structure (buffer, texture, sampler) matches the GBuffer shader's material layout (group 2, bindings 0–2). Using the string literal
"DEFAULT_RENDER_PASS_SHADER"will fail to resolve at runtime since the default render pass was removed. Replace withResource::GBUFFER_SHADER_IDand add the required include:🐛 Proposed fix
+#include "resource/pass/GBuffer.hpp"- Graphic::Resource::BindGroup bindGroup(core, bindGroupName, "DEFAULT_RENDER_PASS_SHADER", 2, + Graphic::Resource::BindGroup bindGroup(core, bindGroupName, Resource::GBUFFER_SHADER_ID, 2,
🤖 Fix all issues with AI agents
In `@src/plugin/default-pipeline/src/resource/buffer/CameraGPUBuffer.hpp`:
- Around line 15-28: Change the CameraTransfer layout to use glm::vec4 for
position, remove the manual "+ sizeof(float)" in GPUSize(), and add a
static_assert to guarantee CPU and GPU sizes match; specifically modify the
CameraTransfer struct (field position -> glm::vec4), update
CameraTransfer::GPUSize() to return sizeof(CameraTransfer) only, and add a
static_assert(sizeof(CameraTransfer) == CameraTransfer::GPUSize()) (or
equivalent) to document and enforce the expected alignment/size invariants so
CPUSize() and GPUSize() are identical.
🧹 Nitpick comments (2)
src/plugin/default-pipeline/src/resource/pass/Deferred.hpp (1)
31-33: Use a distinct name/ID for the deferred textures bind group.
Right now it reuses the shader name/ID, which is confusing and risks key collisions in BindGroupManager. Consider giving it its own label.♻️ Suggested rename
-static inline constexpr std::string_view DEFERRED_BINDGROUP_TEXTURES_NAME = "DEFERRED_SHADER"; -static inline const entt::hashed_string DEFERRED_BINDGROUP_TEXTURES_ID = - entt::hashed_string{DEFERRED_SHADER_NAME.data(), DEFERRED_SHADER_NAME.size()}; +static inline constexpr std::string_view DEFERRED_BINDGROUP_TEXTURES_NAME = "DEFERRED_BINDGROUP_TEXTURES"; +static inline const entt::hashed_string DEFERRED_BINDGROUP_TEXTURES_ID{ + DEFERRED_BINDGROUP_TEXTURES_NAME.data(), DEFERRED_BINDGROUP_TEXTURES_NAME.size()};src/plugin/default-pipeline/src/system/initialization/Create3DGraph.cpp (1)
141-166: Remove unusedDEFERRED_PASS_OUTPUT_IDtexture creation.The deferred pass renders to
END_RENDER_TEXTURE_ID(line 262), notDEFERRED_PASS_OUTPUT_ID. The latter is created with a resize callback but never consumed in production code—only in the test fileGBufferTest.cpp. This incurs unnecessary GPU allocation and resize overhead that can be eliminated.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/plugin/default-pipeline/src/system/initialization/Create3DGraph.cpp`:
- Around line 71-81: Remove the unused helper
CreateDeferredPassOutputTextureDescriptor: locate the function
CreateDeferredPassOutputTextureDescriptor in Create3DGraph.cpp and delete it
along with any related unused includes or forward declarations; ensure no
remaining references to this function exist (the deferred pass already outputs
to END_RENDER_TEXTURE_ID at the deferred pass creation), and run a build to
confirm no symbols remain referencing the removed function.
🧹 Nitpick comments (3)
src/plugin/default-pipeline/src/resource/pass/GBuffer.hpp (1)
242-242: Consider if disabling backface culling is intentional.
CullMode::Nonedisables backface culling, which renders both front and back faces. While this can be useful for specific effects (e.g., two-sided foliage), it may impact performance for typical opaque geometry. If this is intentional, a comment explaining the rationale would be helpful.src/plugin/default-pipeline/src/system/initialization/Create3DGraph.cpp (1)
143-144: Remove unused variable.
shaderContaineris obtained but never used inCreateDeferredTexturesBindingGroup.🔧 Proposed fix
static void CreateDeferredTexturesBindingGroup(Engine::Core &core) { const auto &context = core.GetResource<Graphic::Resource::Context>(); - auto &shaderContainer = core.GetResource<Graphic::Resource::ShaderContainer>();src/plugin/default-pipeline/src/resource/pass/Deferred.hpp (1)
159-161: Unused variableV- remove or implement specular lighting.The view direction
Vis computed but never used. If specular lighting is planned, consider implementing it; otherwise, remove this dead code.🔧 Proposed fix (remove unused variable)
let N = normalize(normal); - let V = normalize(camera.position - position); var lighting = ambientLight.color;
|
2 participants
This pull request introduces a new
Deferredrender pass and refactors the camera GPU buffer to support more advanced rendering features, specifically for deferred shading. It also removes legacy/default render pipeline code and updates the GBuffer pass to use the enhanced camera data structure. The main focus is on enabling deferred rendering with physically plausible point light attenuation and better camera data handling.Deferred Rendering Pipeline Enhancements:
Deferredrender pass (Deferred.hpp) that implements a deferred shading pipeline, including a WGSL shader for physically plausible point light attenuation and improved lighting calculations.GBuffer.hpp) to output additional camera data (inverse view-projection matrix and camera position) required for deferred shading, and to use the new camera buffer structure. [1] [2]Camera Buffer Improvements:
CameraGPUBufferto include the camera's inverse view-projection matrix and position, and updated its interface and buffer layout to match the requirements of deferred rendering. [1] [2] [3] [4]Code Cleanup and Legacy Removal:
CreateDefaultRenderPipelineand related utilities, to streamline the initialization and reduce confusion. [1] [2] [3] [4] [5]General Integration:
Deferredand updated GBuffer passes into the pipeline, ensuring correct bind group and buffer usage for both camera and lighting data. [1] [2] [3] [4] [5] [6]These changes lay the groundwork for a modern deferred rendering pipeline, supporting more complex lighting and shading effects while simplifying and modernizing the codebase.
Summary by CodeRabbit
New Features
Refactors
Removals
✏️ Tip: You can customize this high-level summary in your review settings.