Include clang-format document#29
Open
JoltedJon wants to merge 2 commits into
Open
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR introduces a repository-wide ChangesFormatting & module conversion
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.clang-format (1)
1-208:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix duplicate
.clang-formatoption and validate the file
- Remove the duplicate key
AllowShortCaseExpressionOnASingleLine: true(it appears twice).- Reword informal/unprofessional comments in the config (e.g., “Microslop BS”, “Pain in the ass”).
- Run
clang-format --style=file --dry-run --Werror <some .cpp/.h file>with the sameclang-formatversion used in CI to catch any remaining option/value issues.🤖 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 @.clang-format around lines 1 - 208, Remove the duplicated key AllowShortCaseExpressionOnASingleLine (leave a single occurrence) and replace the informal comments "Microslop BS \r\n" and "Pain in the ass" with professional wording (e.g., "CRLF line endings not allowed" and "Avoid unbraced statements; prefer braces for clarity") in the .clang-format content; then validate the file by running clang-format with the CI version: clang-format --style=file --dry-run --Werror <example source file> and fix any remaining invalid options or values reported.
🤖 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 @.clang-format:
- Around line 37-38: The .clang-format contains a duplicated option key; remove
the redundant line so that AllowShortCaseExpressionOnASingleLine appears only
once—locate the consecutive duplicate entries for
AllowShortCaseExpressionOnASingleLine and delete one of them to keep a single
setting.
- Line 63: The .clang-format entry uses the incorrect key name
BraceWrappingAfterControlStatementStyle; update the configuration to use the
proper clang-format YAML key by moving this setting under BraceWrapping with the
AfterControlStatement field (e.g. set BraceWrapping: { AfterControlStatement:
BWACS_Never } or equivalent YAML mapping) so the value BWACS_Never is applied;
look for the existing BraceWrappingAfterControlStatementStyle occurrence and
replace it with the BraceWrapping -> AfterControlStatement mapping.
- Line 151: Replace the unprofessional comment string "# No Microslop BS" in the
.clang-format file with a neutral, professional phrase (or remove it entirely);
locate the comment by searching for the exact text "# No Microslop BS" and
update it to something like a concise, descriptive note about the intent (e.g.,
indicating that project-specific or proprietary vendor settings are excluded) to
keep the config file professional.
- Line 109: The .clang-format sets Cpp11BracedListStyle to an invalid value
"BLS_FunctionCall"; update the Cpp11BracedListStyle entry to a boolean (set
Cpp11BracedListStyle: true or Cpp11BracedListStyle: false) and, if you intended
specific braced-list spacing/line-breaking behavior, add the appropriate
braced-list options such as BreakAfterOpenBracketBracedList or
AllowShortBracedLists to express the desired formatting; locate the
Cpp11BracedListStyle key in the file and replace the current token with the
correct boolean and any supplemental braced-list options.
---
Outside diff comments:
In @.clang-format:
- Around line 1-208: Remove the duplicated key
AllowShortCaseExpressionOnASingleLine (leave a single occurrence) and replace
the informal comments "Microslop BS \r\n" and "Pain in the ass" with
professional wording (e.g., "CRLF line endings not allowed" and "Avoid unbraced
statements; prefer braces for clarity") in the .clang-format content; then
validate the file by running clang-format with the CI version: clang-format
--style=file --dry-run --Werror <example source file> and fix any remaining
invalid options or values reported.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
|
|
||
| KeepFormFeed: false | ||
|
|
||
| # No Microslop BS \r\n |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Revise unprofessional comment.
The comment contains informal language and a derogatory reference that is inappropriate for a project configuration file. Consider rewording to maintain professionalism.
📝 Suggested revision
-# No Microslop BS \r\n
+# Use Unix-style line endings (LF) instead of Windows-style (CRLF)
LineEnding: LF📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # No Microslop BS \r\n | |
| # Use Unix-style line endings (LF) instead of Windows-style (CRLF) |
🤖 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 @.clang-format at line 151, Replace the unprofessional comment string "# No
Microslop BS" in the .clang-format file with a neutral, professional phrase (or
remove it entirely); locate the comment by searching for the exact text "# No
Microslop BS" and update it to something like a concise, descriptive note about
the intent (e.g., indicating that project-specific or proprietary vendor
settings are excluded) to keep the config file professional.
OldDev78
reviewed
May 30, 2026
Contributor
OldDev78
left a comment
OldDev78
left a comment
There was a problem hiding this comment.
I'd recommend applying the changes to our source in this PR once you're ready to merge. We'll know all of the whitespace changes spam is for formatting.
| Standard: Cpp23 | ||
|
|
||
| # Pointer and reference alignment style. Possible values: Left, Right, Middle. | ||
| PointerAlignment: Right |
Contributor
There was a problem hiding this comment.
Vote for pointer to the left (on the side of the type)
| AfterNamespace: false | ||
| AfterStruct: false | ||
| AfterExtern: false | ||
| BeforeCatch: true # catch on separate line, not like we'll use catch |
Contributor
There was a problem hiding this comment.
Just for consistency. I'd also rather have K&R
| AfterStruct: false | ||
| AfterExtern: false | ||
| BeforeCatch: true # catch on separate line, not like we'll use catch | ||
| BeforeElse: true |
Contributor
There was a problem hiding this comment.
Not having a break for if but having for else will look strange for me.
Contributor
Author
There was a problem hiding this comment.
This looks like
if(false) {
// Stuff
}
else if (true) {
}
if you set BeforeElse to false it would like
if(false) {
// Stuff
} else if (true) {
}
In my opinion that does make it a little harder to read especially if the last line before the else has a lot of values.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
engine/native/core/io/image_loader.cpp (1)
8-9:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winBuild failure: stb_image.h is incompatible with C++20 module scanning.
The pipeline logs show a critical preprocessor error when clang-scan-deps attempts to scan stb_image.h. Legacy single-header libraries with complex preprocessor logic cannot be included in module purview (global module fragment or otherwise) because the module dependency scanner must parse them.
Move the stb implementation to a separate non-module translation unit to isolate it from the module system.
🔧 Recommended fix: separate stb into a non-module TU
Create
engine/native/core/io/image_loader_stb.cpp(non-module file):`#define` STB_IMAGE_IMPLEMENTATION `#include` <stb_image.h> `#include` <filesystem> namespace draco::core::io::image_loader::detail { unsigned char* stb_load_wrapper(const char* path, int* width, int* height, int* channels) { return stbi_load(path, width, height, channels, STBI_rgb_alpha); } void stb_free_wrapper(unsigned char* data) { stbi_image_free(data); } } // namespace draco::core::io::image_loader::detailThen update this file to remove the stb include from the global fragment and forward-declare the wrapper:
module; `#include` <vector> `#include` <filesystem> `#include` <limits> `#include` <print> - -#define STB_IMAGE_IMPLEMENTATION -#include <stb_image.h> module core.io.image_loader; import core.stdtypes; +namespace draco::core::io::image_loader::detail { +extern unsigned char* stb_load_wrapper(const char* path, int* width, int* height, int* channels); +extern void stb_free_wrapper(unsigned char* data); +} + // TODO: I'm too lazy to write code so we need somethin' better namespace draco::core::io::image_loader {And replace calls to
stbi_loadandstbi_image_freewith the wrapper functions:- unsigned char *data = stbi_load(path.string().c_str(), &width, &height, &channels, STBI_rgb_alpha); + unsigned char *data = detail::stb_load_wrapper(path.string().c_str(), &width, &height, &channels); if (!data) { std::println("Error: Failed to decode image: {}", path.string()); @@ -34,7 +38,7 @@ } if (width <= 0 || height <= 0) { - stbi_image_free(data); + detail::stb_free_wrapper(data); return result; } @@ -42,7 +46,7 @@ usize const w = static_cast<usize>(width); usize const h = static_cast<usize>(height); if (w > (std::numeric_limits<usize>::max() / 4) / h) { - stbi_image_free(data); + detail::stb_free_wrapper(data); return result; } @@ -55,7 +59,7 @@ result.is_valid = true; // Free the memory allocated by stb - stbi_image_free(data); + detail::stb_free_wrapper(data); return result; }Add the new file to CMakeLists.txt as a regular (non-module) source file.
🤖 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/native/core/io/image_loader.cpp` around lines 8 - 9, stb_image's implementation must be moved out of the C++20 module TU: create a new non-module translation unit (e.g., image_loader_stb.cpp) that defines STB_IMAGE_IMPLEMENTATION, includes <stb_image.h>, and implements two C++ wrappers in namespace draco::core::io::image_loader::detail (e.g., stb_load_wrapper(const char* path, int* w,int* h,int* c) returning unsigned char* and stb_free_wrapper(unsigned char*)): replace direct calls to stbi_load/stbi_image_free in image_loader.cpp with these wrapper functions and add a forward declaration for the wrappers in image_loader.cpp, then add image_loader_stb.cpp to the build (CMakeLists) as a normal non-module source.
🤖 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/native/core/math/vector4.cppm`:
- Around line 66-90: The consteval branches of Vector4::operator[](i32) and the
const overload currently map any invalid index to w; change those default cases
to reject invalid indices (so bad constant indices fail to compile) by throwing
an out-of-range exception (e.g. throw std::out_of_range("Vector4 index out of
range")) or otherwise forcing a compile-time error instead of returning w; leave
the non-consteval path (return (&x)[i]) unchanged and apply the same change to
both Vector4::operator[](i32) and Vector4::operator[](i32) const to ensure
consistent behavior in constexpr evaluation.
In `@engine/native/rendering/mesh/mesh.cpp`:
- Around line 68-81: The functions (e.g., create_cube) currently insert the
returned MeshHandle from create(...) into g_mesh_cache unconditionally, which
caches failed/empty handles; change the logic to only insert into g_mesh_cache
after verifying the handle is valid/non-empty (check the MeshHandle result of
create(...) before assigning g_mesh_cache[key] = h and returning), and apply the
same fix to the other factory functions mentioned (the blocks around the other
similar create_* functions) so failed creations are not cached.
In `@engine/native/rendering/rhi/commands.cpp`:
- Around line 53-64: apply_view currently skips calling bgfx::setViewFrameBuffer
when desc.fb == InvalidFramebuffer, leaving the view's framebuffer sticky and
possibly redirecting later draws; update apply_view so that when desc.fb ==
InvalidFramebuffer you explicitly call bgfx::setViewFrameBuffer(view,
BGFX_INVALID_HANDLE) otherwise resolve the framebuffer handle via
get_checked(g_framebuffers, desc.fb) and call bgfx::setViewFrameBuffer(view,
fb->fbh) as before, preserving the existing RHI_WARN path for invalid handles.
In `@engine/native/rendering/rhi/core.cpp`:
- Around line 127-158: The shutdown() function currently only destroys live
registries and calls bgfx::shutdown(), but skips items already moved into
g_deletion_queue and never releases g_framebuffers; before calling
bgfx::shutdown() flush g_deletion_queue by iterating its entries and performing
the appropriate bgfx::destroy on stored handles (matching the same destroy logic
used in destroy_buffer()/destroy_uniform()), clear the queue, and add a loop
over g_framebuffers.internal().raw() to destroy any live framebuffer handles
(similar to the g_textures/g_uniforms loops). Ensure these flushes happen prior
to bgfx::shutdown() so all deferred native destroys run.
In `@engine/native/rendering/rhi/rhi.cppm`:
- Around line 114-121: The Buffer struct's DynamicIndexBufferHandle member dibh
is not initialized which can leave it indeterminate; modify struct Buffer so
dibh is default-initialized to BGFX_INVALID_HANDLE (matching vbh, dvbh, ibh) to
ensure safe default construction and proper cleanup/update paths, i.e. update
the declaration of dibh inside struct Buffer to use BGFX_INVALID_HANDLE.
In `@engine/native/rendering/rhi/texture.cpp`:
- Around line 54-60: create_framebuffer currently ignores the requested
TextureFormat and always uses bgfx::TextureFormat::RGBA8; change it to map the
incoming TextureFormat to the appropriate bgfx::TextureFormat::Enum before
creating the texture. Update create_framebuffer to convert TextureFormat (the
enum parameter) into a bgfx_format via a switch or small helper (e.g.,
mapTextureFormatToBgfx) covering all supported formats and providing a sensible
fallback (or assert/log) for unsupported values, then pass that bgfx_format into
bgfx::createTexture2D instead of the hardcoded RGBA8.
---
Outside diff comments:
In `@engine/native/core/io/image_loader.cpp`:
- Around line 8-9: stb_image's implementation must be moved out of the C++20
module TU: create a new non-module translation unit (e.g., image_loader_stb.cpp)
that defines STB_IMAGE_IMPLEMENTATION, includes <stb_image.h>, and implements
two C++ wrappers in namespace draco::core::io::image_loader::detail (e.g.,
stb_load_wrapper(const char* path, int* w,int* h,int* c) returning unsigned
char* and stb_free_wrapper(unsigned char*)): replace direct calls to
stbi_load/stbi_image_free in image_loader.cpp with these wrapper functions and
add a forward declaration for the wrappers in image_loader.cpp, then add
image_loader_stb.cpp to the build (CMakeLists) as a normal non-module source.
🪄 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: 7264fcd0-e9da-40d0-874a-b4c0f4b66d0f
📒 Files selected for processing (74)
.clang-format.clang-format-ignoreengine/native/core/containers/string.cppmengine/native/core/definitions/definitions.cppmengine/native/core/definitions/stdtypes.cppmengine/native/core/definitions/version.cppmengine/native/core/io/filesystem.cppengine/native/core/io/filesystem.cppmengine/native/core/io/image_loader.cppengine/native/core/io/image_loader.cppmengine/native/core/io/io.cppmengine/native/core/math/constants.cppmengine/native/core/math/functions.cppmengine/native/core/math/math.test.cppengine/native/core/math/transform.cppengine/native/core/math/transform.cppmengine/native/core/math/types.cppmengine/native/core/math/types_common.cppmengine/native/core/math/vector2.cppmengine/native/core/math/vector3.cppmengine/native/core/math/vector4.cppmengine/native/core/memory/allocator.cppengine/native/core/memory/allocator.cppmengine/native/core/memory/bumpAllocator.cppengine/native/core/memory/bumpAllocator.cppmengine/native/core/memory/bumpAllocator.test.cppengine/native/core/memory/fixedAllocator.cppengine/native/core/memory/fixedAllocator.cppmengine/native/core/memory/fixedAllocator.test.cppengine/native/core/memory/handle.cppmengine/native/core/memory/handle_registry.cppmengine/native/core/memory/pageAllocator.cppengine/native/core/memory/pageAllocator.cppmengine/native/core/memory/slice.cppmengine/native/core/memory/slot_array.cppmengine/native/core/memory/trackingAllocator.cppengine/native/core/memory/trackingAllocator.cppmengine/native/core/memory/trackingAllocator.test.cppengine/native/input/input.cppengine/native/input/input.cppmengine/native/main/main.cppengine/native/platform/cpu/cpu_info.hengine/native/platform/cpu/cpu_info_neon.cppengine/native/platform/cpu/cpu_info_x64.cppengine/native/platform/linux/linux.cppengine/native/platform/platform.cppmengine/native/platform/simd.hengine/native/platform/win32/win32.cppengine/native/rendering/material/material.cppmengine/native/rendering/mesh/mesh.cppengine/native/rendering/mesh/mesh.cppmengine/native/rendering/quad_renderer/quad_renderer.cppengine/native/rendering/quad_renderer/quad_renderer.cppmengine/native/rendering/renderer/renderer.cppengine/native/rendering/renderer/renderer.cppmengine/native/rendering/rendergraph/rendergraph.cppengine/native/rendering/rendergraph/rendergraph.cppmengine/native/rendering/rhi/buffers.cppengine/native/rendering/rhi/commands.cppengine/native/rendering/rhi/core.cppengine/native/rendering/rhi/macros.hengine/native/rendering/rhi/pipelines.cppengine/native/rendering/rhi/rhi.cppmengine/native/rendering/rhi/texture.cppengine/native/rendering/rhi/uniform_registry.cppmengine/native/rendering/rhi/vertex.cppmengine/native/scene/camera/camera_controller.cppengine/native/scene/camera/camera_controller.cppmengine/native/scene/renderable/renderable.cppmengine/native/scene/scene.cppmengine/native/scene/transform_component/transform_component.cppengine/native/scene/transform_component/transform_component.cppmengine/native/thirdparty/doctest/doctest.hengine/native/thirdparty/stb/stb_image.h
✅ Files skipped from review due to trivial changes (51)
- .clang-format-ignore
- engine/native/platform/cpu/cpu_info.h
- engine/native/scene/transform_component/transform_component.cpp
- engine/native/scene/scene.cppm
- engine/native/core/io/filesystem.cppm
- engine/native/core/math/types.cppm
- engine/native/core/math/transform.cppm
- engine/native/core/memory/handle.cppm
- engine/native/core/memory/fixedAllocator.test.cpp
- engine/native/core/containers/string.cppm
- engine/native/core/memory/allocator.cppm
- engine/native/core/memory/slice.cppm
- engine/native/platform/cpu/cpu_info_neon.cpp
- engine/native/core/memory/trackingAllocator.test.cpp
- engine/native/core/io/io.cppm
- engine/native/core/definitions/definitions.cppm
- engine/native/scene/camera/camera_controller.cppm
- engine/native/platform/platform.cppm
- engine/native/scene/transform_component/transform_component.cppm
- engine/native/platform/simd.h
- engine/native/rendering/material/material.cppm
- .clang-format
- engine/native/scene/renderable/renderable.cppm
- engine/native/rendering/rhi/uniform_registry.cppm
- engine/native/core/io/image_loader.cppm
- engine/native/core/memory/handle_registry.cppm
- engine/native/platform/linux/linux.cpp
- engine/native/input/input.cppm
- engine/native/core/memory/trackingAllocator.cppm
- engine/native/core/memory/pageAllocator.cppm
- engine/native/rendering/rhi/vertex.cppm
- engine/native/core/memory/fixedAllocator.cppm
- engine/native/rendering/rhi/macros.h
- engine/native/core/definitions/version.cppm
- engine/native/rendering/renderer/renderer.cppm
- engine/native/rendering/rendergraph/rendergraph.cpp
- engine/native/rendering/rendergraph/rendergraph.cppm
- engine/native/core/definitions/stdtypes.cppm
- engine/native/core/memory/fixedAllocator.cpp
- engine/native/core/memory/trackingAllocator.cpp
- engine/native/platform/cpu/cpu_info_x64.cpp
- engine/native/core/memory/bumpAllocator.test.cpp
- engine/native/rendering/rhi/buffers.cpp
- engine/native/platform/win32/win32.cpp
- engine/native/core/math/functions.cppm
- engine/native/core/math/vector3.cppm
- engine/native/core/io/filesystem.cpp
- engine/native/rendering/quad_renderer/quad_renderer.cppm
- engine/native/rendering/quad_renderer/quad_renderer.cpp
- engine/native/core/math/math.test.cpp
- engine/native/core/math/vector2.cppm
Comment on lines
+66
to
+90
| [[nodiscard]] constexpr f32 &Vector4::operator[](i32 const i) noexcept { | ||
| if consteval { | ||
| switch (i) { | ||
| case 0: return x; | ||
| case 1: return y; | ||
| case 2: return z; | ||
| default: | ||
| case 3: return w; | ||
| } | ||
| } | ||
| else { return (&x)[i]; } | ||
| } | ||
|
|
||
| [[nodiscard]] constexpr f32 const &Vector4::operator[](i32 const i) const noexcept { | ||
| if consteval { | ||
| switch (i) { | ||
| case 0: return x; | ||
| case 1: return y; | ||
| case 2: return z; | ||
| default: | ||
| case 3: return w; | ||
| } | ||
| } | ||
| else { return (&x)[i]; } | ||
| } |
Contributor
There was a problem hiding this comment.
Reject invalid constexpr indices instead of aliasing them to w.
In the if consteval path, any index other than 0/1/2 currently falls into default and returns w. That means v[4] in a constant-evaluated context produces the wrong component instead of surfacing the bug, while the swizzle helpers already reject bad indices.
Suggested fix
[[nodiscard]] constexpr f32 &Vector4::operator[](i32 const i) noexcept {
if consteval {
switch (i) {
case 0: return x;
case 1: return y;
case 2: return z;
- default:
case 3: return w;
+ default: throw "Index out of range";
}
}
else { return (&x)[i]; }
}
[[nodiscard]] constexpr f32 const &Vector4::operator[](i32 const i) const noexcept {
if consteval {
switch (i) {
case 0: return x;
case 1: return y;
case 2: return z;
- default:
case 3: return w;
+ default: throw "Index out of range";
}
}
else { return (&x)[i]; }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| [[nodiscard]] constexpr f32 &Vector4::operator[](i32 const i) noexcept { | |
| if consteval { | |
| switch (i) { | |
| case 0: return x; | |
| case 1: return y; | |
| case 2: return z; | |
| default: | |
| case 3: return w; | |
| } | |
| } | |
| else { return (&x)[i]; } | |
| } | |
| [[nodiscard]] constexpr f32 const &Vector4::operator[](i32 const i) const noexcept { | |
| if consteval { | |
| switch (i) { | |
| case 0: return x; | |
| case 1: return y; | |
| case 2: return z; | |
| default: | |
| case 3: return w; | |
| } | |
| } | |
| else { return (&x)[i]; } | |
| } | |
| [[nodiscard]] constexpr f32 &Vector4::operator[](i32 const i) noexcept { | |
| if consteval { | |
| switch (i) { | |
| case 0: return x; | |
| case 1: return y; | |
| case 2: return z; | |
| case 3: return w; | |
| default: throw "Index out of range"; | |
| } | |
| } | |
| else { return (&x)[i]; } | |
| } | |
| [[nodiscard]] constexpr f32 const &Vector4::operator[](i32 const i) const noexcept { | |
| if consteval { | |
| switch (i) { | |
| case 0: return x; | |
| case 1: return y; | |
| case 2: return z; | |
| case 3: return w; | |
| default: throw "Index out of range"; | |
| } | |
| } | |
| else { return (&x)[i]; } | |
| } |
🤖 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/native/core/math/vector4.cppm` around lines 66 - 90, The consteval
branches of Vector4::operator[](i32) and the const overload currently map any
invalid index to w; change those default cases to reject invalid indices (so bad
constant indices fail to compile) by throwing an out-of-range exception (e.g.
throw std::out_of_range("Vector4 index out of range")) or otherwise forcing a
compile-time error instead of returning w; leave the non-consteval path (return
(&x)[i]) unchanged and apply the same change to both Vector4::operator[](i32)
and Vector4::operator[](i32) const to ensure consistent behavior in constexpr
evaluation.
Comment on lines
+68
to
+81
| MeshHandle create_cube() { | ||
| ensure_mesh_layout(); | ||
|
|
||
| mesh.vertex_count = vertex_count; | ||
| mesh.index_count = static_cast<u32>(indices.size()); | ||
| usize key = 1; | ||
|
|
||
| mesh.valid = (mesh.vbh != rhi::InvalidBuffer) && (mesh.ibh != rhi::InvalidBuffer); | ||
| if (auto it = g_mesh_cache.find(key); it != g_mesh_cache.end()) { return it->second; } | ||
|
|
||
| if (!mesh.valid) | ||
| { | ||
| if (mesh.vbh != rhi::InvalidBuffer) rhi::destroy_buffer(mesh.vbh); | ||
| if (mesh.ibh != rhi::InvalidBuffer) rhi::destroy_buffer(mesh.ibh); | ||
| return {}; | ||
| } | ||
| return g_meshes.create(mesh); | ||
| } | ||
| auto v = gen::cube_vertices(); | ||
| auto i = gen::cube_indices(); | ||
|
|
||
| MeshHandle create_cube() | ||
| { | ||
| ensure_mesh_layout(); | ||
| MeshHandle h = create(v.data(), v.size() * sizeof(Vertex), (u32)v.size(), i, g_mesh_layout); | ||
|
|
||
| usize key = 1; | ||
| g_mesh_cache[key] = h; | ||
| return h; |
Contributor
There was a problem hiding this comment.
Don't cache failed mesh creations.
Each factory stores h in g_mesh_cache even when create(...) returns an empty handle. That makes a transient allocation/init failure permanent for that key, because every later call will hit the cache and immediately return the same invalid handle. Only populate the cache after confirming the mesh creation succeeded.
Also applies to: 84-97, 100-115, 118-133, 136-150
🤖 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/native/rendering/mesh/mesh.cpp` around lines 68 - 81, The functions
(e.g., create_cube) currently insert the returned MeshHandle from create(...)
into g_mesh_cache unconditionally, which caches failed/empty handles; change the
logic to only insert into g_mesh_cache after verifying the handle is
valid/non-empty (check the MeshHandle result of create(...) before assigning
g_mesh_cache[key] = h and returning), and apply the same fix to the other
factory functions mentioned (the blocks around the other similar create_*
functions) so failed creations are not cached.
Comment on lines
+53
to
+64
| void apply_view(ViewID view, ViewDesc const &desc) { | ||
| if (desc.fb != InvalidFramebuffer) { | ||
| auto *fb = get_checked(g_framebuffers, desc.fb, "Framebuffer"); | ||
|
|
||
| if (fb && bgfx::isValid(fb->fbh)) { bgfx::setViewFrameBuffer(view, fb->fbh); } | ||
| else { RHI_WARN(false, "Framebuffer invalid at apply_view"); } | ||
| } | ||
|
|
||
| bgfx::setViewRect(view, desc.x, desc.y, desc.w, desc.h); | ||
|
|
||
| if (desc.clear_flags != 0) { bgfx::setViewClear(view, desc.clear_flags, desc.clear_color); } | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In bgfx, do per-view settings like the framebuffer remain in effect until bgfx::setViewFrameBufferis called again, and isBGFX_INVALID_HANDLE the correct way to restore rendering to the default backbuffer?
💡 Result:
In bgfx, per-view settings like those configured via bgfx::setViewFrameBuffer are persistent throughout the duration of a single frame [1][2]. Because bgfx views are stateful containers that persist until the next call to bgfx::frame, any view configuration (such as the assigned framebuffer) remains in effect for all subsequent draw calls submitted to that same view ID within the same frame [1][2]. If you call bgfx::setViewFrameBuffer multiple times for the same view ID before bgfx::frame is called, only the final configuration is respected [1][2]. Regarding the default backbuffer, yes, BGFX_INVALID_HANDLE is the correct handle to use with bgfx::setViewFrameBuffer to restore or set the rendering target to the default backbuffer [3][4].
Citations:
- 1: Question about view ids bkaradzic/bgfx#1054
- 2: How to setViewTransform for each time bgfx::submit bkaradzic/bgfx#1673
- 3: https://bkaradzic.github.io/bgfx/bgfx.html
- 4: https://docs.rs/bgfx-sys/latest/bgfx_sys/fn.bgfx_set_view_frame_buffer.html
Reset the bgfx view framebuffer when desc.fb is invalid.
apply_view only calls bgfx::setViewFrameBuffer for non-invalid desc.fb; bgfx view framebuffer state is sticky for subsequent draws in the frame, so reusing a view that previously targeted an offscreen framebuffer can cause later passes to render to the wrong target. Use BGFX_INVALID_HANDLE when desc.fb == InvalidFramebuffer.
🐛 Proposed fix
void apply_view(ViewID view, ViewDesc const &desc) {
if (desc.fb != InvalidFramebuffer) {
auto *fb = get_checked(g_framebuffers, desc.fb, "Framebuffer");
if (fb && bgfx::isValid(fb->fbh)) { bgfx::setViewFrameBuffer(view, fb->fbh); }
else { RHI_WARN(false, "Framebuffer invalid at apply_view"); }
}
+ else {
+ bgfx::setViewFrameBuffer(view, BGFX_INVALID_HANDLE);
+ }
bgfx::setViewRect(view, desc.x, desc.y, desc.w, desc.h);
if (desc.clear_flags != 0) { bgfx::setViewClear(view, desc.clear_flags, desc.clear_color); }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| void apply_view(ViewID view, ViewDesc const &desc) { | |
| if (desc.fb != InvalidFramebuffer) { | |
| auto *fb = get_checked(g_framebuffers, desc.fb, "Framebuffer"); | |
| if (fb && bgfx::isValid(fb->fbh)) { bgfx::setViewFrameBuffer(view, fb->fbh); } | |
| else { RHI_WARN(false, "Framebuffer invalid at apply_view"); } | |
| } | |
| bgfx::setViewRect(view, desc.x, desc.y, desc.w, desc.h); | |
| if (desc.clear_flags != 0) { bgfx::setViewClear(view, desc.clear_flags, desc.clear_color); } | |
| } | |
| void apply_view(ViewID view, ViewDesc const &desc) { | |
| if (desc.fb != InvalidFramebuffer) { | |
| auto *fb = get_checked(g_framebuffers, desc.fb, "Framebuffer"); | |
| if (fb && bgfx::isValid(fb->fbh)) { bgfx::setViewFrameBuffer(view, fb->fbh); } | |
| else { RHI_WARN(false, "Framebuffer invalid at apply_view"); } | |
| } | |
| else { | |
| bgfx::setViewFrameBuffer(view, BGFX_INVALID_HANDLE); | |
| } | |
| bgfx::setViewRect(view, desc.x, desc.y, desc.w, desc.h); | |
| if (desc.clear_flags != 0) { bgfx::setViewClear(view, desc.clear_flags, desc.clear_color); } | |
| } |
🤖 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/native/rendering/rhi/commands.cpp` around lines 53 - 64, apply_view
currently skips calling bgfx::setViewFrameBuffer when desc.fb ==
InvalidFramebuffer, leaving the view's framebuffer sticky and possibly
redirecting later draws; update apply_view so that when desc.fb ==
InvalidFramebuffer you explicitly call bgfx::setViewFrameBuffer(view,
BGFX_INVALID_HANDLE) otherwise resolve the framebuffer handle via
get_checked(g_framebuffers, desc.fb) and call bgfx::setViewFrameBuffer(view,
fb->fbh) as before, preserving the existing RHI_WARN path for invalid handles.
Comment on lines
+127
to
+158
| void shutdown() { | ||
| // Walk all registries and destroy live GPU objects | ||
| for (auto &slot : g_buffers.internal().raw()) { | ||
| if (!slot.alive) { continue; } | ||
|
|
||
| if (bgfx::isValid(slot.value.vbh)) { bgfx::destroy(slot.value.vbh); } | ||
|
|
||
| if (bgfx::isValid(slot.value.ibh)) { bgfx::destroy(slot.value.ibh); } | ||
|
|
||
| if (bgfx::isValid(slot.value.dvbh)) { bgfx::destroy(slot.value.dvbh); } | ||
| } | ||
|
|
||
| for (auto &slot : g_pipelines.internal().raw()) { | ||
| if (!slot.alive) { continue; } | ||
|
|
||
| if (bgfx::isValid(slot.value.program)) { bgfx::destroy(slot.value.program); } | ||
| } | ||
|
|
||
| for (auto &slot : g_uniforms.internal().raw()) { | ||
| if (!slot.alive) { continue; } | ||
|
|
||
| if (bgfx::isValid(slot.value)) { bgfx::destroy(slot.value); } | ||
| } | ||
|
|
||
| for (auto &slot : g_textures.internal().raw()) { | ||
| if (!slot.alive) { continue; } | ||
|
|
||
| if (bgfx::isValid(slot.value)) { bgfx::destroy(slot.value); } | ||
| } | ||
|
|
||
| bgfx::shutdown(); | ||
| } |
Contributor
There was a problem hiding this comment.
Flush deferred deletions and destroy framebuffers during shutdown.
shutdown() only walks live registry slots. Anything already passed through destroy_buffer()/destroy_uniform() has been removed from its registry and now exists only in g_deletion_queue, so it is skipped here and bgfx::shutdown() runs before those native destroys ever happen. g_framebuffers is also never drained in this path. Please flush the queued deletions and explicitly release live framebuffers before shutdown.
🤖 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/native/rendering/rhi/core.cpp` around lines 127 - 158, The shutdown()
function currently only destroys live registries and calls bgfx::shutdown(), but
skips items already moved into g_deletion_queue and never releases
g_framebuffers; before calling bgfx::shutdown() flush g_deletion_queue by
iterating its entries and performing the appropriate bgfx::destroy on stored
handles (matching the same destroy logic used in
destroy_buffer()/destroy_uniform()), clear the queue, and add a loop over
g_framebuffers.internal().raw() to destroy any live framebuffer handles (similar
to the g_textures/g_uniforms loops). Ensure these flushes happen prior to
bgfx::shutdown() so all deferred native destroys run.
Comment on lines
+114
to
+121
| struct Buffer { | ||
| bgfx::VertexBufferHandle vbh = BGFX_INVALID_HANDLE; | ||
| bgfx::DynamicVertexBufferHandle dvbh = BGFX_INVALID_HANDLE; | ||
| bgfx::IndexBufferHandle ibh = BGFX_INVALID_HANDLE; | ||
| bgfx::DynamicIndexBufferHandle dibh; | ||
| bool is_dynamic = false; | ||
| bool is_index = false; | ||
| }; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In C++, if a struct member like bgfx::DynamicIndexBufferHandle dibh; has no default member initializer, is that member indeterminate after default construction?
💡 Result:
Yes—if the member has no default member initializer and you default-initialize the enclosing object such that that member is not mentioned in a constructor initializer list, then for automatic storage duration the member’s value is indeterminate (i.e., it has unspecified/indeterminate contents), and reading it is undefined behavior except in limited cases. Reasoning (C++ initialization rules): 1) Default-initialization of an object with automatic (stack) or dynamic storage duration results in “indeterminate value”, and if no initialization is performed for the object, it retains that indeterminate value until it is replaced [1]. 2) When a class object is default-initialized (e.g., S s; for a type with an implicitly-defined default constructor), each non-static data member that is not explicitly initialized by the constructor is also default-initialized; only members with default member initializers are initialized from those initializers [2]. 3) Therefore, for a member like bgfx::DynamicIndexBufferHandle dibh; with no default member initializer, default construction of the containing struct/class leaves dibh indeterminate (for automatic/dynamic storage), unless the handle type’s default constructor gives it a deterministic value. How to ensure a deterministic “null/invalid handle” value: - Use value-initialization: S s{}; (or S s = {};). Value-initialization zero-initializes the object before default construction is applied, which makes scalar subobjects become zero [3][4]. - Or add a default member initializer for the handle member (recommended if bgfx::DynamicIndexBufferHandle has an appropriate invalid/zero state). Net: No default member initializer + default-initialization of an automatic object => the member is indeterminate after construction, unless the member’s type default constructor itself initializes it determinately [1][2].
Citations:
- 1: https://cppreference.dev/w/cpp/language/default_initialization
- 2: https://timsong-cpp.github.io/cppwp/std20/dcl.init
- 3: https://cppreference.dev/w/cpp/language/value_initialization
- 4: https://abseil.io/tips/146
Initialize Buffer::dibh to BGFX_INVALID_HANDLE
In engine/native/rendering/rhi/rhi.cppm (struct Buffer, lines 114-121), dibh lacks a default member initializer; with default construction it can be left indeterminate unless bgfx::DynamicIndexBufferHandle’s default constructor deterministically sets it, which can cause UB when later used for cleanup/update paths. Initialize it to the same invalid sentinel as the other handle members.
🐛 Proposed fix
struct Buffer {
bgfx::VertexBufferHandle vbh = BGFX_INVALID_HANDLE;
bgfx::DynamicVertexBufferHandle dvbh = BGFX_INVALID_HANDLE;
bgfx::IndexBufferHandle ibh = BGFX_INVALID_HANDLE;
- bgfx::DynamicIndexBufferHandle dibh;
+ bgfx::DynamicIndexBufferHandle dibh = BGFX_INVALID_HANDLE;
bool is_dynamic = false;
bool is_index = false;
};🤖 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/native/rendering/rhi/rhi.cppm` around lines 114 - 121, The Buffer
struct's DynamicIndexBufferHandle member dibh is not initialized which can leave
it indeterminate; modify struct Buffer so dibh is default-initialized to
BGFX_INVALID_HANDLE (matching vbh, dvbh, ibh) to ensure safe default
construction and proper cleanup/update paths, i.e. update the declaration of
dibh inside struct Buffer to use BGFX_INVALID_HANDLE.
Comment on lines
+54
to
+60
| FramebufferHandle create_framebuffer(u32 width, u32 height, TextureFormat format) { | ||
| // We set render target flags so it can be attached to a framebuffer object | ||
| u64 flags = BGFX_TEXTURE_RT | BGFX_SAMPLER_U_CLAMP | BGFX_SAMPLER_V_CLAMP; | ||
|
|
||
| bgfx::TextureFormat::Enum bgfx_format = bgfx::TextureFormat::RGBA8; | ||
|
|
||
| bgfx::TextureHandle th = bgfx::createTexture2D(static_cast<u16>(width), static_cast<u16>(height), false, 1, bgfx_format, flags); |
Contributor
There was a problem hiding this comment.
Honor the requested framebuffer format.
format is part of the function contract, but this path always creates an RGBA8 texture. Any caller asking for a different render-target format will silently get the wrong backing texture. Please map TextureFormat to the corresponding bgfx format here instead of hardcoding RGBA8.
🤖 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/native/rendering/rhi/texture.cpp` around lines 54 - 60,
create_framebuffer currently ignores the requested TextureFormat and always uses
bgfx::TextureFormat::RGBA8; change it to map the incoming TextureFormat to the
appropriate bgfx::TextureFormat::Enum before creating the texture. Update
create_framebuffer to convert TextureFormat (the enum parameter) into a
bgfx_format via a switch or small helper (e.g., mapTextureFormatToBgfx) covering
all supported formats and providing a sensible fallback (or assert/log) for
unsupported values, then pass that bgfx_format into bgfx::createTexture2D
instead of the hardcoded RGBA8.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
engine/native/scene/camera/camera_controller.cpp (1)
3-3: 💤 Low valueConsider using
<cmath>instead of<math.h>For C++ code,
<cmath>is the idiomatic header. It places functions likecosf/sinfin thestd::namespace and provides better type safety through overloads. However, since this appears to be pre-existing code that's only being reformatted, this can be addressed in a separate cleanup PR.🤖 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/native/scene/camera/camera_controller.cpp` at line 3, Replace the C header include <math.h> with the C++ header <cmath> in camera_controller.cpp and update any uses of math functions in this file (e.g., cosf, sinf, sqrt, fabs, etc. referenced in functions inside CameraController or related methods) to either qualify them with the std:: namespace or rely on the overloaded names from <cmath>; this keeps the code idiomatic C++ and preserves type-safe overload resolution.
🤖 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 @.clang-format:
- Around line 75-87: Pin the repository to a single clang-format version and
update CI/docs so everyone uses the same formatter: add a clear version marker
at the top of .clang-format (near the existing "TODO Use after upgrading to
Clang 23" and the BinPackParameters/PackParameters lines) and update the CI
formatting job to install and invoke that exact clang-format binary (use the
distro/package name you choose, e.g., clang-format-XX) and fail the build when
diffs are present; also add a short note to repo docs/README explaining which
clang-format version to install and how to run it locally. Ensure the CI job
name and script call the pinned binary (so BreakAfterReturnType,
BreakBeforeBinaryOperators, BinPackParameters/PackParameters behavior is
deterministic) and keep the TODO comment indicating upgrade plans.
In `@engine/native/rendering/rhi/rhi.cppm`:
- Around line 123-126: The FramebufferResource struct's bgfx::FrameBufferHandle
fbh is uninitialized; set it to BGFX_INVALID_HANDLE by adding a default
initializer for fbh (e.g., "fbh = BGFX_INVALID_HANDLE") or provide a default
constructor that initializes fbh to BGFX_INVALID_HANDLE (and leave TextureHandle
texture defaulted) so the handle is never indeterminate after default
construction and cleanup/validation can safely check for BGFX_INVALID_HANDLE.
---
Nitpick comments:
In `@engine/native/scene/camera/camera_controller.cpp`:
- Line 3: Replace the C header include <math.h> with the C++ header <cmath> in
camera_controller.cpp and update any uses of math functions in this file (e.g.,
cosf, sinf, sqrt, fabs, etc. referenced in functions inside CameraController or
related methods) to either qualify them with the std:: namespace or rely on the
overloaded names from <cmath>; this keeps the code idiomatic C++ and preserves
type-safe overload resolution.
🪄 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: 6654f158-2b02-4b28-af27-f48b6fee6700
📒 Files selected for processing (71)
.clang-format.clang-format-ignoreengine/native/core/definitions/definitions.cppmengine/native/core/definitions/stdtypes.cppmengine/native/core/definitions/version.cppmengine/native/core/io/filesystem.cppengine/native/core/io/filesystem.cppmengine/native/core/io/image_loader.cppengine/native/core/io/image_loader.cppmengine/native/core/io/io.cppmengine/native/core/math/constants.cppmengine/native/core/math/functions.cppmengine/native/core/math/math.test.cppengine/native/core/math/transform.cppengine/native/core/math/transform.cppmengine/native/core/math/types.cppmengine/native/core/math/types_common.cppmengine/native/core/math/vector2.cppmengine/native/core/math/vector3.cppmengine/native/core/math/vector4.cppmengine/native/core/memory/allocator.cppengine/native/core/memory/allocator.cppmengine/native/core/memory/bumpAllocator.cppengine/native/core/memory/bumpAllocator.cppmengine/native/core/memory/bumpAllocator.test.cppengine/native/core/memory/fixedAllocator.cppengine/native/core/memory/fixedAllocator.cppmengine/native/core/memory/fixedAllocator.test.cppengine/native/core/memory/handle.cppmengine/native/core/memory/handle_registry.cppmengine/native/core/memory/pageAllocator.cppengine/native/core/memory/pageAllocator.cppmengine/native/core/memory/slice.cppmengine/native/core/memory/slot_array.cppmengine/native/core/memory/trackingAllocator.cppengine/native/core/memory/trackingAllocator.cppmengine/native/core/memory/trackingAllocator.test.cppengine/native/input/input.cppengine/native/input/input.cppmengine/native/main/main.cppengine/native/platform/cpu/cpu_info.hengine/native/platform/cpu/cpu_info_neon.cppengine/native/platform/cpu/cpu_info_x64.cppengine/native/platform/linux/linux.cppengine/native/platform/platform.cppmengine/native/platform/simd.hengine/native/platform/win32/win32.cppengine/native/rendering/material/material.cppmengine/native/rendering/mesh/mesh.cppengine/native/rendering/mesh/mesh.cppmengine/native/rendering/quad_renderer/quad_renderer.cppengine/native/rendering/quad_renderer/quad_renderer.cppmengine/native/rendering/renderer/renderer.cppengine/native/rendering/renderer/renderer.cppmengine/native/rendering/rendergraph/rendergraph.cppengine/native/rendering/rendergraph/rendergraph.cppmengine/native/rendering/rhi/buffers.cppengine/native/rendering/rhi/commands.cppengine/native/rendering/rhi/core.cppengine/native/rendering/rhi/macros.hengine/native/rendering/rhi/pipelines.cppengine/native/rendering/rhi/rhi.cppmengine/native/rendering/rhi/texture.cppengine/native/rendering/rhi/uniform_registry.cppmengine/native/rendering/rhi/vertex.cppmengine/native/scene/camera/camera_controller.cppengine/native/scene/camera/camera_controller.cppmengine/native/scene/renderable/renderable.cppmengine/native/scene/scene.cppmengine/native/scene/transform_component/transform_component.cppengine/native/scene/transform_component/transform_component.cppm
✅ Files skipped from review due to trivial changes (36)
- .clang-format-ignore
- engine/native/platform/cpu/cpu_info.h
- engine/native/core/memory/slice.cppm
- engine/native/core/memory/trackingAllocator.test.cpp
- engine/native/core/io/io.cppm
- engine/native/core/math/types.cppm
- engine/native/core/memory/handle_registry.cppm
- engine/native/core/memory/fixedAllocator.test.cpp
- engine/native/scene/transform_component/transform_component.cppm
- engine/native/core/memory/trackingAllocator.cppm
- engine/native/scene/renderable/renderable.cppm
- engine/native/platform/simd.h
- engine/native/core/io/image_loader.cppm
- engine/native/rendering/rhi/pipelines.cpp
- engine/native/scene/transform_component/transform_component.cpp
- engine/native/core/memory/fixedAllocator.cppm
- engine/native/rendering/rendergraph/rendergraph.cppm
- engine/native/core/memory/fixedAllocator.cpp
- engine/native/core/memory/handle.cppm
- engine/native/core/math/math.test.cpp
- engine/native/core/math/constants.cppm
- engine/native/core/math/transform.cppm
- engine/native/core/definitions/stdtypes.cppm
- engine/native/rendering/material/material.cppm
- engine/native/platform/cpu/cpu_info_x64.cpp
- engine/native/core/io/filesystem.cppm
- engine/native/input/input.cppm
- engine/native/scene/camera/camera_controller.cppm
- engine/native/core/math/vector3.cppm
- engine/native/rendering/rhi/macros.h
- engine/native/core/memory/pageAllocator.cppm
- engine/native/platform/linux/linux.cpp
- engine/native/rendering/renderer/renderer.cppm
- engine/native/core/math/functions.cppm
- engine/native/core/definitions/version.cppm
- engine/native/rendering/quad_renderer/quad_renderer.cppm
🚧 Files skipped from review as they are similar to previous changes (28)
- engine/native/platform/win32/win32.cpp
- engine/native/core/definitions/definitions.cppm
- engine/native/rendering/rhi/uniform_registry.cppm
- engine/native/core/memory/bumpAllocator.test.cpp
- engine/native/core/memory/slot_array.cppm
- engine/native/scene/scene.cppm
- engine/native/core/memory/bumpAllocator.cppm
- engine/native/platform/platform.cppm
- engine/native/core/memory/allocator.cpp
- engine/native/rendering/rendergraph/rendergraph.cpp
- engine/native/rendering/quad_renderer/quad_renderer.cpp
- engine/native/rendering/mesh/mesh.cppm
- engine/native/core/io/image_loader.cpp
- engine/native/rendering/rhi/texture.cpp
- engine/native/rendering/rhi/commands.cpp
- engine/native/core/memory/pageAllocator.cpp
- engine/native/rendering/rhi/core.cpp
- engine/native/rendering/rhi/buffers.cpp
- engine/native/core/memory/allocator.cppm
- engine/native/main/main.cpp
- engine/native/core/math/vector4.cppm
- engine/native/input/input.cpp
- engine/native/rendering/mesh/mesh.cpp
- engine/native/core/math/transform.cpp
- engine/native/core/memory/trackingAllocator.cpp
- engine/native/rendering/renderer/renderer.cpp
- engine/native/core/math/types_common.cppm
- engine/native/core/math/vector2.cppm
Comment on lines
+75
to
+87
| BreakAfterOpenBracketBracedList: false | ||
| BreakAfterOpenBracketFunction: false | ||
| BreakAfterOpenBracketIf: false | ||
| BreakAfterOpenBracketLoop: false | ||
| BreakAfterOpenBracketSwitch: false | ||
| BreakAfterReturnType: Automatic | ||
|
|
||
| BreakBeforeBinaryOperators: None | ||
| BreakBeforeCloseBracketBracedList: true | ||
| BreakBeforeCloseBracketFunction: true | ||
| BreakBeforeCloseBracketIf: true | ||
| BreakBeforeCloseBracketSwitch: true | ||
|
|
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Look for clang-format version pinning in repo docs/CI =="
rg -n -S "clang-format|llvm|clang version|PackParameters|BinPackParameters" -g '!**/build/**'
echo
echo "== Show .clang-format lines around version-sensitive settings =="
nl -ba .clang-format | sed -n '70,95p;155,170p'Repository: Redot-Engine/DraconicEngine
Length of output: 3135
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Search for clang-format pinning/version usage (CI/docs/scripts) =="
rg -n -S --hidden --glob '!.git/*' \
"clang-format[ -]?(1[5-9]|2[0-9]|[3-9][0-9])|clang-format\\b|LLVM_VERSION|clang version|PackParameters|BinPackParameters" \
.github .gitlab ci scripts tools docs . 2>/dev/null | head -n 200
echo
echo "== Show .clang-format lines 70-95 (with line numbers) =="
awk 'NR>=70 && NR<=95 {printf "%4d:%s\n", NR, $0}' .clang-format
echo
echo "== Show .clang-format lines 155-170 (with line numbers) =="
awk 'NR>=155 && NR<=170 {printf "%4d:%s\n", NR, $0}' .clang-formatRepository: Redot-Engine/DraconicEngine
Length of output: 3186
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== .clang-format lines 70-95 =="
awk 'NR>=70 && NR<=95 {printf "%4d:%s\n", NR, $0}' .clang-format
echo
echo "== .clang-format lines 155-170 =="
awk 'NR>=155 && NR<=170 {printf "%4d:%s\n", NR, $0}' .clang-formatRepository: Redot-Engine/DraconicEngine
Length of output: 1350
Pin and enforce a single clang-format version for this config.
This .clang-format includes a Clang 23–scoped migration note—# TODO Use after upgrading to Clang 23 and currently uses BinPackParameters (with PackParameters commented out)—so different clang-format versions can produce divergent formatting. No explicit repo CI/docs pin for a specific clang-format/Clang version was found (beyond clang-format off/on markers in third-party code). Update CI/docs to install/run the same formatter version for formatting checks.
🤖 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 @.clang-format around lines 75 - 87, Pin the repository to a single
clang-format version and update CI/docs so everyone uses the same formatter: add
a clear version marker at the top of .clang-format (near the existing "TODO Use
after upgrading to Clang 23" and the BinPackParameters/PackParameters lines) and
update the CI formatting job to install and invoke that exact clang-format
binary (use the distro/package name you choose, e.g., clang-format-XX) and fail
the build when diffs are present; also add a short note to repo docs/README
explaining which clang-format version to install and how to run it locally.
Ensure the CI job name and script call the pinned binary (so
BreakAfterReturnType, BreakBeforeBinaryOperators,
BinPackParameters/PackParameters behavior is deterministic) and keep the TODO
comment indicating upgrade plans.
Comment on lines
+123
to
+126
| struct FramebufferResource { | ||
| bgfx::FrameBufferHandle fbh; | ||
| TextureHandle texture; | ||
| }; |
Contributor
There was a problem hiding this comment.
Initialize FramebufferResource::fbh to BGFX_INVALID_HANDLE
Similar to Buffer::dibh, the fbh member lacks a default initializer. This could leave it indeterminate after default construction, causing undefined behavior in cleanup or validation paths.
struct FramebufferResource {
- bgfx::FrameBufferHandle fbh;
+ bgfx::FrameBufferHandle fbh = BGFX_INVALID_HANDLE;
TextureHandle texture;
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| struct FramebufferResource { | |
| bgfx::FrameBufferHandle fbh; | |
| TextureHandle texture; | |
| }; | |
| struct FramebufferResource { | |
| bgfx::FrameBufferHandle fbh = BGFX_INVALID_HANDLE; | |
| TextureHandle texture; | |
| }; |
🤖 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/native/rendering/rhi/rhi.cppm` around lines 123 - 126, The
FramebufferResource struct's bgfx::FrameBufferHandle fbh is uninitialized; set
it to BGFX_INVALID_HANDLE by adding a default initializer for fbh (e.g., "fbh =
BGFX_INVALID_HANDLE") or provide a default constructor that initializes fbh to
BGFX_INVALID_HANDLE (and leave TextureHandle texture defaulted) so the handle is
never indeterminate after default construction and cleanup/validation can safely
check for BGFX_INVALID_HANDLE.
| AccessModifierOffset: 0 | ||
|
|
||
| AlignArrayOfStructures: Right | ||
| AlignAfterOpenBracket: true |
Contributor
There was a problem hiding this comment.
I'd prefer AlwaysBreak. This is important to comment out parameters without having to worry if it is the first or last one.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is a draft of a clang-format document.
I want comments on anything that should be changed, these are just what I think might be good defaults. I will run it on the project before it gets merged.
Please go to https://clang.llvm.org/docs/ClangFormatStyleOptions.html for referencing each value. I tried to go in order but some at the top are ones I think are most important, but thinking back on it probably should have gone in order
Summary by CodeRabbit
Style
Chores
Note: This release contains internal code maintenance and modernization with no user-facing changes or new features.