Remove bgfx/bx/bimg and the rendering/scene stack; add Matrix4 + Quaternion to Core#51
Remove bgfx/bx/bimg and the rendering/scene stack; add Matrix4 + Quaternion to Core#51jayrulez wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (3)
📒 Files selected for processing (98)
💤 Files with no reviewable changes (81)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (13)
📝 WalkthroughWalkthroughThe change adds Matrix4, Quaternion, transform interpolation, and approximate-comparison math APIs. It removes rendering, scene, shader, and bgfx-related build surfaces, narrows runtime dependencies, and rewrites the render sample as a shell event-loop application. ChangesEngine math foundation
Runtime and sample restructuring
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
Samples/cpp/Render/RenderSample.cpp (1)
12-16: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winWrap the
createShellresult instd::unique_ptrto avoid a leak.
createShellreturns a raw owningIShell*(new SDL3Shell(settings)) that is never freed.IShellhas a virtual destructor, sounique_ptrwill correctly clean up. Ensure#include <memory>is present.♻️ Proposed fix
- auto shell = createShell(WindowSettings{ + auto shell = std::unique_ptr<IShell>(createShell(WindowSettings{ .title = u8"Draconic Engine Sample", .width = 1280, .height = 720, - }); + }));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Samples/cpp/Render/RenderSample.cpp` around lines 12 - 16, Update the shell initialization around createShell to store its owning raw IShell* result in a std::unique_ptr, ensuring the virtual destructor performs cleanup at scope exit. Add the <memory> include if it is not already present, and preserve the existing WindowSettings arguments.Engine/cpp/Runtime/Core/Math/Math.test.cpp (1)
1500-1505: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExercise projection and view behavior, not only coefficients.
Add near/far homogeneous-depth checks and coverage for
orthographicRH()andlookAtRH(). The current assertions would miss common sign or transposition regressions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Engine/cpp/Runtime/Core/Math/Math.test.cpp` around lines 1500 - 1505, Add behavioral checks to the “perspective has the expected projective structure” test by projecting representative near- and far-plane points and validating their homogeneous depth results. Add coverage for orthographicRH() and lookAtRH(), including transformed points and expected view/depth behavior, so sign and matrix-transposition regressions are detected rather than only checking raw coefficients.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Engine/cpp/Runtime/Core/Math/Matrix4.cppm`:
- Around line 251-255: Update Matrix4 inversion to expose singular failures
through tryInverse or an optional result instead of returning
Matrix4::identity(), and replace the absolute determinant check with a
scale-aware singularity criterion. In
Engine/cpp/Runtime/Core/Math/Matrix4.cppm:251-255, modify the inverse
implementation; in Engine/cpp/Runtime/Core/Math/Math.test.cpp:1531-1532, assert
explicit failure for singular matrices and add regression coverage proving
small-but-invertible matrices still invert successfully.
In `@Engine/cpp/Runtime/Core/Math/Quaternion.cppm`:
- Around line 77-80: Update Quaternion::nearlyEqual to consider both the
component-wise comparison of a and b and the comparison of a against the negated
components of b, so antipodal quaternions are treated as equal rotations.
Preserve the epsilon parameter and noexcept contract, and ensure the result
remains true for both identical and sign-inverted quaternion pairs.
- Around line 28-33: Update Quaternion::fromAxisAngle to detect a zero-length
axis before normalization and return the identity quaternion for that input,
preventing rotateVector() from scaling vectors. Preserve the existing
normalized-axis conversion for nonzero axes.
---
Nitpick comments:
In `@Engine/cpp/Runtime/Core/Math/Math.test.cpp`:
- Around line 1500-1505: Add behavioral checks to the “perspective has the
expected projective structure” test by projecting representative near- and
far-plane points and validating their homogeneous depth results. Add coverage
for orthographicRH() and lookAtRH(), including transformed points and expected
view/depth behavior, so sign and matrix-transposition regressions are detected
rather than only checking raw coefficients.
In `@Samples/cpp/Render/RenderSample.cpp`:
- Around line 12-16: Update the shell initialization around createShell to store
its owning raw IShell* result in a std::unique_ptr, ensuring the virtual
destructor performs cleanup at scope exit. Add the <memory> include if it is not
already present, and preserve the existing WindowSettings arguments.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 45bd4269-17eb-4bbc-a280-89ebbcde98e9
⛔ Files ignored due to path filters (3)
Engine/cpp/ThirdParty/cmake/bgfx/generated/bounds.cpp.inis excluded by!**/generated/**Engine/cpp/ThirdParty/cmake/bgfx/generated/shader.cpp.inis excluded by!**/generated/**Engine/cpp/ThirdParty/cmake/bgfx/generated/vertexlayout.cpp.inis excluded by!**/generated/**
📒 Files selected for processing (97)
.gitmodulesCMakeLists.txtEngine/cpp/CMakeLists.txtEngine/cpp/Runtime/CMakeLists.txtEngine/cpp/Runtime/Core/CMakeLists.txtEngine/cpp/Runtime/Core/Math/Functions.cppmEngine/cpp/Runtime/Core/Math/Math.cppmEngine/cpp/Runtime/Core/Math/Math.test.cppEngine/cpp/Runtime/Core/Math/Matrix4.cppmEngine/cpp/Runtime/Core/Math/Quaternion.cppmEngine/cpp/Runtime/Core/Math/Transform.cppEngine/cpp/Runtime/Core/Math/Transform.cppmEngine/cpp/Runtime/Core/Math/TypesCommon.cppmEngine/cpp/Runtime/Core/Math/Vector2.cppmEngine/cpp/Runtime/Core/Math/Vector3.cppmEngine/cpp/Runtime/Core/Math/Vector4.cppmEngine/cpp/Runtime/Draconic.cppmEngine/cpp/Runtime/Rendering/CMakeLists.txtEngine/cpp/Runtime/Rendering/Material/Material.cppmEngine/cpp/Runtime/Rendering/Mesh/Mesh.cppEngine/cpp/Runtime/Rendering/Mesh/Mesh.cppmEngine/cpp/Runtime/Rendering/QuadRenderer/QuadRenderer.cppEngine/cpp/Runtime/Rendering/QuadRenderer/QuadRenderer.cppmEngine/cpp/Runtime/Rendering/RHI/Buffers.cppEngine/cpp/Runtime/Rendering/RHI/Commands.cppEngine/cpp/Runtime/Rendering/RHI/Core.cppEngine/cpp/Runtime/Rendering/RHI/Pipelines.cppEngine/cpp/Runtime/Rendering/RHI/RHI.cppmEngine/cpp/Runtime/Rendering/RHI/Texture.cppEngine/cpp/Runtime/Rendering/RHI/UniformRegistry.cppmEngine/cpp/Runtime/Rendering/RHI/Vertex.cppmEngine/cpp/Runtime/Rendering/RHI/macros.hEngine/cpp/Runtime/Rendering/RenderGraph/RenderGraph.cppEngine/cpp/Runtime/Rendering/RenderGraph/RenderGraph.cppmEngine/cpp/Runtime/Rendering/Renderer/Renderer.cppEngine/cpp/Runtime/Rendering/Renderer/Renderer.cppmEngine/cpp/Runtime/Rendering/Rendering.cppmEngine/cpp/Runtime/Rendering/Shaders/fs.scEngine/cpp/Runtime/Rendering/Shaders/fs_quad.scEngine/cpp/Runtime/Rendering/Shaders/varying.def.scEngine/cpp/Runtime/Rendering/Shaders/varying_quad.def.scEngine/cpp/Runtime/Rendering/Shaders/vs.scEngine/cpp/Runtime/Rendering/Shaders/vs_quad.scEngine/cpp/Runtime/Scene/CMakeLists.txtEngine/cpp/Runtime/Scene/Camera/CameraController.cppEngine/cpp/Runtime/Scene/Camera/CameraController.cppmEngine/cpp/Runtime/Scene/Renderable/Renderable.cppmEngine/cpp/Runtime/Scene/Scene.cppmEngine/cpp/Runtime/Scene/TransformComponent/TransformComponent.cppEngine/cpp/Runtime/Scene/TransformComponent/TransformComponent.cppmEngine/cpp/ThirdParty/CMakeLists.txtEngine/cpp/ThirdParty/bgfxEngine/cpp/ThirdParty/bimgEngine/cpp/ThirdParty/bxEngine/cpp/ThirdParty/cmake/Config.cmake.inEngine/cpp/ThirdParty/cmake/bgfx/3rdparty/dear-imgui.cmakeEngine/cpp/ThirdParty/cmake/bgfx/3rdparty/fcpp.cmakeEngine/cpp/ThirdParty/cmake/bgfx/3rdparty/glsl-optimizer.cmakeEngine/cpp/ThirdParty/cmake/bgfx/3rdparty/glslang.cmakeEngine/cpp/ThirdParty/cmake/bgfx/3rdparty/meshoptimizer.cmakeEngine/cpp/ThirdParty/cmake/bgfx/3rdparty/spirv-cross.cmakeEngine/cpp/ThirdParty/cmake/bgfx/3rdparty/spirv-opt.cmakeEngine/cpp/ThirdParty/cmake/bgfx/3rdparty/tint.cmakeEngine/cpp/ThirdParty/cmake/bgfx/3rdparty/webgpu.cmakeEngine/cpp/ThirdParty/cmake/bgfx/CMakeLists.txtEngine/cpp/ThirdParty/cmake/bgfx/bgfx.cmakeEngine/cpp/ThirdParty/cmake/bgfx/examples.cmakeEngine/cpp/ThirdParty/cmake/bgfx/geometryc.cmakeEngine/cpp/ThirdParty/cmake/bgfx/geometryv.cmakeEngine/cpp/ThirdParty/cmake/bgfx/shaderc.cmakeEngine/cpp/ThirdParty/cmake/bgfx/shared.cmakeEngine/cpp/ThirdParty/cmake/bgfx/texturev.cmakeEngine/cpp/ThirdParty/cmake/bgfx/util/ConfigureDebugging.cmakeEngine/cpp/ThirdParty/cmake/bgfxToolUtils.cmakeEngine/cpp/ThirdParty/cmake/bimg/3rdparty/astc_encoder.cmakeEngine/cpp/ThirdParty/cmake/bimg/3rdparty/edtaa3.cmakeEngine/cpp/ThirdParty/cmake/bimg/3rdparty/etc1.cmakeEngine/cpp/ThirdParty/cmake/bimg/3rdparty/etc2.cmakeEngine/cpp/ThirdParty/cmake/bimg/3rdparty/iqa.cmakeEngine/cpp/ThirdParty/cmake/bimg/3rdparty/libsquish.cmakeEngine/cpp/ThirdParty/cmake/bimg/3rdparty/loadpng.cmakeEngine/cpp/ThirdParty/cmake/bimg/3rdparty/miniz.cmakeEngine/cpp/ThirdParty/cmake/bimg/3rdparty/nvtt.cmakeEngine/cpp/ThirdParty/cmake/bimg/3rdparty/pvrtc.cmakeEngine/cpp/ThirdParty/cmake/bimg/3rdparty/tinyexr.cmakeEngine/cpp/ThirdParty/cmake/bimg/CMakeLists.txtEngine/cpp/ThirdParty/cmake/bimg/bimg.cmakeEngine/cpp/ThirdParty/cmake/bimg/bimg_decode.cmakeEngine/cpp/ThirdParty/cmake/bimg/bimg_encode.cmakeEngine/cpp/ThirdParty/cmake/bimg/texturec.cmakeEngine/cpp/ThirdParty/cmake/bx/CMakeLists.txtEngine/cpp/ThirdParty/cmake/bx/bin2c.cmakeEngine/cpp/ThirdParty/cmake/bx/bx.cmakeEngine/cpp/ThirdParty/cmake/version.cmakeSamples/cpp/Render/CMakeLists.txtSamples/cpp/Render/RenderSample.cppcmake/Shaders.cmake
💤 Files with no reviewable changes (81)
- Engine/cpp/Runtime/Rendering/RHI/RHI.cppm
- Engine/cpp/ThirdParty/cmake/bimg/3rdparty/tinyexr.cmake
- Engine/cpp/ThirdParty/cmake/bgfx/3rdparty/glslang.cmake
- Engine/cpp/ThirdParty/cmake/bgfx/util/ConfigureDebugging.cmake
- Engine/cpp/ThirdParty/cmake/bgfx/3rdparty/spirv-cross.cmake
- Engine/cpp/ThirdParty/cmake/bimg/3rdparty/pvrtc.cmake
- Engine/cpp/Runtime/Rendering/Material/Material.cppm
- CMakeLists.txt
- Engine/cpp/Runtime/Scene/TransformComponent/TransformComponent.cpp
- Engine/cpp/Runtime/Rendering/Shaders/fs_quad.sc
- Engine/cpp/ThirdParty/bx
- Engine/cpp/Runtime/Core/Math/Transform.cpp
- Engine/cpp/ThirdParty/cmake/bimg/3rdparty/iqa.cmake
- Engine/cpp/ThirdParty/cmake/Config.cmake.in
- Engine/cpp/Runtime/Rendering/QuadRenderer/QuadRenderer.cpp
- Engine/cpp/Runtime/CMakeLists.txt
- Engine/cpp/ThirdParty/cmake/bimg/texturec.cmake
- Engine/cpp/ThirdParty/cmake/bgfx/shared.cmake
- Engine/cpp/ThirdParty/cmake/bgfx/3rdparty/dear-imgui.cmake
- Engine/cpp/ThirdParty/cmake/bimg/3rdparty/etc1.cmake
- Engine/cpp/Runtime/Rendering/Shaders/varying_quad.def.sc
- Engine/cpp/Runtime/Scene/Scene.cppm
- Engine/cpp/ThirdParty/cmake/bimg/bimg.cmake
- Engine/cpp/ThirdParty/cmake/bgfx/CMakeLists.txt
- Engine/cpp/ThirdParty/cmake/bgfx/3rdparty/meshoptimizer.cmake
- Engine/cpp/ThirdParty/cmake/bimg/3rdparty/etc2.cmake
- Engine/cpp/ThirdParty/cmake/bgfx/texturev.cmake
- Engine/cpp/Runtime/Rendering/RHI/macros.h
- Engine/cpp/Runtime/Scene/TransformComponent/TransformComponent.cppm
- Engine/cpp/Runtime/Rendering/Shaders/vs_quad.sc
- Engine/cpp/Runtime/Rendering/RHI/Pipelines.cpp
- Engine/cpp/ThirdParty/cmake/bgfx/3rdparty/spirv-opt.cmake
- Engine/cpp/ThirdParty/cmake/bimg/bimg_decode.cmake
- Engine/cpp/ThirdParty/cmake/bimg/3rdparty/miniz.cmake
- Engine/cpp/ThirdParty/cmake/bimg/3rdparty/nvtt.cmake
- Engine/cpp/ThirdParty/cmake/bimg/CMakeLists.txt
- Engine/cpp/ThirdParty/cmake/bimg/bimg_encode.cmake
- Engine/cpp/Runtime/Rendering/Mesh/Mesh.cppm
- Engine/cpp/Runtime/Rendering/Shaders/fs.sc
- Engine/cpp/Runtime/Rendering/RHI/Texture.cpp
- Engine/cpp/ThirdParty/cmake/bgfx/3rdparty/glsl-optimizer.cmake
- Engine/cpp/ThirdParty/cmake/bgfx/geometryv.cmake
- Engine/cpp/ThirdParty/cmake/bgfxToolUtils.cmake
- Engine/cpp/ThirdParty/bgfx
- Engine/cpp/Runtime/Scene/Renderable/Renderable.cppm
- Engine/cpp/ThirdParty/cmake/bimg/3rdparty/edtaa3.cmake
- Engine/cpp/ThirdParty/cmake/bx/CMakeLists.txt
- Engine/cpp/Runtime/Rendering/RHI/Buffers.cpp
- Engine/cpp/ThirdParty/cmake/bgfx/shaderc.cmake
- Engine/cpp/Runtime/Rendering/RenderGraph/RenderGraph.cppm
- Engine/cpp/Runtime/Scene/Camera/CameraController.cppm
- Engine/cpp/ThirdParty/cmake/bgfx/3rdparty/webgpu.cmake
- Engine/cpp/Runtime/Rendering/Shaders/vs.sc
- Engine/cpp/ThirdParty/cmake/bgfx/geometryc.cmake
- Engine/cpp/Runtime/Rendering/Rendering.cppm
- Engine/cpp/ThirdParty/cmake/bgfx/bgfx.cmake
- Engine/cpp/Runtime/Rendering/Renderer/Renderer.cpp
- Engine/cpp/ThirdParty/cmake/bimg/3rdparty/libsquish.cmake
- Engine/cpp/Runtime/Rendering/Shaders/varying.def.sc
- Engine/cpp/ThirdParty/cmake/bx/bin2c.cmake
- Engine/cpp/Runtime/Rendering/Mesh/Mesh.cpp
- Engine/cpp/ThirdParty/cmake/bx/bx.cmake
- Engine/cpp/Runtime/Draconic.cppm
- Engine/cpp/Runtime/Rendering/RHI/UniformRegistry.cppm
- Engine/cpp/ThirdParty/cmake/bimg/3rdparty/astc_encoder.cmake
- Engine/cpp/Runtime/Rendering/QuadRenderer/QuadRenderer.cppm
- Engine/cpp/ThirdParty/cmake/bgfx/examples.cmake
- Engine/cpp/Runtime/Rendering/RHI/Vertex.cppm
- Engine/cpp/Runtime/Rendering/Renderer/Renderer.cppm
- Engine/cpp/ThirdParty/cmake/bimg/3rdparty/loadpng.cmake
- Engine/cpp/ThirdParty/cmake/bgfx/3rdparty/fcpp.cmake
- Engine/cpp/Runtime/Scene/Camera/CameraController.cpp
- Engine/cpp/ThirdParty/cmake/version.cmake
- Engine/cpp/Runtime/Rendering/RenderGraph/RenderGraph.cpp
- Engine/cpp/ThirdParty/cmake/bgfx/3rdparty/tint.cmake
- Engine/cpp/Runtime/Scene/CMakeLists.txt
- Engine/cpp/Runtime/Rendering/RHI/Commands.cpp
- Engine/cpp/Runtime/Rendering/CMakeLists.txt
- cmake/Shaders.cmake
- Engine/cpp/ThirdParty/bimg
- Engine/cpp/Runtime/Rendering/RHI/Core.cpp
…ernion to Core Drop the bgfx rendering stack entirely: delete the Rendering and Scene modules, the bx/bimg/bgfx submodules and their ThirdParty build tooling, the shader compilation (cmake/Shaders.cmake), and rewire Runtime/Engine/Draconic so the engine builds on Core + Shell + SDL3 only. RenderSample becomes a shell-only placeholder that opens a window and pumps events until closed. Core/Math kept its one bx dependency (Transform's TRS matrix build), so replace it with native math: add core.math.matrix4 (4x4 row-major, row-vector convention: identity/translation/scale/rotation/perspective/ortho/lookAt, multiply, transpose, transformPoint/Direction/Point2D, determinant, inverse) and core.math.quaternion (fromAxisAngle, Hamilton product, conjugate/dot/inverse/normalize/rotateVector/slerp, rotationMatrix). Rewrite Transform to position/rotation(Quaternion)/scale with an inline toMatrix() and a lerp helper, removing bx from Core entirely. Added missing math helpers used by these types (sin/cos/tan/acos/sqrt, scalar and Vector2/3/4 nearlyEqual, Vector3::zero/one) and added Matrix4/Quaternion/Transform unit tests.
OldDev78
left a comment
There was a problem hiding this comment.
A few small things I'd rather see changed.
| # PIC: these are linked into the shared Runtime library (via Core), so their objects | ||
| # must be position-independent - a non-PIC static (e.g. Quaternion::identity) otherwise | ||
| # fails to relocate when making the shared object. |
There was a problem hiding this comment.
If this is needed for a specific call in Math, then flag only Math as PIC, and propagate it through Core.
The shared library is Runtime.
Do not flag everything as PIC, it costs optimizations at a level where most code is linked statically.
| } | ||
|
|
||
| // Component-wise approximate equality against an epsilon. | ||
| [[nodiscard]] inline bool nearlyEqual(const Vector2& a, const Vector2& b, f32 epsilon = CMP_EPSILON) noexcept { |
There was a problem hiding this comment.
I don´t think we need two variations of the same check, component-wise near equal check should suffice, so it could effectively replace approx_eq - and make this constexpr. I would shorten the name though, near or nearEq.
| static const Vector3 zero; | ||
| static const Vector3 one; |
There was a problem hiding this comment.
This is wrong, because they are declared inline constexpr in Vector3.cppm. Since it's exported as a module fragment, don't bother forward declaring them here.
| import shell; | ||
| import shell.desktop; |
There was a problem hiding this comment.
Don´t repeat yourself, just import shell. shell should also be exporting shell.desktop.
I assume this is only being done temporarily? In fact, just importing draconic should still be fine.
Keep in mind, modules are not headers, "importing everything" does not mean the same as including everything.
| .title = u8"Draconic Engine Rendering Sample", | ||
| .title = u8"Draconic Engine Sample", |
There was a problem hiding this comment.
This is going to be restored for rendering, right?
| [[nodiscard]] inline bool nearlyEqual(f32 a, f32 b, f32 epsilon = CMP_EPSILON) noexcept { | ||
| return abs(a - b) <= epsilon; | ||
| } | ||
| [[nodiscard]] inline bool nearlyZero(f32 v, f32 epsilon = CMP_EPSILON) noexcept { | ||
| return abs(v) <= epsilon; | ||
| } |
There was a problem hiding this comment.
Should be constexpr (along with the Vector* ones)
| [[nodiscard]] constexpr f32 operator()(usize row, usize col) const noexcept | ||
| { | ||
| return m[row][col]; | ||
| } | ||
| [[nodiscard]] constexpr f32& operator()(usize row, usize col) noexcept | ||
| { | ||
| return m[row][col]; | ||
| } |
There was a problem hiding this comment.
Replace by multidimensional [] operator.
There was a problem hiding this comment.
Might turn this into a template, but for now it will do.
| // Identity transform (position 0, rotation identity, scale 1) - the | ||
| // default-constructed value. | ||
| inline constexpr Transform identityTransform{}; |
There was a problem hiding this comment.
We don't need a special constant which is the same as the default constructed value.
Drop the bgfx rendering stack entirely: delete the Rendering and Scene modules, the bx/bimg/bgfx submodules and their ThirdParty build tooling, the shader compilation (cmake/Shaders.cmake), and rewire Runtime/Engine/Draconic so the engine builds on Core + Shell + SDL3 only.
RenderSample becomes a shell-only placeholder that opens a window and pumps events until closed.
Base Transform on our new Matrix type.
Summary by CodeRabbit
New Features
matrix4,quaternion, and value-typeTransformwith interpolation (lerp) andtoMatrix()support.nearlyEqualhelpers (scalar + vector overloads).Changes
Tests