Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughApp::handle_event now switches on SDL window sub-events: it handles SDL_WINDOWEVENT_CLOSE by stopping the main loop and SDL_WINDOWEVENT_SIZE_CHANGED by calling Window::refresh_size() and, if a scene exists, updating the scene viewport via SandboxScene::set_viewport_size(width, height). Window creation now composes SDL window flags from config (resizable/fullscreen), calls refresh_size() after creating the renderer, and refresh_size() prefers renderer output size then falls back to SDL_GetWindowSize. SandboxScene exposes viewport setters/getters. Tooling changes propagate viewport and grid state through Outliner, Inspector and Stats; Outliner gained spawn-placement helpers and persistent search state; Inspector gained a persistent rename buffer and grid-aware clone offset. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5f355668-b509-417a-b132-0d56e85a5d84
📒 Files selected for processing (8)
src/prune/app/app.cppsrc/prune/core/window.cppsrc/prune/core/window.hppsrc/prune/scene/sandbox_scene.cppsrc/prune/scene/sandbox_scene.hppsrc/prune/tooling/stats.cppsrc/prune/tooling/stats.hppsrc/prune/tooling/ui.cpp
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 062b6ecf-91ad-4c48-935d-897c6eb0f052
📒 Files selected for processing (2)
src/prune/core/window.cppsrc/prune/tooling/stats.cpp
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 736055d9-57de-4176-9ffb-0b0f0ccd08a0
📒 Files selected for processing (2)
src/prune/tooling/inspector.cppsrc/prune/tooling/inspector.hpp
| selected->name = objects.make_unique_name(m_rename_buffer.data(), selected->id); | ||
| std::snprintf( | ||
| m_rename_buffer.data(), | ||
| m_rename_buffer.size(), | ||
| "%s", | ||
| selected->name.c_str() | ||
| ); |
There was a problem hiding this comment.
Prevent silent name truncation before persisting edits.
Line [223] copies into a fixed 128-byte buffer, and Line [53] persists from that buffer. Since names are unbounded strings, long names can be truncated and then saved back as shortened values.
Suggested fix direction (dynamic buffer)
- std::array<char, 128> m_rename_buffer{};
+ std::vector<char> m_rename_buffer = std::vector<char>(128, '\0'); void Inspector::sync_rename_buffer(const GameObject* selected)
{
if (!selected) {
m_rename_target_id.reset();
- m_rename_buffer[0] = '\0';
+ if (!m_rename_buffer.empty()) {
+ m_rename_buffer[0] = '\0';
+ }
return;
}
if (m_rename_target_id.has_value() && m_rename_target_id.value() == selected->id) {
return;
}
m_rename_target_id = selected->id;
+ const std::size_t required = selected->name.size() + 64;
+ if (m_rename_buffer.size() < required) {
+ m_rename_buffer.resize(required, '\0');
+ }
std::snprintf(m_rename_buffer.data(), m_rename_buffer.size(), "%s", selected->name.c_str());
} if (ImGui::IsItemDeactivatedAfterEdit()) {
selected->name = objects.make_unique_name(m_rename_buffer.data(), selected->id);
+ const std::size_t required = selected->name.size() + 64;
+ if (m_rename_buffer.size() < required) {
+ m_rename_buffer.resize(required, '\0');
+ }
std::snprintf(
m_rename_buffer.data(),
m_rename_buffer.size(),
"%s",
selected->name.c_str()
);
}Also applies to: 210-224
There was a problem hiding this comment.
Actionable comments posted: 5
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: a5a89249-8583-4f4c-ac7a-94ef9e67f31e
📒 Files selected for processing (5)
src/prune/tooling/inspector.cppsrc/prune/tooling/inspector.hppsrc/prune/tooling/outliner.cppsrc/prune/tooling/outliner.hppsrc/prune/tooling/ui.cpp
| std::optional<GameObjectId> m_rename_target_id; | ||
| std::array<char, 128> m_rename_buffer{}; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Inconsistent indentation detected.
Look at line 36 closely—there's a tab character sneaking in there among your spaces. In game engines, consistent formatting isn't just pedantry; it prevents merge conflicts and keeps diffs clean when multiple people are touching the same files.
♻️ Suggested fix
std::optional<GameObjectId> m_rename_target_id;
- std::array<char, 128> m_rename_buffer{};
+ std::array<char, 128> m_rename_buffer{};📝 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.
| std::optional<GameObjectId> m_rename_target_id; | |
| std::array<char, 128> m_rename_buffer{}; | |
| std::optional<GameObjectId> m_rename_target_id; | |
| std::array<char, 128> m_rename_buffer{}; |
| void draw( | ||
| GameObjectManager& objects, | ||
| float camera_x, | ||
| float camera_y, | ||
| int viewport_width, | ||
| int viewport_height, | ||
| bool snap_to_grid, | ||
| int grid_size | ||
| ); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider consolidating parameters in future.
Seven parameters on draw() is starting to push the limits of cognitive load. In game engine code, when you see this pattern, it's often a signal that a "context" or "config" struct might be warranted. Something like OutlinerDrawContext holding viewport, camera, and grid info.
Not blocking—this works fine for now. Just file it away as technical debt for when this inevitably grows.
| int width, | ||
| int height | ||
| ) const; | ||
| bool contains_case_insensitive(std::string_view text, std::string_view query) const; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add [[nodiscard]] for consistency.
Static analysis correctly points out that contains_case_insensitive should be marked [[nodiscard]] too. It's a pure predicate—discarding its result makes no sense.
♻️ Suggested fix
- bool contains_case_insensitive(std::string_view text, std::string_view query) const;
+ [[nodiscard]] bool contains_case_insensitive(std::string_view text, std::string_view query) const;📝 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.
| bool contains_case_insensitive(std::string_view text, std::string_view query) const; | |
| [[nodiscard]] bool contains_case_insensitive(std::string_view text, std::string_view query) const; |
🧰 Tools
🪛 Clang (14.0.6)
[warning] 50-50: function 'contains_case_insensitive' should be marked [[nodiscard]]
(modernize-use-nodiscard)
[warning] 50-50: use a trailing return type for this function
(modernize-use-trailing-return-type)
| } | ||
|
|
||
| } // namespace prune No newline at end of file | ||
| } No newline at end of file |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Missing newline at end of file.
Most style guides and version control systems prefer a trailing newline at EOF. It's a Unix convention that prevents "no newline at end of file" warnings in diffs. Trivial fix, but good habit to build.
deanblackborough
dismissed
coderabbitai[bot]’s stale review
My project, not the AIs
1 participant
Summary by CodeRabbit
New Features
Improvements