Conversation
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughThis PR adds a new Notes.md roadmap and introduces a viewport abstraction (prune::SceneViewport) carrying position, size and hover/focus state. SandboxScene replaces width/height APIs with set_viewport/get_viewport and uses the new viewport for camera, input gating and coordinate conversion. App renames its UI member to m_ui and forwards SDL events directly to input. Input now updates mouse coordinates on button events. Ui now renders the scene into an SDL texture render target and exposes Ui::render(SandboxScene&, SDL_Renderer*). Tooling signatures (Stats::draw, property_table) were adjusted to use SceneViewport. No behavioural game-logic changes beyond input gating. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 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. ✨ 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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/prune/app/app.cpp (2)
107-114:⚠️ Potential issue | 🟡 MinorRemove the duplicate resize refresh.
m_window->refresh_size()is called twice for the sameSDL_WINDOWEVENT_SIZE_CHANGED. Harmless, but it does duplicate work on every resize and looks like a copy/paste slip.🧹 Proposed fix
case SDL_WINDOWEVENT_SIZE_CHANGED: if (m_window) { m_window->refresh_size(); } - - if (m_window) { - m_window->refresh_size(); - } break;
132-137:⚠️ Potential issue | 🟠 MajorGate keyboard events to prevent app shortcuts firing whilst ImGui is capturing input.
The app closes unexpectedly when users press Escape whilst typing in ImGui widgets. The scene properly gates its own input handling, but
App::run()checks the Escape shortcut unconditionally after callingprocess_events(). Becauseprocess_events()forwards all keyboard events tom_inputwithout checkingImGui::GetIO().WantCaptureKeyboard, the Input system records the key press regardless of ImGui's state.Refactor to check ImGui's keyboard capture flag before forwarding keyboard events:
Proposed fix
while (SDL_PollEvent(&event)) { ImGui_ImplSDL2_ProcessEvent(&event); handle_event(event); - m_input->process_event(event); + const bool is_keyboard_event = + event.type == SDL_KEYDOWN || event.type == SDL_KEYUP; + if (!is_keyboard_event || !ImGui::GetIO().WantCaptureKeyboard) { + m_input->process_event(event); + } }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 65ce4bd0-7217-4a7e-870c-48e60ab450d7
📒 Files selected for processing (12)
Notes.mdsrc/prune/app/app.cppsrc/prune/app/app.hppsrc/prune/core/input.cppsrc/prune/scene/sandbox_scene.cppsrc/prune/scene/sandbox_scene.hppsrc/prune/scene/sandbox_scene_editor.cppsrc/prune/tooling/imgui/property_table.cppsrc/prune/tooling/stats.cppsrc/prune/tooling/stats.hppsrc/prune/tooling/ui.cppsrc/prune/tooling/ui.hpp
|
|
||
| Don’t over-engineer this | ||
| Don’t build a full framework yet | ||
| Just stop SandboxScene becoming the permanent dumping ground No newline at end of file |
There was a problem hiding this comment.
Add a trailing newline at the end of the file.
Standard Markdown formatting expects files to end with a single newline character. This keeps tooling happy and follows best practices.
📝 Proposed fix
Add a single newline after the last line of content.
Just stop SandboxScene becoming the permanent dumping ground
+As per static analysis: MD047 (single-trailing-newline).
📝 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.
| Just stop SandboxScene becoming the permanent dumping ground | |
| Just stop SandboxScene becoming the permanent dumping ground |
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 144-144: Files should end with a single newline character
(MD047, single-trailing-newline)
| if (!m_viewport.hovered) { | ||
| return; | ||
| } | ||
|
|
||
| if (!input.was_mouse_button_pressed(SDL_BUTTON_LEFT)) { | ||
| return; | ||
| } | ||
|
|
||
| if (!m_viewport.contains(input.mouse_x(), input.mouse_y())) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Don’t gate picking on cached hovered state.
m_viewport.hovered is filled during UI render, but handle_scene_click() consumes it in the next update. That makes the first click after moving into the scene panel a no-op, because the hover flag is still from the previous frame. The coordinate check is the reliable guard here; keep contains(input.mouse_x(), input.mouse_y()) and drop the stale hovered early-return.
🛠️ Proposed fix
- if (!m_viewport.hovered) {
- return;
- }
-
if (!input.was_mouse_button_pressed(SDL_BUTTON_LEFT)) {
return;
}
if (!m_viewport.contains(input.mouse_x(), input.mouse_y())) {
return;
}| begin_row(label); | ||
| return ImGui::DragFloat(id, &value, speed, min, max, format); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect drag_float call sites and ID argument style.
rg -nP -C2 --iglob '*.{cpp,cc,cxx,h,hpp,hxx}' '\b(?:property_table::)?drag_float\s*\('
echo "---- callsites with string literal label + id ----"
rg -nP --iglob '*.{cpp,cc,cxx,h,hpp,hxx}' \
'\b(?:property_table::)?drag_float\s*\(\s*"[^"]+"\s*,\s*"[^"]+"'
echo "---- callsites where id is already hidden (starts with ##) ----"
rg -nP --iglob '*.{cpp,cc,cxx,h,hpp,hxx}' \
'\b(?:property_table::)?drag_float\s*\(\s*"[^"]+"\s*,\s*"##[^"]*"'Repository: deanblackborough/Prune
Length of output: 4142
🏁 Script executed:
sed -n '97,110p' src/prune/tooling/imgui/property_table.cppRepository: deanblackborough/Prune
Length of output: 360
🏁 Script executed:
sed -n '70,110p' src/prune/tooling/imgui/property_table.cppRepository: deanblackborough/Prune
Length of output: 1074
🏁 Script executed:
sed -n '1,70p' src/prune/tooling/imgui/property_table.cppRepository: deanblackborough/Prune
Length of output: 1663
Restore hidden-ID semantics in drag_float to match combo pattern.
Right catch here. Look at the combo function a few lines up—it uses PushID(id) paired with a hidden label ("##value"). That's the established pattern in this file for keeping internal IDs out of the UI. Your drag_float refactor at line 108 passes id directly to ImGui::DragFloat, which exposes it as a visible label.
All your call sites use nice descriptive IDs ("editor_x", "game_x", "transform_x", etc.), so they'll now show up in the value column—changing the UI contract quietly. The begin_row(label) refactor is solid, but let's keep the ID-hiding semantics consistent with how combo does it.
Apply the suggested fix: wrap the ImGui call with PushID/PopID and use "##value" as the label. That keeps your layout cleanup while maintaining the established pattern.
| if (!m_show_scene_viewport) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Reset scene viewport when the Scene panel is hidden.
Line 218 returns early without clearing the scene viewport. That leaves stale focused/width/height in SandboxScene, so scene input can remain enabled even after hiding the panel.
Suggested fix
if (!m_show_scene_viewport) {
+ scene.set_viewport({});
return;
}| ~Ui(); | ||
| void render(SandboxScene& scene, SDL_Renderer* renderer); | ||
|
|
||
| private: | ||
| void draw_scene_viewport(SandboxScene& scene, SDL_Renderer* renderer); | ||
| void ensure_scene_render_target(SDL_Renderer* renderer, int width, int height); | ||
| void destroy_scene_render_target(); | ||
|
|
||
| SDL_Texture* m_scene_render_target = nullptr; | ||
| int m_scene_render_target_width = 0; | ||
| int m_scene_render_target_height = 0; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and read the file to verify the exact code at those lines
fd -t f "ui.hpp" | grep -E "prune/tooling"Repository: deanblackborough/Prune
Length of output: 90
🏁 Script executed:
# Read the file to see the class declaration context
cat -n src/prune/tooling/ui.hpp | head -40Repository: deanblackborough/Prune
Length of output: 1373
🏁 Script executed:
# Search for any copy/move operation definitions or deletions in the Ui class
rg -A 20 "class Ui" src/prune/tooling/ui.hppRepository: deanblackborough/Prune
Length of output: 754
🏁 Script executed:
# Get the complete class declaration
cat -n src/prune/tooling/ui.hppRepository: deanblackborough/Prune
Length of output: 1775
🏁 Script executed:
# Search for any copy/move operation declarations (deleted, defined, etc.)
rg "operator=|Ui\(const Ui|Ui\(Ui" src/prune/tooling/ui.hppRepository: deanblackborough/Prune
Length of output: 48
🏁 Script executed:
# Check the destructor implementation to see what it does
rg -A 10 "Ui::~Ui" src/prune/tooling/ui.cppRepository: deanblackborough/Prune
Length of output: 335
Mark the Ui class as non-copyable to prevent accidental double-destruction of the SDL texture.
This class now owns an SDL_Texture* and your destructor cleans it up by calling destroy_scene_render_target(). Without explicitly deleting the copy and move operations, the compiler generates defaults that shallow-copy the pointer—and that's a recipe for a crash when both copies get destroyed. In game engine code, this kind of silent corruption is exactly the bug that haunts you in the wild. Either delete the copy/move operations (simplest fix for now) or wrap the texture in an RAII type if you need to share it later.
Suggested fix
class Ui {
public:
~Ui();
+ Ui(const Ui&) = delete;
+ Ui& operator=(const Ui&) = delete;
+ Ui(Ui&&) = delete;
+ Ui& operator=(Ui&&) = delete;
void render(SandboxScene& scene, SDL_Renderer* renderer);📝 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.
| ~Ui(); | |
| void render(SandboxScene& scene, SDL_Renderer* renderer); | |
| private: | |
| void draw_scene_viewport(SandboxScene& scene, SDL_Renderer* renderer); | |
| void ensure_scene_render_target(SDL_Renderer* renderer, int width, int height); | |
| void destroy_scene_render_target(); | |
| SDL_Texture* m_scene_render_target = nullptr; | |
| int m_scene_render_target_width = 0; | |
| int m_scene_render_target_height = 0; | |
| ~Ui(); | |
| Ui(const Ui&) = delete; | |
| Ui& operator=(const Ui&) = delete; | |
| Ui(Ui&&) = delete; | |
| Ui& operator=(Ui&&) = delete; | |
| void render(SandboxScene& scene, SDL_Renderer* renderer); | |
| private: | |
| void draw_scene_viewport(SandboxScene& scene, SDL_Renderer* renderer); | |
| void ensure_scene_render_target(SDL_Renderer* renderer, int width, int height); | |
| void destroy_scene_render_target(); | |
| SDL_Texture* m_scene_render_target = nullptr; | |
| int m_scene_render_target_width = 0; | |
| int m_scene_render_target_height = 0; |
deanblackborough
dismissed
coderabbitai[bot]’s stale review
Still in earlier development
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Documentation