refactor(graphic): remove default render pipeline from graphic plugin#424
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. 📒 Files selected for processing (1)
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds a new DefaultPipeline plugin that extracts GPU-component wiring, render-pipeline setup, material/lighting buffers, and update systems from the Graphic plugin; updates examples/build to use DefaultPipeline; introduces tests and resource API additions; moves many namespaces from Graphic::* to DefaultPipeline::*. Changes
Sequence Diagram(s)sequenceDiagram
participant Core as Engine::Core
participant Plugin as DefaultPipeline::Plugin
participant Rendering as RenderingPipeline
participant GraphicRes as Graphic::Resource
Core ->> Plugin: construct Plugin(core)
Plugin ->> Core: Require(Rendering, Graphic)
Plugin ->> Core: Register Resource(AmbientLight)
Plugin ->> Rendering: Register Setup systems (CreateDefaultRenderPipeline, CreateDefaultMaterial, CreatePointLights)
Plugin ->> Rendering: Register Preparation systems (UpdateGPUTransforms, UpdateGPUCameras, UpdateGPUMaterials, UpdateAmbientLight, UpdatePointLights)
Core ->> Rendering: Run Setup -> create shaders, buffers, bind groups (uses Graphic::Resource)
Core ->> Rendering: Run Preparation -> update GPU buffers from components
Rendering ->> GraphicRes: Store/Update GPU buffers & bind groups
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
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/resource/buffer/PointGPUBuffer.hpp (1)
29-39: Potential out-of-bounds access if normals/texCoords arrays are smaller than vertices.The loop iterates based on
vertices.size()but unconditionally accessesnormals[i]andtexCoords[i]. If these arrays have fewer elements thanvertices, this results in undefined behavior.Suggested defensive check
+ if (meshComponent->vertices.size() != meshComponent->normals.size() || + meshComponent->vertices.size() != meshComponent->texCoords.size()) + { + throw Graphic::Exception::NonexistentComponentError( + "Mesh vertices, normals, and texCoords must have the same size."); + } + for (uint64_t i = 0; i < meshComponent->vertices.size(); ++i) {src/plugin/graphic/src/system/preparation/PrepareEndRenderTexture.cpp (1)
86-111: Address TODO forEND_DEPTH_RENDER_TEXTURE_IDnamespace inconsistency.
END_DEPTH_RENDER_TEXTURE_ID(lines 86, 88, 93, 97, 111) is defined inGraphic::SystemwhileEND_RENDER_TEXTURE_IDis inGraphic::Utils. The code already contains a TODO comment acknowledging this: "remove this from Graphic plugin, as we don't theorically need it here". MigrateEND_DEPTH_RENDER_TEXTURE_IDtoGraphic::Utilsto align withEND_RENDER_TEXTURE_IDand resolve the inconsistency.
🤖 Fix all issues with AI agents
In @src/plugin/default-pipeline/src/DefaultPipeline.hpp:
- Around line 18-24: Remove the duplicate include of the OnCameraCreation.hpp
header in DefaultPipeline.hpp: keep a single #include
"system/GPUComponentManagement/OnCameraCreation.hpp" and delete the other
redundant line so the header is only included once.
In @src/plugin/default-pipeline/src/system/preparation/UpdatePointLight.cpp:
- Line 1: There are unused/duplicate files: an empty UpdatePointLight.cpp and
UpdatePointLights.hpp (singular) while the real implementation lives in
UpdatePointLights.cpp/UpdatePointLights.hpp (plural) and none are referenced;
either remove the empty/duplicate singular files from the PR and from build
lists (CMakeLists/compile units) to avoid confusion, or consolidate by
moving/renaming the implemented symbols from UpdatePointLights (plural) to the
singular filenames and update any registration/registration list (system
registry, plugin init, or systems vector) to reference the intended
class/function (the UpdatePointLights system) so only one consistent pair is
built and registered. Ensure you also update include paths and any forward
declarations so the build and system integration reference the single canonical
symbol (UpdatePointLights) and remove the unused files from version control if
you choose deletion.
In @src/plugin/default-pipeline/xmake.lua:
- Around line 33-38: The add_headerfiles patterns currently listed
(add_headerfiles("src/(component/*.hpp)", "src/(plugin/*.hpp)",
"src/(resource/*.hpp)", "src/(system/*.hpp)", "src/(system/preparation/*.hpp)",
"src/(utils/*.hpp)")) omit deeper subdirectories and thus miss headers in
resource/buffer, system/GPUComponentManagement, and system/initialization;
update the add_headerfiles invocations to either add explicit patterns for those
subdirs (e.g., include src/(resource/buffer/*.hpp),
src/(system/GPUComponentManagement/*.hpp), src/(system/initialization/*.hpp)) or
replace the single-level globs with recursive globs (e.g.,
src/(resource/**.hpp), src/(system/**.hpp)) so all nested headers are captured.
In @src/plugin/graphic/src/system/commandCreation/ExecuteRenderPass.cpp:
- Around line 6-16: The function GetRenderGraph is dead code; either remove
GetRenderGraph or update ExecuteRenderPass to call GetRenderGraph instead of
using GetOrDefault() so lookups throw on missing graphs. If you choose removal,
delete the GetRenderGraph definition and any unused includes; if you choose to
enforce strict lookup, replace the RenderGraphContainer::GetOrDefault(...) call
inside ExecuteRenderPass with a call to GetRenderGraph(core, name) (which relies
on RenderGraphContainer::Contains and Get) so missing graphs raise a
runtime_error.
🧹 Nitpick comments (20)
src/plugin/graphic/src/system/commandCreation/ExecuteRenderPass.cpp (1)
3-3: Unused include.
entity/Entity.hppis included but noEntitytype or related functionality appears to be used in this file. Consider removing this include.src/plugin/graphic/tests/BufferTest.cpp (2)
5-5: Potentially unused include.
Object.hppdoes not appear to be directly used in this test file. If it's not required transitively, consider removing it.
12-24: Consider guarding against double-creation to prevent resource leaks.If
Create()is called twice without an interveningDestroy(), the previously created buffer will be leaked. While this test code controls the call sequence, adding a guard improves robustness and mirrors production patterns.💡 Suggested guard
void Create(Engine::Core &core) override { + if (_isCreated) + { + return; // or throw, depending on desired behavior + } const auto &context = core.GetResource<Graphic::Resource::Context>(); wgpu::BufferDescriptor bufferDesc(wgpu::Default);src/plugin/default-pipeline/src/system/preparation/UpdatePointLight.hpp (1)
1-1: Consider deferring creation of this empty header file.This header is currently empty and unused in the codebase. Files should be created when they have actual content rather than committing empty scaffolding. Either add the intended declarations for
UpdatePointLightnow, or defer creation of this file until the system is implemented.src/plugin/default-pipeline/src/system/GPUComponentManagement/OnCameraDestruction.hpp (1)
6-10: LGTM!The namespace migration to
DefaultPipeline::Systemis correct and consistent with the PR's refactoring goals.🔧 Optional: Add closing namespace comment for consistency
Other files in this PR include closing namespace comments. Consider adding one here for consistency:
void OnCameraDestruction(Engine::Core &core, Engine::Entity entity); -} +} // namespace DefaultPipeline::Systemsrc/plugin/default-pipeline/src/component/GPUMaterial.hpp (2)
17-17: Remove the trailing semicolon after the namespace closing brace.Namespaces don't require a semicolon after the closing brace. While most compilers tolerate this, it's technically superfluous.
Suggested fix
-}; // namespace DefaultPipeline::Component +} // namespace DefaultPipeline::Component
3-4: Remove unused includes.
Camera.hppandTransform.hppare not referenced in this header—theGPUMaterialstruct only depends onentt::hashed_string. Removing them would reduce coupling and improve compile times.Suggested fix
#pragma once -#include "component/Camera.hpp" -#include "component/Transform.hpp" #include "glm/glm.hpp" #include <entt/core/hashed_string.hpp>src/plugin/default-pipeline/src/plugin/PluginDefaultPipeline.hpp (1)
8-11: Minor formatting inconsistency.The constructor's brace placement and the
// emptycomment are unconventional. Consider aligning with standard formatting.Suggested formatting
explicit Plugin(Engine::Core &core) - : Engine::APlugin(core){ - // empty - }; + : Engine::APlugin(core) + { + }src/plugin/default-pipeline/src/resource/buffer/IndexGPUBuffer.hpp (1)
35-37: Consider caching the context reference to avoid redundant resource lookups.
GetResource<Graphic::Resource::Context>()is called twice. Other buffer implementations (e.g.,CameraGPUBuffer.hpplines 17-18) cache the context in a local reference.Suggested improvement
+ const auto &context = core.GetResource<Graphic::Resource::Context>(); - _buffer = core.GetResource<Graphic::Resource::Context>().deviceContext.GetDevice()->createBuffer(bufferDesc); + _buffer = context.deviceContext.GetDevice()->createBuffer(bufferDesc); - core.GetResource<Graphic::Resource::Context>().queue->writeBuffer(_buffer, 0, indices.data(), bufferDesc.size); + context.queue->writeBuffer(_buffer, 0, indices.data(), bufferDesc.size);src/plugin/default-pipeline/src/resource/buffer/CameraGPUBuffer.hpp (1)
17-18: Minor inconsistency in component namespace qualification.Line 17 uses
DefaultPipeline::Component::GPUCamera(fully qualified) while line 43 usesComponent::GPUCamera(relative). Both resolve correctly, but consider using consistent qualification style throughout the file.Also applies to: 43-44
src/plugin/default-pipeline/tests/PluginDefaultPipelineTest.cpp (1)
8-19: Consider a more descriptive test name and adding assertions.The test name "Casual" doesn't convey what's being verified. Consider renaming to something like
PluginRegistration_RunsWithoutCrashingorSmokeTest.The test currently only verifies that the plugin loads and systems run without throwing, which is a valid smoke test. However, consider adding assertions to verify expected state after
RunSystems()completes (e.g., that expected resources are registered).src/plugin/default-pipeline/src/system/initialization/CreatePointLights.hpp (1)
9-9: Minor: Add namespace closing comment for consistency.Other headers in this PR (e.g.,
UpdateGPUCameras.hpp) use} // namespace DefaultPipeline::Systemfor the closing brace. Consider adding the same comment here for consistency.Suggested fix
-} +} // namespace DefaultPipeline::Systemsrc/plugin/default-pipeline/src/component/GPUTransform.hpp (1)
11-11: Nitpick: Remove unnecessary semicolon after namespace closing brace.Namespaces don't require a semicolon after the closing brace (unlike classes/structs). This compiles fine but is unnecessary.
Suggested fix
-}; // namespace DefaultPipeline::Component +} // namespace DefaultPipeline::Componentsrc/plugin/default-pipeline/src/system/GPUComponentManagement/OnMeshDestruction.hpp (1)
6-10: Consider adding closing namespace comment for consistency.Other files in this PR include closing comments (e.g.,
// namespace DefaultPipeline::Resource). Adding one here would maintain consistency.🔧 Suggested change
namespace DefaultPipeline::System { void OnMeshDestruction(Engine::Core &core, Engine::Entity entity); -} +} // namespace DefaultPipeline::Systemsrc/plugin/default-pipeline/src/system/GPUComponentManagement/OnMaterialCreation.hpp (1)
6-10: Consider adding closing namespace comment for consistency.Same as
OnMeshDestruction.hpp— a closing comment would align with other files in this PR.🔧 Suggested change
namespace DefaultPipeline::System { void OnMaterialCreation(Engine::Core &core, Engine::Entity entity); -} +} // namespace DefaultPipeline::Systemsrc/plugin/default-pipeline/src/system/initialization/CreateDefaultRenderPipeline.hpp (1)
5-7: LGTM with a minor style suggestion.The namespace migration is correct. For consistency with other headers in this PR (e.g.,
UpdateGPUMaterials.hpp,UpdatePointLights.cpp), consider adding the closing namespace comment.♻️ Optional style improvement
namespace DefaultPipeline::System { void CreateDefaultRenderPipeline(Engine::Core &core); -} +} // namespace DefaultPipeline::Systemsrc/plugin/default-pipeline/src/system/GPUComponentManagement/OnTransformCreation.hpp (1)
6-10: Namespace migration is correct.The function declaration is properly moved to
DefaultPipeline::System. Consider adding a closing namespace comment for consistency with other headers in this PR (e.g.,CreateAmbientLight.hppuses// namespace DefaultPipeline::System).♻️ Optional: Add closing namespace comment for consistency
namespace DefaultPipeline::System { void OnTransformCreation(Engine::Core &core, Engine::Entity entity); -} +} // namespace DefaultPipeline::Systemsrc/plugin/default-pipeline/src/system/GPUComponentManagement/OnMaterialDestruction.hpp (1)
6-10: Namespace change looks good, but missing closing comment.The namespace refactoring to
DefaultPipeline::Systemis consistent with the PR objectives. However, for consistency with other files in this PR (e.g.,UpdateAmbientLight.hpp,CreateDefaultMaterial.hpp), consider adding the closing namespace comment.✨ Suggested improvement
namespace DefaultPipeline::System { void OnMaterialDestruction(Engine::Core &core, Engine::Entity entity); -} +} // namespace DefaultPipeline::Systemsrc/plugin/graphic/src/system/preparation/PrepareEndRenderTexture.hpp (1)
8-9: Acknowledged TODO for future cleanup.The TODO comment correctly identifies that
END_DEPTH_RENDER_TEXTURE_IDshould eventually be moved out of the Graphic plugin to align with the refactoring goals (similar to howEND_RENDER_TEXTURE_IDwas moved toGraphic::Utils).Would you like me to open an issue to track this cleanup task?
src/plugin/default-pipeline/src/resource/buffer/PointLightsBuffer.hpp (1)
42-57: Consider extracting buffer creation to a private helper for consistency.Unlike sibling buffer classes (
AmbientLightBuffer,TransformGPUBuffer,CameraGPUBuffer) that use a private_CreateBufferhelper method, this class inlines the buffer creation logic directly inCreate(). Consider extracting to a_CreateBuffermethod for consistency.♻️ Optional refactor for consistency
void Create(Engine::Core &core) override { const auto &context = core.GetResource<Graphic::Resource::Context>(); - wgpu::BufferDescriptor bufferDesc(wgpu::Default); - bufferDesc.usage = wgpu::BufferUsage::CopyDst | wgpu::BufferUsage::Uniform; - bufferDesc.size = sizeof(PointLightsData); - bufferDesc.label = wgpu::StringView(_debugName); - - _buffer = context.deviceContext.GetDevice()->createBuffer(bufferDesc); + _buffer = _CreateBuffer(context.deviceContext); _isCreated = true; PointLightsData data{}; data.count = 0; context.queue->writeBuffer(_buffer, 0, &data, sizeof(PointLightsData)); } + +private: +wgpu::Buffer _CreateBuffer(const Graphic::Resource::DeviceContext &context) +{ + wgpu::BufferDescriptor bufferDesc(wgpu::Default); + bufferDesc.usage = wgpu::BufferUsage::CopyDst | wgpu::BufferUsage::Uniform; + bufferDesc.size = sizeof(PointLightsData); + bufferDesc.label = wgpu::StringView(_debugName); + return context.GetDevice()->createBuffer(bufferDesc); +}
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (78)
examples/graphic_light_usage/src/main.cppexamples/graphic_light_usage/xmake.luaexamples/graphic_material_usage/src/main.cppexamples/graphic_material_usage/xmake.luaexamples/graphic_usage/src/main.cppexamples/graphic_usage/xmake.luasrc/plugin/default-pipeline/src/DefaultPipeline.hppsrc/plugin/default-pipeline/src/component/GPUCamera.hppsrc/plugin/default-pipeline/src/component/GPUMaterial.hppsrc/plugin/default-pipeline/src/component/GPUMesh.hppsrc/plugin/default-pipeline/src/component/GPUTransform.hppsrc/plugin/default-pipeline/src/plugin/PluginDefaultPipeline.cppsrc/plugin/default-pipeline/src/plugin/PluginDefaultPipeline.hppsrc/plugin/default-pipeline/src/resource/AmbientLight.cppsrc/plugin/default-pipeline/src/resource/AmbientLight.hppsrc/plugin/default-pipeline/src/resource/buffer/AmbientLightBuffer.hppsrc/plugin/default-pipeline/src/resource/buffer/CameraGPUBuffer.hppsrc/plugin/default-pipeline/src/resource/buffer/IndexGPUBuffer.hppsrc/plugin/default-pipeline/src/resource/buffer/MaterialGPUBuffer.hppsrc/plugin/default-pipeline/src/resource/buffer/PointGPUBuffer.hppsrc/plugin/default-pipeline/src/resource/buffer/PointLightsBuffer.hppsrc/plugin/default-pipeline/src/resource/buffer/TransformGPUBuffer.hppsrc/plugin/default-pipeline/src/system/GPUComponentManagement/OnCameraCreation.cppsrc/plugin/default-pipeline/src/system/GPUComponentManagement/OnCameraCreation.hppsrc/plugin/default-pipeline/src/system/GPUComponentManagement/OnCameraDestruction.cppsrc/plugin/default-pipeline/src/system/GPUComponentManagement/OnCameraDestruction.hppsrc/plugin/default-pipeline/src/system/GPUComponentManagement/OnMaterialCreation.cppsrc/plugin/default-pipeline/src/system/GPUComponentManagement/OnMaterialCreation.hppsrc/plugin/default-pipeline/src/system/GPUComponentManagement/OnMaterialDestruction.cppsrc/plugin/default-pipeline/src/system/GPUComponentManagement/OnMaterialDestruction.hppsrc/plugin/default-pipeline/src/system/GPUComponentManagement/OnMeshCreation.cppsrc/plugin/default-pipeline/src/system/GPUComponentManagement/OnMeshCreation.hppsrc/plugin/default-pipeline/src/system/GPUComponentManagement/OnMeshDestruction.cppsrc/plugin/default-pipeline/src/system/GPUComponentManagement/OnMeshDestruction.hppsrc/plugin/default-pipeline/src/system/GPUComponentManagement/OnTransformCreation.cppsrc/plugin/default-pipeline/src/system/GPUComponentManagement/OnTransformCreation.hppsrc/plugin/default-pipeline/src/system/GPUComponentManagement/OnTransformDestruction.cppsrc/plugin/default-pipeline/src/system/GPUComponentManagement/OnTransformDestruction.hppsrc/plugin/default-pipeline/src/system/initialization/CreateAmbientLight.cppsrc/plugin/default-pipeline/src/system/initialization/CreateAmbientLight.hppsrc/plugin/default-pipeline/src/system/initialization/CreateDefaultMaterial.cppsrc/plugin/default-pipeline/src/system/initialization/CreateDefaultMaterial.hppsrc/plugin/default-pipeline/src/system/initialization/CreateDefaultRenderPipeline.cppsrc/plugin/default-pipeline/src/system/initialization/CreateDefaultRenderPipeline.hppsrc/plugin/default-pipeline/src/system/initialization/CreatePointLights.cppsrc/plugin/default-pipeline/src/system/initialization/CreatePointLights.hppsrc/plugin/default-pipeline/src/system/preparation/UpdateAmbientLight.cppsrc/plugin/default-pipeline/src/system/preparation/UpdateAmbientLight.hppsrc/plugin/default-pipeline/src/system/preparation/UpdateGPUCameras.cppsrc/plugin/default-pipeline/src/system/preparation/UpdateGPUCameras.hppsrc/plugin/default-pipeline/src/system/preparation/UpdateGPUMaterials.cppsrc/plugin/default-pipeline/src/system/preparation/UpdateGPUMaterials.hppsrc/plugin/default-pipeline/src/system/preparation/UpdateGPUTransforms.cppsrc/plugin/default-pipeline/src/system/preparation/UpdateGPUTransforms.hppsrc/plugin/default-pipeline/src/system/preparation/UpdatePointLight.cppsrc/plugin/default-pipeline/src/system/preparation/UpdatePointLight.hppsrc/plugin/default-pipeline/src/system/preparation/UpdatePointLights.cppsrc/plugin/default-pipeline/src/system/preparation/UpdatePointLights.hppsrc/plugin/default-pipeline/src/utils/AmbientLight.hppsrc/plugin/default-pipeline/src/utils/DefaultMaterial.hppsrc/plugin/default-pipeline/src/utils/DefaultRenderPass.hppsrc/plugin/default-pipeline/src/utils/PointLights.hppsrc/plugin/default-pipeline/tests/PluginDefaultPipelineTest.cppsrc/plugin/default-pipeline/tests/main.cppsrc/plugin/default-pipeline/xmake.luasrc/plugin/graphic/src/Graphic.hppsrc/plugin/graphic/src/plugin/PluginGraphic.cppsrc/plugin/graphic/src/resource/PointLights.hppsrc/plugin/graphic/src/system/commandCreation/ExecuteRenderPass.cppsrc/plugin/graphic/src/system/initialization/CreatePointLights.cppsrc/plugin/graphic/src/system/preparation/PrepareEndRenderTexture.cppsrc/plugin/graphic/src/system/preparation/PrepareEndRenderTexture.hppsrc/plugin/graphic/src/system/preparation/UpdatePointLights.cppsrc/plugin/graphic/src/utils/EndRenderTexture.hppsrc/plugin/graphic/tests/BufferTest.cppsrc/plugin/graphic/tests/PrepareEndRenderTextureTest.cppsrc/plugin/graphic/xmake.luaxmake.lua
💤 Files with no reviewable changes (3)
- src/plugin/graphic/src/system/initialization/CreatePointLights.cpp
- src/plugin/graphic/src/system/preparation/UpdatePointLights.cpp
- src/plugin/graphic/src/resource/PointLights.hpp
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2026-01-11T10:21:30.302Z
Learnt from: ripel2
Repo: EngineSquared/EngineSquared PR: 423
File: examples/graphic_usage_with_physics/src/main.cpp:16-16
Timestamp: 2026-01-11T10:21:30.302Z
Learning: In the EngineSquared repository, examples located under the examples/ directory may not need strict production-quality rules (e.g., removing unused includes). Reviews of example files should focus on demonstrating functionality and correct behavior rather than enforcing full production-grade code cleanliness.
Applied to files:
examples/graphic_usage/src/main.cppexamples/graphic_light_usage/src/main.cppexamples/graphic_material_usage/src/main.cpp
📚 Learning: 2026-01-11T10:25:31.402Z
Learnt from: ripel2
Repo: EngineSquared/EngineSquared PR: 423
File: src/plugin/camera-movement/src/plugin/PluginCameraMovement.hpp:7-17
Timestamp: 2026-01-11T10:25:31.402Z
Learning: The Object plugin in the EngineSquared codebase is a static library (target "PluginObject") that provides components like Transform and Camera. It does not have an APlugin implementation or Object::Plugin class, so it should never be added to RequirePlugins(). Object components are made available through build-time dependencies via add_deps("PluginObject") in xmake.lua files.
Applied to files:
src/plugin/default-pipeline/src/plugin/PluginDefaultPipeline.hpp
📚 Learning: 2026-01-11T10:25:23.874Z
Learnt from: ripel2
Repo: EngineSquared/EngineSquared PR: 423
File: src/plugin/camera-movement/src/plugin/PluginCameraMovement.hpp:7-17
Timestamp: 2026-01-11T10:25:23.874Z
Learning: In EngineSquared, PluginObject is a static library exposing Object components (e.g., Transform, Camera). Do not add an APlugin or Object::Plugin to RequirePlugins(). Instead, make object components available via build-time dependencies by adding add_deps("PluginObject") in xmake.lua files that depend on those components. This rule applies across all xmake.lua configurations that reference PluginObject.
Applied to files:
examples/graphic_material_usage/xmake.luaexamples/graphic_light_usage/xmake.luaxmake.luaexamples/graphic_usage/xmake.luasrc/plugin/graphic/xmake.luasrc/plugin/default-pipeline/xmake.lua
🧬 Code graph analysis (33)
src/plugin/default-pipeline/src/system/GPUComponentManagement/OnMaterialDestruction.cpp (1)
src/plugin/default-pipeline/src/system/GPUComponentManagement/OnMaterialDestruction.hpp (1)
OnMaterialDestruction(8-8)
src/plugin/default-pipeline/src/system/GPUComponentManagement/OnMeshCreation.cpp (2)
src/plugin/default-pipeline/src/system/GPUComponentManagement/OnMeshCreation.hpp (1)
OnMeshCreation(8-8)src/engine/src/entity/Entity.hpp (1)
static_cast(65-65)
examples/graphic_usage/src/main.cpp (3)
src/plugin/default-pipeline/src/resource/AmbientLight.hpp (5)
core(16-16)core(17-17)core(18-18)core(19-19)core(22-22)src/plugin/default-pipeline/src/resource/buffer/AmbientLightBuffer.hpp (10)
core(40-46)core(40-40)core(47-54)core(47-47)core(56-56)core(56-56)core(57-65)core(57-57)core(69-79)core(69-69)src/plugin/default-pipeline/src/resource/buffer/CameraGPUBuffer.hpp (1)
core(15-24)
src/plugin/default-pipeline/src/utils/AmbientLight.hpp (1)
src/plugin/default-pipeline/src/resource/AmbientLight.hpp (1)
AMBIENT_LIGHT_BUFFER_ID(14-14)
src/plugin/default-pipeline/src/system/preparation/UpdatePointLights.hpp (2)
src/plugin/default-pipeline/src/resource/buffer/AmbientLightBuffer.hpp (10)
core(40-46)core(40-40)core(47-54)core(47-47)core(56-56)core(56-56)core(57-65)core(57-57)core(69-79)core(69-69)src/plugin/default-pipeline/src/resource/buffer/CameraGPUBuffer.hpp (1)
core(15-24)
src/plugin/default-pipeline/src/system/initialization/CreateAmbientLight.cpp (3)
src/plugin/default-pipeline/src/resource/AmbientLight.hpp (5)
core(16-16)core(17-17)core(18-18)core(19-19)core(22-22)src/plugin/default-pipeline/src/resource/buffer/AmbientLightBuffer.hpp (10)
core(40-46)core(40-40)core(47-54)core(47-47)core(56-56)core(56-56)core(57-65)core(57-57)core(69-79)core(69-69)src/plugin/default-pipeline/src/resource/buffer/CameraGPUBuffer.hpp (1)
core(15-24)
src/plugin/default-pipeline/src/system/preparation/UpdateGPUMaterials.hpp (2)
src/plugin/default-pipeline/src/resource/buffer/AmbientLightBuffer.hpp (10)
core(40-46)core(40-40)core(47-54)core(47-47)core(56-56)core(56-56)core(57-65)core(57-57)core(69-79)core(69-69)src/plugin/default-pipeline/src/resource/buffer/CameraGPUBuffer.hpp (1)
core(15-24)
src/plugin/default-pipeline/src/system/preparation/UpdateAmbientLight.cpp (3)
src/plugin/default-pipeline/src/resource/AmbientLight.hpp (5)
core(16-16)core(17-17)core(18-18)core(19-19)core(22-22)src/plugin/default-pipeline/src/resource/buffer/AmbientLightBuffer.hpp (10)
core(40-46)core(40-40)core(47-54)core(47-47)core(56-56)core(56-56)core(57-65)core(57-57)core(69-79)core(69-69)src/plugin/default-pipeline/src/resource/buffer/CameraGPUBuffer.hpp (1)
core(15-24)
src/plugin/default-pipeline/src/system/initialization/CreateAmbientLight.hpp (3)
src/plugin/default-pipeline/src/resource/AmbientLight.hpp (5)
core(16-16)core(17-17)core(18-18)core(19-19)core(22-22)src/plugin/default-pipeline/src/resource/buffer/AmbientLightBuffer.hpp (10)
core(40-46)core(40-40)core(47-54)core(47-47)core(56-56)core(56-56)core(57-65)core(57-57)core(69-79)core(69-69)src/plugin/default-pipeline/src/resource/buffer/CameraGPUBuffer.hpp (1)
core(15-24)
src/plugin/default-pipeline/src/system/preparation/UpdateGPUCameras.cpp (2)
src/plugin/default-pipeline/src/system/preparation/UpdateGPUCameras.hpp (1)
UpdateGPUCameras(7-7)src/plugin/default-pipeline/src/component/GPUCamera.hpp (2)
camera(23-30)camera(23-23)
src/plugin/default-pipeline/src/system/GPUComponentManagement/OnTransformDestruction.cpp (1)
src/plugin/default-pipeline/src/system/GPUComponentManagement/OnTransformDestruction.hpp (1)
OnTransformDestruction(8-8)
src/plugin/default-pipeline/src/system/preparation/UpdateGPUCameras.hpp (2)
src/plugin/default-pipeline/src/resource/buffer/AmbientLightBuffer.hpp (10)
core(40-46)core(40-40)core(47-54)core(47-47)core(56-56)core(56-56)core(57-65)core(57-57)core(69-79)core(69-69)src/plugin/default-pipeline/src/resource/buffer/CameraGPUBuffer.hpp (1)
core(15-24)
src/plugin/default-pipeline/src/system/initialization/CreateDefaultMaterial.hpp (3)
src/plugin/default-pipeline/src/resource/AmbientLight.hpp (5)
core(16-16)core(17-17)core(18-18)core(19-19)core(22-22)src/plugin/default-pipeline/src/resource/buffer/AmbientLightBuffer.hpp (10)
core(40-46)core(40-40)core(47-54)core(47-47)core(56-56)core(56-56)core(57-65)core(57-57)core(69-79)core(69-69)src/plugin/default-pipeline/src/resource/buffer/CameraGPUBuffer.hpp (1)
core(15-24)
src/plugin/default-pipeline/src/resource/buffer/PointLightsBuffer.hpp (1)
src/plugin/default-pipeline/src/resource/buffer/AmbientLightBuffer.hpp (14)
context(86-94)context(86-86)context(96-101)context(96-97)core(40-46)core(40-40)core(47-54)core(47-47)core(56-56)core(56-56)core(57-65)core(57-57)core(69-79)core(69-69)
src/plugin/default-pipeline/src/resource/buffer/AmbientLightBuffer.hpp (3)
src/plugin/default-pipeline/src/resource/buffer/CameraGPUBuffer.hpp (6)
context(51-61)context(51-51)core(15-24)core(15-15)core(37-46)core(37-37)src/plugin/default-pipeline/src/resource/buffer/MaterialGPUBuffer.hpp (2)
context(84-92)context(84-84)src/plugin/default-pipeline/src/resource/buffer/TransformGPUBuffer.hpp (2)
context(71-81)context(71-71)
src/plugin/graphic/src/system/commandCreation/ExecuteRenderPass.cpp (2)
src/plugin/graphic/src/resource/RenderGraph.hpp (4)
name(47-47)name(49-49)name(53-53)name(53-53)src/plugin/graphic/src/system/commandCreation/ExecuteRenderPass.hpp (1)
ExecuteRenderPass(7-7)
src/plugin/default-pipeline/src/resource/AmbientLight.cpp (2)
src/plugin/default-pipeline/src/resource/buffer/AmbientLightBuffer.hpp (10)
core(40-46)core(40-40)core(47-54)core(47-47)core(56-56)core(56-56)core(57-65)core(57-57)core(69-79)core(69-69)src/plugin/default-pipeline/src/resource/buffer/CameraGPUBuffer.hpp (1)
core(15-24)
src/plugin/default-pipeline/src/resource/buffer/MaterialGPUBuffer.hpp (3)
src/plugin/default-pipeline/src/resource/buffer/CameraGPUBuffer.hpp (5)
context(51-61)context(51-51)core(15-24)_isCreated(26-33)_isCreated(35-35)src/plugin/default-pipeline/src/resource/buffer/TransformGPUBuffer.hpp (2)
context(71-81)context(71-71)src/plugin/default-pipeline/src/resource/AmbientLight.hpp (5)
core(16-16)core(17-17)core(18-18)core(19-19)core(22-22)
src/plugin/default-pipeline/src/resource/buffer/CameraGPUBuffer.hpp (4)
src/plugin/default-pipeline/src/resource/buffer/IndexGPUBuffer.hpp (2)
core(15-40)_buffer(64-64)src/plugin/default-pipeline/src/resource/buffer/TransformGPUBuffer.hpp (3)
context(71-81)context(71-71)_buffer(68-68)src/plugin/default-pipeline/src/resource/buffer/PointGPUBuffer.hpp (1)
_buffer(78-78)src/plugin/graphic/tests/BufferTest.cpp (1)
_buffer(43-43)
src/plugin/default-pipeline/src/system/preparation/UpdateGPUTransforms.cpp (1)
src/plugin/default-pipeline/src/system/preparation/UpdateGPUTransforms.hpp (1)
UpdateGPUTransforms(7-7)
src/plugin/graphic/src/plugin/PluginGraphic.cpp (6)
src/plugin/graphic/src/system/initialization/CreateDefaultSampler.cpp (2)
CreateDefaultSampler(8-16)CreateDefaultSampler(8-8)src/plugin/graphic/src/system/initialization/CreateDefaultSampler.hpp (1)
CreateDefaultSampler(6-6)src/plugin/graphic/src/system/initialization/SetupResizableRenderTexture.cpp (2)
SetupResizableRenderTexture(11-17)SetupResizableRenderTexture(11-11)src/plugin/graphic/src/system/initialization/SetupResizableRenderTexture.hpp (1)
SetupResizableRenderTexture(7-7)src/plugin/graphic/src/system/preparation/PrepareEndRenderTexture.cpp (2)
PrepareEndRenderTexture(114-130)PrepareEndRenderTexture(114-114)src/plugin/graphic/src/system/preparation/PrepareEndRenderTexture.hpp (1)
PrepareEndRenderTexture(11-11)
src/plugin/default-pipeline/src/resource/buffer/IndexGPUBuffer.hpp (2)
src/plugin/default-pipeline/src/resource/buffer/CameraGPUBuffer.hpp (3)
_buffer(48-48)core(15-24)bufferDesc(53-53)src/plugin/graphic/tests/BufferTest.cpp (2)
_buffer(43-43)bufferDesc(16-16)
src/plugin/default-pipeline/src/system/GPUComponentManagement/OnMeshDestruction.cpp (1)
src/plugin/default-pipeline/src/system/GPUComponentManagement/OnMeshDestruction.hpp (1)
OnMeshDestruction(8-8)
src/plugin/default-pipeline/src/system/GPUComponentManagement/OnCameraCreation.cpp (2)
src/plugin/default-pipeline/src/system/GPUComponentManagement/OnCameraCreation.hpp (1)
OnCameraCreation(8-8)src/plugin/default-pipeline/src/resource/buffer/CameraGPUBuffer.hpp (1)
core(15-24)
src/plugin/default-pipeline/src/system/preparation/UpdateGPUMaterials.cpp (1)
src/plugin/default-pipeline/src/system/preparation/UpdateGPUMaterials.hpp (1)
UpdateGPUMaterials(7-7)
src/plugin/default-pipeline/src/system/initialization/CreatePointLights.cpp (3)
src/plugin/default-pipeline/src/resource/AmbientLight.hpp (6)
core(16-16)core(17-17)core(18-18)core(19-19)core(22-22)AMBIENT_LIGHT_BUFFER_ID(14-14)src/plugin/default-pipeline/src/resource/buffer/AmbientLightBuffer.hpp (10)
core(40-46)core(40-40)core(47-54)core(47-47)core(56-56)core(56-56)core(57-65)core(57-57)core(69-79)core(69-69)src/plugin/default-pipeline/src/resource/buffer/CameraGPUBuffer.hpp (1)
core(15-24)
src/plugin/default-pipeline/src/system/initialization/CreateDefaultRenderPipeline.cpp (4)
src/plugin/default-pipeline/src/system/initialization/CreateDefaultRenderPipeline.hpp (1)
CreateDefaultRenderPipeline(6-6)src/plugin/default-pipeline/src/resource/buffer/AmbientLightBuffer.hpp (10)
core(40-46)core(40-40)core(47-54)core(47-47)core(56-56)core(56-56)core(57-65)core(57-57)core(69-79)core(69-69)src/plugin/default-pipeline/src/resource/buffer/CameraGPUBuffer.hpp (1)
core(15-24)src/plugin/default-pipeline/src/utils/DefaultRenderPass.hpp (2)
renderPass(156-204)renderPass(156-156)
src/plugin/default-pipeline/src/system/initialization/CreateDefaultMaterial.cpp (3)
src/plugin/default-pipeline/src/resource/AmbientLight.hpp (5)
core(16-16)core(17-17)core(18-18)core(19-19)core(22-22)src/plugin/default-pipeline/src/resource/buffer/AmbientLightBuffer.hpp (10)
core(40-46)core(40-40)core(47-54)core(47-47)core(56-56)core(56-56)core(57-65)core(57-57)core(69-79)core(69-69)src/plugin/default-pipeline/src/resource/buffer/CameraGPUBuffer.hpp (1)
core(15-24)
src/plugin/default-pipeline/src/resource/buffer/TransformGPUBuffer.hpp (3)
src/plugin/default-pipeline/src/resource/buffer/AmbientLightBuffer.hpp (17)
entity(67-67)entity(67-67)core(40-46)core(40-40)core(47-54)core(47-47)core(56-56)core(56-56)core(57-65)core(57-57)core(69-79)core(69-69)context(86-94)context(86-86)context(96-101)context(96-97)_buffer(81-81)src/plugin/default-pipeline/src/resource/buffer/CameraGPUBuffer.hpp (4)
core(15-24)context(51-61)context(51-51)_buffer(48-48)src/plugin/default-pipeline/src/resource/buffer/MaterialGPUBuffer.hpp (3)
context(84-92)context(84-84)_buffer(79-79)
src/plugin/default-pipeline/src/system/preparation/UpdatePointLights.cpp (3)
src/plugin/default-pipeline/src/resource/AmbientLight.hpp (5)
core(16-16)core(17-17)core(18-18)core(19-19)core(22-22)src/plugin/default-pipeline/src/resource/buffer/AmbientLightBuffer.hpp (10)
core(40-46)core(40-40)core(47-54)core(47-47)core(56-56)core(56-56)core(57-65)core(57-57)core(69-79)core(69-69)src/plugin/default-pipeline/src/resource/buffer/CameraGPUBuffer.hpp (1)
core(15-24)
src/plugin/default-pipeline/src/system/GPUComponentManagement/OnCameraDestruction.cpp (2)
src/plugin/default-pipeline/src/system/GPUComponentManagement/OnCameraDestruction.hpp (1)
OnCameraDestruction(8-8)src/plugin/default-pipeline/src/resource/buffer/CameraGPUBuffer.hpp (1)
core(15-24)
src/plugin/default-pipeline/src/utils/DefaultRenderPass.hpp (3)
src/plugin/default-pipeline/src/component/GPUCamera.hpp (2)
camera(23-30)camera(23-23)src/plugin/default-pipeline/src/resource/buffer/AmbientLightBuffer.hpp (12)
core(40-46)core(40-40)core(47-54)core(47-47)core(56-56)core(56-56)core(57-65)core(57-57)core(69-79)core(69-69)entity(67-67)entity(67-67)src/plugin/default-pipeline/src/resource/buffer/MaterialGPUBuffer.hpp (2)
materialComponent(94-98)materialComponent(94-94)
src/plugin/default-pipeline/src/system/GPUComponentManagement/OnMaterialCreation.cpp (3)
src/plugin/default-pipeline/src/system/GPUComponentManagement/OnMaterialCreation.hpp (1)
OnMaterialCreation(8-8)src/plugin/default-pipeline/src/resource/buffer/CameraGPUBuffer.hpp (3)
core(15-24)context(51-61)context(51-51)src/plugin/default-pipeline/src/resource/buffer/MaterialGPUBuffer.hpp (2)
context(84-92)context(84-84)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Run tests (ubuntu-latest)
- GitHub Check: Build and verify binaries (windows-latest)
- GitHub Check: Build and verify binaries (ubuntu-latest)
- GitHub Check: Build and verify binaries (macos-latest)
- GitHub Check: Run tests (macos-latest)
- GitHub Check: Run tests (windows-latest)
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @src/plugin/default-pipeline/src/DefaultPipeline.hpp:
- Around line 1-6: The header aggregator is missing the material component and
its GPU buffer: add #include "component/GPUMaterial.hpp" so
DefaultRenderPass.hpp can use Component::GPUMaterial without incomplete-type
errors, and add #include "resource/buffer/MaterialGPUBuffer.hpp" to match the
other *GPUBuffer includes so functions like CreateDefaultMaterial and
OnMaterialCreation that reference MaterialGPUBuffer compile correctly; update
the includes in DefaultPipeline.hpp accordingly.
In @src/plugin/default-pipeline/src/utils/DefaultRenderPass.hpp:
- Around line 158-164: In DefaultRenderPass::UniqueRenderCallback, avoid UB by
not calling .front() on an entt::view before verifying it contains elements:
first get the view from core.GetRegistry().view<Component::GPUCamera>(), check
view.empty() (or view.begin() == view.end()) and log/return if no camera exists,
then call view.front() to construct Engine::Entity camera and proceed; update
any uses of core.IsEntityValid(camera) accordingly so the emptiness check occurs
before .front() is invoked.
🧹 Nitpick comments (2)
src/plugin/object/src/resource/ResourceManager.hpp (1)
184-198: Add[[nodiscard]]attribute and documentation for consistency.The new
GetDefault()methods are missing:
[[nodiscard]]attribute — all other getter methods (Get,GetOrDefault) use it.- Doxygen documentation — all other public methods in this class are documented.
♻️ Suggested fix
+ /** + * @brief Get the default resource. + * + * @exception ResourceManagerError if no default resource is set. + * + * @return the default resource + */ + [[nodiscard]] ResourceType &GetDefault() - ResourceType &GetDefault() { if (!defaultResource.has_value()) throw ResourceManagerError("No default resource is set."); return *defaultResource; } + /** + * @brief Get the default resource. + * + * @exception ResourceManagerError if no default resource is set. + * + * @return the default resource + */ + [[nodiscard]] const ResourceType &GetDefault() const - const ResourceType &GetDefault() const { if (!defaultResource.has_value()) throw ResourceManagerError("No default resource is set."); return *defaultResource; }src/plugin/default-pipeline/xmake.lua (1)
63-63: Use separateadd_depscalls for consistency across the codebase.This line mixes a string (
plugin_name) with a table (target_dependencies), whereas line 31 in the same file and virtually all other test targets in the codebase calladd_depsseparately for each dependency. The overwhelming pattern in the codebase is individual string arguments or a single table, not mixed arguments.♻️ Suggested refactor for consistency
- add_deps(plugin_name, target_dependencies) + add_deps(plugin_name) + add_deps(target_dependencies)
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/plugin/default-pipeline/src/DefaultPipeline.hppsrc/plugin/default-pipeline/src/utils/DefaultRenderPass.hppsrc/plugin/default-pipeline/xmake.luasrc/plugin/graphic/src/system/commandCreation/ExecuteRenderPass.cppsrc/plugin/object/src/resource/ResourceManager.hpp
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-01-11T10:25:23.874Z
Learnt from: ripel2
Repo: EngineSquared/EngineSquared PR: 423
File: src/plugin/camera-movement/src/plugin/PluginCameraMovement.hpp:7-17
Timestamp: 2026-01-11T10:25:23.874Z
Learning: In EngineSquared, PluginObject is a static library exposing Object components (e.g., Transform, Camera). Do not add an APlugin or Object::Plugin to RequirePlugins(). Instead, make object components available via build-time dependencies by adding add_deps("PluginObject") in xmake.lua files that depend on those components. This rule applies across all xmake.lua configurations that reference PluginObject.
Applied to files:
src/plugin/default-pipeline/xmake.lua
🧬 Code graph analysis (2)
src/plugin/object/src/resource/ResourceManager.hpp (1)
src/plugin/object/src/exception/ResourceManagerError.hpp (2)
ResourceManagerError(28-28)ResourceManagerError(28-28)
src/plugin/default-pipeline/src/utils/DefaultRenderPass.hpp (6)
src/plugin/default-pipeline/src/component/GPUCamera.hpp (2)
camera(23-30)camera(23-23)src/plugin/default-pipeline/src/resource/AmbientLight.hpp (5)
core(16-16)core(17-17)core(18-18)core(19-19)core(22-22)src/plugin/default-pipeline/src/resource/buffer/CameraGPUBuffer.hpp (4)
core(15-24)core(15-15)core(37-46)core(37-37)src/plugin/default-pipeline/src/resource/buffer/IndexGPUBuffer.hpp (7)
core(15-40)core(15-15)core(41-48)core(41-41)core(50-50)core(50-50)core(51-62)src/plugin/default-pipeline/src/resource/buffer/AmbientLightBuffer.hpp (2)
entity(67-67)entity(67-67)src/plugin/default-pipeline/src/resource/buffer/MaterialGPUBuffer.hpp (2)
materialComponent(94-98)materialComponent(94-94)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Run tests (ubuntu-latest)
- GitHub Check: Run tests (windows-latest)
- GitHub Check: Build and verify binaries (windows-latest)
🔇 Additional comments (10)
src/plugin/default-pipeline/xmake.lua (3)
1-22: LGTM on includes and dependency declarations.The package list and target dependencies are appropriate for a graphics pipeline plugin. The inclusion of
PluginGraphicas a dependency aligns with the PR's goal of separating the default pipeline from the core graphic plugin.
24-45: Main target configuration looks correct.The static library setup with C++20, packages, dependencies, and header/source organization follows established patterns. Consider adding
target_end()after line 45 before the test loop for explicit target closure, though xmake handles the implicit closure when the nexttarget()is called.
47-73: Test generation loop structure is correct.The
goto continue/::continue::pattern is valid Lua for implementing continue behavior. Skippingmain.cppand adding it separately to each test target is the correct approach for gtest.src/plugin/graphic/src/system/commandCreation/ExecuteRenderPass.cpp (1)
8-9: LGTM - Simplified execution flow looks correct.The delegation to the default render graph aligns with the refactoring objectives. Note that if no default render graph exists, the function silently returns. Verify this is the intended behavior when only custom pipelines are registered.
src/plugin/default-pipeline/src/utils/DefaultRenderPass.hpp (5)
16-22: LGTM - Namespace relocation is consistent.The include path and namespace changes correctly reflect the migration to
DefaultPipeline::Utils.
166-192: LGTM - Component qualifications are consistent.The namespace-relative qualifications (
Component::GPUCamera,Component::GPUMaterial, etc.) correctly reference theDefaultPipeline::Componenttypes within theDefaultPipeline::Utilsnamespace scope.
251-255: LGTM - Buffer size reference updated correctly.The
Resource::PointLightsBuffer::GPUSize()reference correctly uses the relative namespace path withinDefaultPipeline::Utils.
286-298: LGTM - Validation error severity references are correct.The fully qualified
Graphic::Utils::ValidationError::Severityis appropriate sinceValidationErrorremains in theGraphicplugin while this code is now inDefaultPipeline::Utils.
303-303: LGTM - Namespace closing is correct.src/plugin/default-pipeline/src/DefaultPipeline.hpp (1)
7-40: LGTM - Well-organized aggregator header.The includes are logically grouped by category (plugin, resources, systems, utils) with clear visual separation. This structure will aid maintainability as the DefaultPipeline grows.
…ove resource management
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/plugin/graphic/tests/BufferTest.cpp (1)
50-66: LGTM!The test covers the complete GPU buffer lifecycle effectively.
Consider also testing that
Updatethrows afterDestroyto verify the guard inUpdateworks correctly in the post-destruction state.🧪 Optional: Add post-destroy Update test
buffer.Destroy(core); EXPECT_FALSE(buffer.IsCreated(core)); + + EXPECT_THROW(buffer.Update(core), Graphic::Exception::UpdateBufferError); }src/plugin/default-pipeline/src/resource/buffer/IndexGPUBuffer.hpp (1)
52-64: Unused variable inUpdate()method.The
meshComponentvariable fetched on line 60 is never used. While the comment explains dynamic resizing isn't implemented, the unused variable should be removed or commented out to avoid compiler warnings and clarify intent.Proposed fix
void Update(Engine::Core &core) override { if (!_isCreated) { throw Graphic::Exception::UpdateBufferError("Cannot update a GPU buffer that is not created."); } - auto &meshComponent = core.GetRegistry().get<Object::Component::Mesh>(_entity); - // For now, we will not implement dynamic resizing of the buffer. As we should have a way to know if the size // changed. And it would be so heavy to check every frame every index. + // TODO: Implement dynamic resizing when mesh indices change. };src/plugin/default-pipeline/tests/PluginDefaultPipelineTest.cpp (1)
8-20: Consider adding assertions to verify plugin initialization state.This smoke test only verifies that the plugin doesn't crash during initialization. Consider adding assertions to confirm expected resources or components are registered, which would catch initialization regressions more effectively.
Example additions:
// After RunSystems(), verify expected resources exist EXPECT_NO_THROW(core.GetResource<Graphic::Resource::Context>()); // Or verify specific systems were registered
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
src/plugin/default-pipeline/src/DefaultPipeline.hppsrc/plugin/default-pipeline/src/component/GPUMaterial.hppsrc/plugin/default-pipeline/src/component/GPUTransform.hppsrc/plugin/default-pipeline/src/resource/buffer/CameraGPUBuffer.hppsrc/plugin/default-pipeline/src/resource/buffer/IndexGPUBuffer.hppsrc/plugin/default-pipeline/src/resource/buffer/PointLightsBuffer.hppsrc/plugin/default-pipeline/src/system/GPUComponentManagement/OnCameraCreation.hppsrc/plugin/default-pipeline/src/system/GPUComponentManagement/OnCameraDestruction.hppsrc/plugin/default-pipeline/src/system/GPUComponentManagement/OnMaterialCreation.hppsrc/plugin/default-pipeline/src/system/GPUComponentManagement/OnMaterialDestruction.hppsrc/plugin/default-pipeline/src/system/GPUComponentManagement/OnMeshCreation.hppsrc/plugin/default-pipeline/src/system/GPUComponentManagement/OnMeshDestruction.hppsrc/plugin/default-pipeline/src/system/GPUComponentManagement/OnTransformCreation.hppsrc/plugin/default-pipeline/src/system/GPUComponentManagement/OnTransformDestruction.hppsrc/plugin/default-pipeline/src/system/initialization/CreateDefaultRenderPipeline.hppsrc/plugin/default-pipeline/src/system/initialization/CreatePointLights.hppsrc/plugin/default-pipeline/src/utils/DefaultRenderPass.hppsrc/plugin/default-pipeline/tests/PluginDefaultPipelineTest.cppsrc/plugin/default-pipeline/xmake.luasrc/plugin/graphic/tests/BufferTest.cppsrc/plugin/object/src/resource/ResourceManager.hpp
🚧 Files skipped from review as they are similar to previous changes (5)
- src/plugin/default-pipeline/src/system/GPUComponentManagement/OnMeshCreation.hpp
- src/plugin/default-pipeline/src/DefaultPipeline.hpp
- src/plugin/default-pipeline/src/utils/DefaultRenderPass.hpp
- src/plugin/default-pipeline/src/system/GPUComponentManagement/OnMaterialCreation.hpp
- src/plugin/default-pipeline/src/system/GPUComponentManagement/OnTransformDestruction.hpp
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-01-11T10:25:23.874Z
Learnt from: ripel2
Repo: EngineSquared/EngineSquared PR: 423
File: src/plugin/camera-movement/src/plugin/PluginCameraMovement.hpp:7-17
Timestamp: 2026-01-11T10:25:23.874Z
Learning: In EngineSquared, PluginObject is a static library exposing Object components (e.g., Transform, Camera). Do not add an APlugin or Object::Plugin to RequirePlugins(). Instead, make object components available via build-time dependencies by adding add_deps("PluginObject") in xmake.lua files that depend on those components. This rule applies across all xmake.lua configurations that reference PluginObject.
Applied to files:
src/plugin/default-pipeline/xmake.lua
🧬 Code graph analysis (5)
src/plugin/graphic/tests/BufferTest.cpp (7)
src/plugin/default-pipeline/src/resource/buffer/CameraGPUBuffer.hpp (10)
core(15-24)core(15-15)core(37-46)core(37-37)context(51-61)context(51-51)bufferDesc(53-53)_buffer(48-48)_isCreated(26-33)_isCreated(35-35)src/plugin/default-pipeline/src/resource/buffer/IndexGPUBuffer.hpp (10)
core(15-42)core(15-15)core(43-50)core(43-43)core(52-52)core(52-52)core(53-64)core(53-53)bufferDesc(28-28)_buffer(66-66)src/plugin/graphic/src/resource/AGPUBuffer.hpp (4)
core(10-10)core(11-11)core(13-13)core(14-14)src/plugin/default-pipeline/src/resource/buffer/PointLightsBuffer.hpp (4)
context(114-122)context(114-114)bufferDesc(116-116)_buffer(107-107)src/plugin/default-pipeline/src/resource/buffer/MaterialGPUBuffer.hpp (4)
context(84-92)context(84-84)bufferDesc(86-86)_buffer(79-79)src/plugin/default-pipeline/src/resource/buffer/TransformGPUBuffer.hpp (4)
context(71-81)context(71-71)bufferDesc(73-73)_buffer(68-68)src/plugin/graphic/src/utils/webgpu.hpp (1)
StringView(619-619)
src/plugin/object/src/resource/ResourceManager.hpp (1)
src/plugin/object/src/exception/ResourceManagerError.hpp (2)
ResourceManagerError(28-28)ResourceManagerError(28-28)
src/plugin/default-pipeline/src/resource/buffer/CameraGPUBuffer.hpp (5)
src/plugin/default-pipeline/src/resource/buffer/IndexGPUBuffer.hpp (9)
core(15-42)core(15-15)core(43-50)core(43-43)core(52-52)core(52-52)core(53-64)core(53-53)_buffer(66-66)src/plugin/default-pipeline/src/resource/buffer/PointLightsBuffer.hpp (11)
core(42-52)core(42-42)core(54-61)core(54-54)core(63-63)core(63-63)core(65-105)core(65-65)context(114-122)context(114-114)_buffer(107-107)src/plugin/default-pipeline/src/resource/buffer/MaterialGPUBuffer.hpp (3)
context(84-92)context(84-84)_buffer(79-79)src/plugin/default-pipeline/src/resource/buffer/TransformGPUBuffer.hpp (3)
context(71-81)context(71-71)_buffer(68-68)src/plugin/default-pipeline/src/resource/buffer/PointGPUBuffer.hpp (1)
_buffer(78-78)
src/plugin/default-pipeline/src/resource/buffer/PointLightsBuffer.hpp (7)
src/plugin/default-pipeline/src/resource/buffer/CameraGPUBuffer.hpp (8)
context(51-61)context(51-51)core(15-24)core(15-15)core(37-46)core(37-37)_buffer(48-48)bufferDesc(53-53)src/plugin/default-pipeline/src/resource/buffer/MaterialGPUBuffer.hpp (5)
context(84-92)context(84-84)_buffer(79-79)bufferDesc(86-86)_debugName(81-81)src/plugin/default-pipeline/src/resource/buffer/AmbientLightBuffer.hpp (7)
context(86-94)context(86-86)context(96-101)context(96-97)_buffer(81-81)bufferDesc(88-88)_debugName(83-83)src/plugin/default-pipeline/src/resource/buffer/TransformGPUBuffer.hpp (4)
context(71-81)context(71-71)_buffer(68-68)bufferDesc(73-73)src/plugin/default-pipeline/src/resource/buffer/IndexGPUBuffer.hpp (10)
core(15-42)core(15-15)core(43-50)core(43-43)core(52-52)core(52-52)core(53-64)core(53-53)_buffer(66-66)bufferDesc(28-28)src/plugin/graphic/tests/BufferTest.cpp (6)
core(11-23)core(11-11)core(33-41)core(33-33)_buffer(42-42)bufferDesc(15-15)src/plugin/graphic/src/utils/webgpu.hpp (1)
StringView(619-619)
src/plugin/default-pipeline/src/resource/buffer/IndexGPUBuffer.hpp (5)
src/plugin/default-pipeline/src/resource/buffer/CameraGPUBuffer.hpp (8)
context(51-61)context(51-51)core(15-24)core(15-15)core(37-46)core(37-37)_buffer(48-48)bufferDesc(53-53)src/plugin/default-pipeline/src/resource/buffer/PointLightsBuffer.hpp (12)
context(114-122)context(114-114)core(42-52)core(42-42)core(54-61)core(54-54)core(63-63)core(63-63)core(65-105)core(65-65)_buffer(107-107)bufferDesc(116-116)src/plugin/default-pipeline/src/resource/buffer/AmbientLightBuffer.hpp (6)
context(86-94)context(86-86)context(96-101)context(96-97)_buffer(81-81)bufferDesc(88-88)src/plugin/default-pipeline/src/resource/buffer/TransformGPUBuffer.hpp (4)
context(71-81)context(71-71)_buffer(68-68)bufferDesc(73-73)src/plugin/default-pipeline/src/resource/buffer/PointGPUBuffer.hpp (2)
_buffer(78-78)bufferDesc(41-41)
🔇 Additional comments (22)
src/plugin/object/src/resource/ResourceManager.hpp (1)
184-212: LGTM!The new
GetDefault()accessors are well-implemented and consistent with the existing class patterns:
- Follows the same error-handling approach as
GetOrDefault().- Proper use of
[[nodiscard]]attribute matching other getters.- Documentation is complete and accurate.
- Both const and non-const overloads are provided appropriately.
src/plugin/default-pipeline/src/component/GPUMaterial.hpp (1)
5-14: LGTM!Clean namespace migration from
Graphic::ComponenttoDefaultPipeline::Component. The struct definition remains unchanged, and the closing comment correctly reflects the new namespace. The removal of unused includes (Camera.hpp, Transform.hpp, glm/glm.hpp as noted in the summary) is appropriate.src/plugin/default-pipeline/src/component/GPUTransform.hpp (1)
5-11: LGTM!Consistent namespace migration from
Graphic::ComponenttoDefaultPipeline::Component, matching the pattern established in other GPU component headers. The struct definition and its members remain unchanged.src/plugin/default-pipeline/src/system/GPUComponentManagement/OnTransformCreation.hpp (1)
6-10: LGTM!Clean namespace migration from
Graphic::SystemtoDefaultPipeline::System. The function signature remains unchanged, and the closing comment correctly reflects the new namespace. This aligns with the broader refactoring to separate the default render pipeline into its own plugin.src/plugin/default-pipeline/src/system/GPUComponentManagement/OnMeshDestruction.hpp (1)
6-10: Namespace migration is complete and consistent.The namespace change from
Graphic::SystemtoDefaultPipeline::Systemhas been properly applied across both the header declaration and the implementation file. No remaining references to the old namespace exist.src/plugin/graphic/tests/BufferTest.cpp (3)
1-5: LGTM!The includes are appropriate for the test requirements.
7-48: LGTM!The
GPUBufferTestclass correctly implements theAGPUBufferinterface, following the same patterns used in other buffer implementations (e.g.,CameraGPUBuffer,MaterialGPUBuffer). The lifecycle management with_isCreatedflag, proper guard inUpdate, andrelease()inDestroyare all correctly implemented.
68-81: LGTM!The test setup correctly configures the graphic plugin for headless testing by setting
WindowSystem::None.src/plugin/default-pipeline/src/system/GPUComponentManagement/OnMaterialDestruction.hpp (1)
6-10: Namespace change aligns with the plugin restructuring and migration is complete.The namespace migration from
Graphic::SystemtoDefaultPipeline::Systemis consistent with the file's location within thedefault-pipelineplugin and the PR's objective to separate the default render pipeline from the core graphic plugin. No lingering references to the old namespace remain, and all call sites have been properly updated to use the new namespace via the plugin's function pointer registration.src/plugin/default-pipeline/src/resource/buffer/PointLightsBuffer.hpp (4)
13-14: LGTM!The namespace move to
DefaultPipeline::Resourceand explicit qualification of the base classGraphic::Resource::AGPUBufferalign well with the PR objective of separating the default pipeline implementation from the core graphic plugin skeleton.
42-52: LGTM!The
Createmethod correctly delegates buffer creation to the new_CreateBufferhelper and follows the established pattern seen in other buffer implementations (e.g.,IndexGPUBuffer). The fully qualifiedGraphic::Resource::Contextis appropriate given the namespace move.
65-72: LGTM!The
Updatemethod correctly uses fully qualified types (Graphic::Exception::UpdateBufferError,Graphic::Resource::Context) after the namespace move. The existing logic for iterating point lights and updating the GPU buffer remains intact and correct.
114-122: LGTM!The
_CreateBufferhelper follows the established pattern used by other buffer implementations in the codebase (MaterialGPUBuffer,TransformGPUBuffer,CameraGPUBuffer,AmbientLightBuffer). This extraction improves code organization and maintainability.src/plugin/default-pipeline/xmake.lua (2)
24-45: LGTM!The target configuration correctly sets up a C++20 static library with appropriate dependencies, packages, and include paths. The header file patterns comprehensively cover the module structure.
47-74: The code is correct as-is; explicittarget_end()is not required.In xmake,
target_end()is optional. According to xmake documentation, opening a new target domain (such as callingtarget(name)with a different name) implicitly closes the previous target scope. Since each loop iteration callstarget(name)with a different test file name, the previous iteration's target is automatically closed when the next iteration begins.However, for improved code clarity and to follow modern xmake best practices, consider using the function form instead:
for _, file in ipairs(os.files("tests/**.cpp")) do local name = path.basename(file) if name == "main" then goto continue end target(name, function() set_group(TEST_GROUP_NAME) set_kind("binary") if is_plat("linux") then add_cxxflags("--coverage", "-fprofile-arcs", "-ftest-coverage", {force = true}) add_ldflags("--coverage") end set_languages("cxx20") add_packages("gtest", required_packages) add_links("gtest") add_deps(plugin_name) add_deps(target_dependencies) add_files(file) add_files("tests/main.cpp") add_tests("default") if is_mode("debug") then add_defines("DEBUG") end end) ::continue:: endThis function form automatically confines configuration to the target scope without requiring explicit
target_end()calls.Likely an incorrect or invalid review comment.
src/plugin/default-pipeline/src/resource/buffer/CameraGPUBuffer.hpp (1)
9-73: LGTM!The namespace migration to
DefaultPipeline::Resourceis consistent with the PR's refactoring goals. The fully-qualified references toGraphic::Resource::Context,Graphic::Resource::DeviceContext, andGraphic::Exception::UpdateBufferErrorcorrectly maintain the dependency on the graphic plugin's core types while relocating the buffer implementation.src/plugin/default-pipeline/src/resource/buffer/IndexGPUBuffer.hpp (1)
10-11: LGTM!The namespace relocation and inheritance from
Graphic::Resource::AGPUBufferare consistent with the other buffer implementations in the DefaultPipeline module.src/plugin/default-pipeline/tests/PluginDefaultPipelineTest.cpp (1)
1-6: LGTM!The includes and test setup are appropriate for a plugin integration test. Disabling the window system via
SetWindowSystem(Graphic::Resource::WindowSystem::None)is a good practice for headless CI environments.src/plugin/default-pipeline/src/system/initialization/CreatePointLights.hpp (1)
5-9: LGTM!The namespace relocation from
Graphic::SystemtoDefaultPipeline::Systemis clean and consistent with the broader refactoring effort.src/plugin/default-pipeline/src/system/initialization/CreateDefaultRenderPipeline.hpp (1)
5-7: LGTM!Namespace relocation is consistent with the default pipeline extraction pattern.
src/plugin/default-pipeline/src/system/GPUComponentManagement/OnCameraCreation.hpp (1)
6-10: LGTM!Namespace relocation is consistent. The includes properly cover both parameter types.
src/plugin/default-pipeline/src/system/GPUComponentManagement/OnCameraDestruction.hpp (1)
6-10: LGTM!Namespace relocation is consistent with the rest of the refactoring. All references to
OnCameraDestructionhave been properly updated—the function is declared and implemented inDefaultPipeline::System, and all call sites inPluginDefaultPipeline.cppcorrectly resolve to the new namespace.
There was a problem hiding this comment.
Pull request overview
This PR refactors the graphic plugin by splitting it into two parts: a core graphics plugin (skeleton) that provides the base infrastructure for graphics pipelines, and a new DefaultPipeline plugin that implements the actual 3D rendering functionality. This separation enables better modularity and allows alternative rendering pipeline implementations to be created more easily.
Changes:
- Created a new DefaultPipeline plugin containing rendering components, systems, and resources previously in the Graphic plugin
- Added safe
GetDefault()accessors to ResourceManager for accessing default resources - Simplified example build configurations to use the unified EngineSquared dependency
Reviewed changes
Copilot reviewed 77 out of 77 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| xmake.lua | Added DefaultPipeline plugin dependency to main build |
| src/plugin/object/src/resource/ResourceManager.hpp | Added GetDefault() methods with exception handling |
| src/plugin/graphic/xmake.lua | Added tinyobjloader package dependency |
| src/plugin/graphic/tests/*.cpp | Updated tests to use new namespaces and simplified buffer testing |
| src/plugin/graphic/src/utils/EndRenderTexture.hpp | New file defining END_RENDER_TEXTURE_ID constant |
| src/plugin/graphic/src/system/preparation/PrepareEndRenderTexture.cpp | Updated to use Graphic::Utils::END_RENDER_TEXTURE_ID |
| src/plugin/graphic/src/system/commandCreation/ExecuteRenderPass.cpp | Refactored to use default render pass from container |
| src/plugin/graphic/src/plugin/PluginGraphic.cpp | Removed GPU component setup and rendering systems (moved to DefaultPipeline) |
| src/plugin/graphic/src/Graphic.hpp | Removed includes for moved components and systems |
| src/plugin/default-pipeline/** | New plugin with all components, resources, systems, and utilities for 3D rendering |
| examples/** | Updated to use DefaultPipeline::Plugin and simplified build dependencies |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Miou-zora
deleted the
remove-default-render-pipeline-from-graphic-plugin
branch
Not related to any issues.
As graphic plugin is getting bigger over time, I wanted to split it into 2 parts: one for the skeleton (graphic) that can be used to implement real implementation of graphic pipeline, and one for the implementation (default pipeline) that creates RenderPass, components and all stuff related to real 3d related stuff.
Summary by CodeRabbit
New Features
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.