Tool mode state and tool palette - #80
Conversation
📝 WalkthroughWalkthroughThis PR introduces an explicit Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/prune/scene/simple_shooter/simple_shooter_scene.cpp (1)
83-89:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winNormalise
player_idbefore mutating the loaded player objectLine 83 currently patches
runtime.behaviourandeditor.movablebeforeplayer_idis validated. Ifplayer_idpoints at a non-player object, the wrong object is mutated and the resolved player may never geteditor.movable = true.🔧 Suggested fix
- if (GameObject* player = state.objects.get_by_id(shooter_state.player_id)) { - if (player->runtime.behaviour.empty()) { - player->runtime.behaviour = simple_shooter_ids::player_behaviour; - } - - player->editor.movable = true; - } - restore_legacy_wall_concepts(state); if (!object_has_kind(state, shooter_state.player_id, simple_shooter_concepts::ObjectKind::Player)) { shooter_state.player_id = first_object_id_for_kind(state, simple_shooter_concepts::ObjectKind::Player); } if (shooter_state.player_id == k_invalid_game_object_id) { shooter_state.player_id = state.objects.create_object(simple_shooter_factory::create_player()); } + + if (GameObject* player = state.objects.get_by_id(shooter_state.player_id)) { + if (player->runtime.behaviour.empty()) { + player->runtime.behaviour = simple_shooter_ids::player_behaviour; + } + + player->editor.movable = true; + }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 97ac6068-b317-47d7-8666-de65844ba3b8
⛔ Files ignored due to path filters (3)
assets/repo/artillery.pngis excluded by!**/*.pngassets/repo/platformer.pngis excluded by!**/*.pngassets/repo/simple-shooter.pngis excluded by!**/*.png
📒 Files selected for processing (15)
DECISIONS.mdNOTES.mdsrc/prune/editor/editor_tool.hppsrc/prune/scene/artillery/artillery_scene.cppsrc/prune/scene/platformer/platformer_scene.cppsrc/prune/scene/scene.hppsrc/prune/scene/scene_interaction.cppsrc/prune/scene/scene_interaction.hppsrc/prune/scene/scene_state.hppsrc/prune/scene/simple_shooter/simple_shooter_scene.cppsrc/prune/scene/world_scene.cppsrc/prune/scene/world_scene.hppsrc/prune/tooling/controls.cppsrc/prune/tooling/editor_layout.cppsrc/prune/tooling/ui.cpp
| tooling::imgui::layout::text_wrapped("Shift+Left click toggles objects in the selection set"); | ||
| tooling::imgui::layout::text_wrapped("Empty viewport click clears the selection set"); | ||
| tooling::imgui::layout::text_wrapped("Mouse button left on a selection handle moves the selected object or selected objects"); | ||
| tooling::imgui::layout::text_wrapped("Use the Select/Move tool buttons in the main menu bar to change viewport tool mode"); |
There was a problem hiding this comment.
Update the tool location text to match the actual UI.
The tools are rendered in the viewport palette, not in the main menu bar, so this instruction is currently misleading.
| ImGui::PushID(action.id.data()); | ||
|
|
||
| if (ImGui::Button(action.label.data(), ImVec2(button_width, 0.0f))) { | ||
| scene.execute_scene_creation_action(action.id); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify SceneCreationAction string field types and call sites that pass .data() to ImGui APIs.
rg -n -C3 'struct\s+SceneCreationAction|SceneCreationAction' src
rg -n -C3 'ImGui::(Button|PushID)\(' src/prune/tooling/ui.cpp
rg -n -C3 '\.id\.data\(\)|\.label\.data\(\)' srcRepository: deanblackborough/Prune
Length of output: 11326
Ensure string_view::data() is NUL-terminated before passing to ImGui APIs.
action.id.data() and action.label.data() assume NUL-termination, but std::string_view doesn't guarantee it in its contract. Right now you're safe because all the static action arrays use string literals, but if those views ever come from slicing a longer string or a non-literal source, you'll get undefined behaviour. Think of it as a defensive bet—protect the API boundary now, before someone else inherits this code and accidentally breaks it.
The fix is straightforward: use the range overload for ImGui::PushID() and convert the label to a temporary std::string to guarantee NUL-termination.
Suggested fix
void draw_scene_creation_action_button(
Scene& scene,
const SceneCreationAction& action,
float button_width
)
{
- ImGui::PushID(action.id.data());
-
- if (ImGui::Button(action.label.data(), ImVec2(button_width, 0.0f))) {
+ const std::string button_text(action.label);
+ ImGui::PushID(action.id.data(), action.id.data() + action.id.size());
+ if (ImGui::Button(button_text.c_str(), ImVec2(button_width, 0.0f))) {
scene.execute_scene_creation_action(action.id);
}
ImGui::PopID();
}🧰 Tools
🪛 Clang (14.0.6)
[warning] 129-129: floating point literal has suffix 'f', which is not uppercase
(readability-uppercase-literal-suffix)
hoanbin631-debug
left a comment
hoanbin631-debug
left a comment
There was a problem hiding this comment.
Approved in CodeRabbit Change Stack
hoanbin631-debug
left a comment
hoanbin631-debug
left a comment
There was a problem hiding this comment.
Requested changes in CodeRabbit Change Stack
2 participants
Summary by CodeRabbit
New Features
Documentation