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:
📝 WalkthroughWalkthroughThis PR adds YAML-based scene persistence and UI commands for New/Save/Load of the sandbox scene. A new serialization unit implements scene save/load (scene metadata, camera, grid, options, objects) and integrates with SandboxScene::save_to_file / load_from_file. GameObjectManager gains add_loaded_object, set_selected_id, set_next_id and next_id APIs. SandboxScene initialisation is refactored into new_scene, reset_runtime_state and restore_defaults. UI now exposes File → New/Save/Load with status feedback. Build, vcpkg manifest, CMake and Windows CI were updated to locate and link yaml-cpp. Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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: 8
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1519b6e6-2b02-4708-bf6c-70b0543aa81c
📒 Files selected for processing (10)
CMakeLists.txtREADME.mdsrc/prune/scene/game_object_manager.cppsrc/prune/scene/game_object_manager.hppsrc/prune/scene/sandbox_scene.cppsrc/prune/scene/sandbox_scene.hppsrc/prune/scene/sandbox_scene_serialization.cppsrc/prune/tooling/outliner.cppsrc/prune/tooling/ui.cppsrc/prune/tooling/ui.hpp
| ## Next | ||
| - Camera and limitless world space foundation | ||
| ### Foundation | ||
| - Camera and limitless world space | ||
| - Editor camera vs player camera separation | ||
| - Object types (player, blocks, platforms, hazards, etc.) | ||
| - Static sprites | ||
|
|
||
| ### Editing tools | ||
| - Gizmos for object movement | ||
| - Save and load | ||
| - Animated sprites | ||
| - Editor camera | ||
| - Player camera | ||
| - Gizmos for scaling the selected object | ||
|
|
||
| ### Core systems | ||
| - Undo/redo system | ||
| - ENTT at some point | ||
| - Multi-scene support, game type per scene (top-down, platformer, etc) | ||
| - Improved save and load (v2) | ||
|
|
||
| ### Content | ||
| - Animated sprites | ||
|
|
||
| ### Architecture | ||
| - Multi-scene support (scene types: platformer, top-down, etc.) |
There was a problem hiding this comment.
Give these headings a blank line each side.
Markdownlint is right here: the ##/### headings in Next are packed straight against neighbouring content, so this section will keep failing MD022 until you separate them.
💡 Minimal tidy-up
## Next
+
### Foundation
+
- Camera and limitless world space
- Editor camera vs player camera separation
- Object types (player, blocks, platforms, hazards, etc.)
- Static sprites
+
### Editing tools
+
- Gizmos for object movement
- Gizmos for scaling the selected object
+
### Core systems
+
- Undo/redo system
- Improved save and load (v2)
+
### Content
+
- Animated sprites
+
### Architecture
+
- Multi-scene support (scene types: platformer, top-down, etc.)
- ENTT (ECS integration)🧰 Tools
🪛 LanguageTool
[uncategorized] ~177-~177: It seems likely that a singular genitive (’s) apostrophe is missing.
Context: ...r scaling the selected object ### Core systems - Undo/redo system - Improved save and ...
(AI_HYDRA_LEO_APOSTROPHE_S_XS)
🪛 markdownlint-cli2 (0.22.0)
[warning] 166-166: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 167-167: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Above
(MD022, blanks-around-headings)
[warning] 167-167: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 173-173: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 177-177: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 181-181: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 184-184: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
| void GameObjectManager::set_next_id(GameObjectId next_id) noexcept | ||
| { | ||
| m_next_id = std::max<GameObjectId>(1, next_id); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify std::max usage and direct include coverage in the touched files.
rg -n 'std::max|#include <algorithm>' src/prune/scene/game_object_manager.cpp src/prune/scene/game_object_manager.hppRepository: deanblackborough/Prune
Length of output: 164
🏁 Script executed:
head -30 src/prune/scene/game_object_manager.cppRepository: deanblackborough/Prune
Length of output: 765
🏁 Script executed:
head -30 src/prune/scene/game_object_manager.hppRepository: deanblackborough/Prune
Length of output: 1034
🏁 Script executed:
head -20 src/prune/scene/game_object.hppRepository: deanblackborough/Prune
Length of output: 387
🏁 Script executed:
# Check if <vector> transitively provides std::max by inspecting a minimal test
cat > /tmp/test_max.cpp << 'EOF'
`#include` <vector>
int main() {
int x = std::max(1, 2);
return x;
}
EOF
# Try to compile with standard library only (this will fail if <algorithm> is truly needed)
echo "Note: checking if std::max is available through <vector> alone..."Repository: deanblackborough/Prune
Length of output: 131
🏁 Script executed:
fd -t f "game_object_manager" src/prune/Repository: deanblackborough/Prune
Length of output: 145
Add <algorithm> for std::max usage.
Line 54 uses std::max<GameObjectId>, but the file doesn't include <algorithm>. It's relying on something it hasn't explicitly asked for—bad habit in C++. You'll find your code breaks on stricter toolchains or when header inclusion order changes. Make it explicit:
Proposed fix
`#include` "prune/scene/game_object_manager.hpp"
+#include <algorithm>📝 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.
| void GameObjectManager::set_next_id(GameObjectId next_id) noexcept | |
| { | |
| m_next_id = std::max<GameObjectId>(1, next_id); | |
| } | |
| `#include` "prune/scene/game_object_manager.hpp" | |
| `#include` <algorithm> |
🧰 Tools
🪛 Cppcheck (2.20.0)
[style] 52-52: The function 'set_next_id' is never used.
(unusedFunction)
| std::ofstream output{ std::string(path) }; | ||
| if (!output.is_open()) { | ||
| error = "Could not open save file for writing."; | ||
| return false; | ||
| } | ||
|
|
||
| output << root; | ||
| return true; |
There was a problem hiding this comment.
Check the write, not just the open.
is_open() only proves the handle was created. If the disk fills up or the flush fails after output << root, this still returns true and the editor will think the scene was saved when it was not.
💡 Tighten the save-path error handling
std::ofstream output{ std::string(path) };
if (!output.is_open()) {
error = "Could not open save file for writing.";
return false;
}
output << root;
+ output.flush();
+ if (!output) {
+ error = "Failed to write save file.";
+ return false;
+ }
return true;🧰 Tools
🪛 Clang (14.0.6)
[warning] 174-174: variable 'output' is not initialized
(cppcoreguidelines-init-variables)
[warning] 177-177: redundant boolean literal in conditional return statement
(readability-simplify-boolean-expr)
| reset_runtime_state(); | ||
|
|
||
| GameObjectId loaded_next_id = 1; | ||
| GameObjectId loaded_player_id = kInvalidGameObjectId; | ||
| GameObjectId loaded_selected_id = kInvalidGameObjectId; | ||
|
|
||
| if (!read_required_uint(scene, "next_object_id", loaded_next_id)) { | ||
| error = "scene.next_object_id is missing."; | ||
| return false; | ||
| } | ||
|
|
||
| if (!read_required_uint(scene, "player_id", loaded_player_id)) { | ||
| error = "scene.player_id is missing."; | ||
| return false; | ||
| } | ||
|
|
||
| if (scene["selected_object_id"]) { | ||
| loaded_selected_id = scene["selected_object_id"].as<GameObjectId>(); | ||
| } | ||
|
|
||
| if (!read_required_float(camera, "x", m_camera.x) || | ||
| !read_required_float(camera, "y", m_camera.y)) { | ||
| error = "camera is incomplete."; | ||
| return false; | ||
| } | ||
|
|
||
| if (!read_required_bool(grid, "show_grid", m_grid_options.show_grid) || | ||
| !read_required_bool(grid, "snap_to_grid", m_grid_options.snap_to_grid) || | ||
| !read_required_int(grid, "grid_size", m_grid_options.grid_size) || | ||
| !read_required_int(grid, "nudge_step", m_grid_options.nudge_step) || | ||
| !read_required_int(grid, "shift_nudge_steps", m_grid_options.shift_nudge_steps)) { | ||
| error = "grid is incomplete."; | ||
| return false; | ||
| } | ||
|
|
||
| if (!read_required_bool(options, "highlight_selected", m_scene_options.highlight_selected)) { | ||
| error = "options.highlight_selected is missing."; | ||
| return false; | ||
| } | ||
|
|
||
| int player_count = 0; | ||
|
|
||
| for (const auto& entry : objects) { | ||
| GameObject object{}; | ||
| if (!load_object_from_node(entry, object, error)) { | ||
| return false; | ||
| } | ||
|
|
||
| if (!m_objects.add_loaded_object(object)) { | ||
| error = "Failed to restore object. Duplicate or invalid id."; | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Make the load path transactional.
Right now you clear runtime state before the file has fully validated, then write camera/grid/options straight into the live scene as you parse. A malformed YAML can therefore wipe or partially replace the current scene and still return false, which is a nasty editor data-loss path. Parse into temporaries first, then commit once the whole file is known-good.
💡 Safer shape for the loader
- reset_runtime_state();
-
GameObjectId loaded_next_id = 1;
GameObjectId loaded_player_id = kInvalidGameObjectId;
GameObjectId loaded_selected_id = kInvalidGameObjectId;
+ decltype(m_camera) loaded_camera{};
+ decltype(m_grid_options) loaded_grid{};
+ decltype(m_scene_options) loaded_options{};
+ std::vector<GameObject> loaded_objects;
...
- if (!read_required_float(camera, "x", m_camera.x) ||
- !read_required_float(camera, "y", m_camera.y)) {
+ if (!read_required_float(camera, "x", loaded_camera.x) ||
+ !read_required_float(camera, "y", loaded_camera.y)) {
error = "camera is incomplete.";
return false;
}
...
- if (!read_required_bool(grid, "show_grid", m_grid_options.show_grid) ||
- !read_required_bool(grid, "snap_to_grid", m_grid_options.snap_to_grid) ||
- !read_required_int(grid, "grid_size", m_grid_options.grid_size) ||
- !read_required_int(grid, "nudge_step", m_grid_options.nudge_step) ||
- !read_required_int(grid, "shift_nudge_steps", m_grid_options.shift_nudge_steps)) {
+ if (!read_required_bool(grid, "show_grid", loaded_grid.show_grid) ||
+ !read_required_bool(grid, "snap_to_grid", loaded_grid.snap_to_grid) ||
+ !read_required_int(grid, "grid_size", loaded_grid.grid_size) ||
+ !read_required_int(grid, "nudge_step", loaded_grid.nudge_step) ||
+ !read_required_int(grid, "shift_nudge_steps", loaded_grid.shift_nudge_steps)) {
error = "grid is incomplete.";
return false;
}
...
- if (!m_objects.add_loaded_object(object)) {
- error = "Failed to restore object. Duplicate or invalid id.";
- return false;
- }
+ loaded_objects.push_back(object);
...
+ reset_runtime_state();
+ m_camera = loaded_camera;
+ m_grid_options = loaded_grid;
+ m_scene_options = loaded_options;
+ for (const auto& object : loaded_objects) {
+ if (!m_objects.add_loaded_object(object)) {
+ error = "Failed to restore object. Duplicate or invalid id.";
+ return false;
+ }
+ }🧰 Tools
🪛 Clang (14.0.6)
[warning] 211-211: variable 'loaded_next_id' is not initialized
(cppcoreguidelines-init-variables)
[warning] 212-212: variable 'loaded_player_id' is not initialized
(cppcoreguidelines-init-variables)
[warning] 213-213: variable 'loaded_selected_id' is not initialized
(cppcoreguidelines-init-variables)
| if (!read_required_uint(scene, "next_object_id", loaded_next_id)) { | ||
| error = "scene.next_object_id is missing."; | ||
| return false; | ||
| } | ||
|
|
||
| if (!read_required_uint(scene, "player_id", loaded_player_id)) { | ||
| error = "scene.player_id is missing."; | ||
| return false; | ||
| } | ||
|
|
||
| if (scene["selected_object_id"]) { | ||
| loaded_selected_id = scene["selected_object_id"].as<GameObjectId>(); | ||
| } | ||
|
|
||
| if (!read_required_float(camera, "x", m_camera.x) || | ||
| !read_required_float(camera, "y", m_camera.y)) { | ||
| error = "camera is incomplete."; | ||
| return false; | ||
| } | ||
|
|
||
| if (!read_required_bool(grid, "show_grid", m_grid_options.show_grid) || | ||
| !read_required_bool(grid, "snap_to_grid", m_grid_options.snap_to_grid) || | ||
| !read_required_int(grid, "grid_size", m_grid_options.grid_size) || | ||
| !read_required_int(grid, "nudge_step", m_grid_options.nudge_step) || | ||
| !read_required_int(grid, "shift_nudge_steps", m_grid_options.shift_nudge_steps)) { | ||
| error = "grid is incomplete."; | ||
| return false; | ||
| } | ||
|
|
||
| if (!read_required_bool(options, "highlight_selected", m_scene_options.highlight_selected)) { | ||
| error = "options.highlight_selected is missing."; | ||
| return false; | ||
| } | ||
|
|
||
| int player_count = 0; | ||
|
|
||
| for (const auto& entry : objects) { | ||
| GameObject object{}; | ||
| if (!load_object_from_node(entry, object, error)) { | ||
| return false; | ||
| } | ||
|
|
||
| if (!m_objects.add_loaded_object(object)) { | ||
| error = "Failed to restore object. Duplicate or invalid id."; | ||
| return false; | ||
| } | ||
|
|
||
| if (object.is_player) { | ||
| ++player_count; | ||
| } | ||
| } | ||
|
|
||
| if (m_objects.empty()) { | ||
| error = "Save file contains no objects."; | ||
| return false; | ||
| } | ||
|
|
||
| if (player_count != 1) { | ||
| error = "Save file must contain exactly one player object."; | ||
| return false; | ||
| } | ||
|
|
||
| m_player_id = loaded_player_id; | ||
| GameObject* player = m_objects.get_by_id(m_player_id); | ||
|
|
||
| if (!player) { | ||
| error = "Saved player_id does not exist."; | ||
| return false; | ||
| } | ||
|
|
||
| if (!player->is_player) { | ||
| error = "Saved player_id does not point to a player object."; | ||
| return false; | ||
| } | ||
|
|
||
| m_objects.set_next_id(loaded_next_id); |
There was a problem hiding this comment.
Reject next_object_id values that move backwards.
The loader trusts scene.next_object_id blindly. If a hand-edited file sets it to an existing ID, or to the invalid sentinel, the next spawned object can collide with an already loaded one and break the manager later. Sanity-check it against the highest loaded object ID before calling set_next_id().
💡 Validate the loaded sequence point
int player_count = 0;
+ GameObjectId max_loaded_id = kInvalidGameObjectId;
for (const auto& entry : objects) {
GameObject object{};
if (!load_object_from_node(entry, object, error)) {
return false;
@@
if (object.is_player) {
++player_count;
}
+
+ if (object.id > max_loaded_id) {
+ max_loaded_id = object.id;
+ }
}
@@
+ if (loaded_next_id == kInvalidGameObjectId || loaded_next_id <= max_loaded_id) {
+ error = "scene.next_object_id must be greater than every loaded object id.";
+ return false;
+ }
+
m_objects.set_next_id(loaded_next_id);🧰 Tools
🪛 Clang (14.0.6)
[warning] 278-278: variable 'player' is not initialized
(cppcoreguidelines-init-variables)
[warning] 280-280: implicit conversion 'prune::GameObject *' -> bool
(readability-implicit-bool-conversion)
| if (scene["selected_object_id"]) { | ||
| loaded_selected_id = scene["selected_object_id"].as<GameObjectId>(); | ||
| } | ||
|
|
||
| if (!read_required_float(camera, "x", m_camera.x) || | ||
| !read_required_float(camera, "y", m_camera.y)) { | ||
| error = "camera is incomplete."; | ||
| return false; | ||
| } | ||
|
|
||
| if (!read_required_bool(grid, "show_grid", m_grid_options.show_grid) || | ||
| !read_required_bool(grid, "snap_to_grid", m_grid_options.snap_to_grid) || | ||
| !read_required_int(grid, "grid_size", m_grid_options.grid_size) || | ||
| !read_required_int(grid, "nudge_step", m_grid_options.nudge_step) || | ||
| !read_required_int(grid, "shift_nudge_steps", m_grid_options.shift_nudge_steps)) { | ||
| error = "grid is incomplete."; | ||
| return false; | ||
| } | ||
|
|
||
| if (!read_required_bool(options, "highlight_selected", m_scene_options.highlight_selected)) { | ||
| error = "options.highlight_selected is missing."; | ||
| return false; | ||
| } | ||
|
|
||
| int player_count = 0; | ||
|
|
||
| for (const auto& entry : objects) { | ||
| GameObject object{}; | ||
| if (!load_object_from_node(entry, object, error)) { | ||
| return false; | ||
| } | ||
|
|
||
| if (!m_objects.add_loaded_object(object)) { | ||
| error = "Failed to restore object. Duplicate or invalid id."; | ||
| return false; | ||
| } | ||
|
|
||
| if (object.is_player) { | ||
| ++player_count; | ||
| } | ||
| } | ||
|
|
||
| if (m_objects.empty()) { | ||
| error = "Save file contains no objects."; | ||
| return false; | ||
| } | ||
|
|
||
| if (player_count != 1) { | ||
| error = "Save file must contain exactly one player object."; | ||
| return false; | ||
| } | ||
|
|
||
| m_player_id = loaded_player_id; | ||
| GameObject* player = m_objects.get_by_id(m_player_id); | ||
|
|
||
| if (!player) { | ||
| error = "Saved player_id does not exist."; | ||
| return false; | ||
| } | ||
|
|
||
| if (!player->is_player) { | ||
| error = "Saved player_id does not point to a player object."; | ||
| return false; | ||
| } | ||
|
|
||
| m_objects.set_next_id(loaded_next_id); | ||
|
|
||
| if (loaded_selected_id != kInvalidGameObjectId && | ||
| m_objects.get_by_id(loaded_selected_id) != nullptr) { | ||
| m_objects.set_selected_id(loaded_selected_id); | ||
| } | ||
| else { | ||
| m_objects.set_selected_id(m_player_id); | ||
| } |
There was a problem hiding this comment.
Preserve “no selection” on round-trip.
Save omits selected_object_id when nothing is selected, but load currently falls back to the player. That means a valid save/load cycle changes editor state instead of restoring it. Only fall back when the file carries a stale ID; if the key is absent, clear selection explicitly.
💡 Restore the optional field faithfully
- if (loaded_selected_id != kInvalidGameObjectId &&
- m_objects.get_by_id(loaded_selected_id) != nullptr) {
+ if (!scene["selected_object_id"]) {
+ m_objects.set_selected_id(kInvalidGameObjectId);
+ }
+ else if (loaded_selected_id != kInvalidGameObjectId &&
+ m_objects.get_by_id(loaded_selected_id) != nullptr) {
m_objects.set_selected_id(loaded_selected_id);
}
else {
m_objects.set_selected_id(m_player_id);
}📝 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.
| if (scene["selected_object_id"]) { | |
| loaded_selected_id = scene["selected_object_id"].as<GameObjectId>(); | |
| } | |
| if (!read_required_float(camera, "x", m_camera.x) || | |
| !read_required_float(camera, "y", m_camera.y)) { | |
| error = "camera is incomplete."; | |
| return false; | |
| } | |
| if (!read_required_bool(grid, "show_grid", m_grid_options.show_grid) || | |
| !read_required_bool(grid, "snap_to_grid", m_grid_options.snap_to_grid) || | |
| !read_required_int(grid, "grid_size", m_grid_options.grid_size) || | |
| !read_required_int(grid, "nudge_step", m_grid_options.nudge_step) || | |
| !read_required_int(grid, "shift_nudge_steps", m_grid_options.shift_nudge_steps)) { | |
| error = "grid is incomplete."; | |
| return false; | |
| } | |
| if (!read_required_bool(options, "highlight_selected", m_scene_options.highlight_selected)) { | |
| error = "options.highlight_selected is missing."; | |
| return false; | |
| } | |
| int player_count = 0; | |
| for (const auto& entry : objects) { | |
| GameObject object{}; | |
| if (!load_object_from_node(entry, object, error)) { | |
| return false; | |
| } | |
| if (!m_objects.add_loaded_object(object)) { | |
| error = "Failed to restore object. Duplicate or invalid id."; | |
| return false; | |
| } | |
| if (object.is_player) { | |
| ++player_count; | |
| } | |
| } | |
| if (m_objects.empty()) { | |
| error = "Save file contains no objects."; | |
| return false; | |
| } | |
| if (player_count != 1) { | |
| error = "Save file must contain exactly one player object."; | |
| return false; | |
| } | |
| m_player_id = loaded_player_id; | |
| GameObject* player = m_objects.get_by_id(m_player_id); | |
| if (!player) { | |
| error = "Saved player_id does not exist."; | |
| return false; | |
| } | |
| if (!player->is_player) { | |
| error = "Saved player_id does not point to a player object."; | |
| return false; | |
| } | |
| m_objects.set_next_id(loaded_next_id); | |
| if (loaded_selected_id != kInvalidGameObjectId && | |
| m_objects.get_by_id(loaded_selected_id) != nullptr) { | |
| m_objects.set_selected_id(loaded_selected_id); | |
| } | |
| else { | |
| m_objects.set_selected_id(m_player_id); | |
| } | |
| if (scene["selected_object_id"]) { | |
| loaded_selected_id = scene["selected_object_id"].as<GameObjectId>(); | |
| } | |
| if (!read_required_float(camera, "x", m_camera.x) || | |
| !read_required_float(camera, "y", m_camera.y)) { | |
| error = "camera is incomplete."; | |
| return false; | |
| } | |
| if (!read_required_bool(grid, "show_grid", m_grid_options.show_grid) || | |
| !read_required_bool(grid, "snap_to_grid", m_grid_options.snap_to_grid) || | |
| !read_required_int(grid, "grid_size", m_grid_options.grid_size) || | |
| !read_required_int(grid, "nudge_step", m_grid_options.nudge_step) || | |
| !read_required_int(grid, "shift_nudge_steps", m_grid_options.shift_nudge_steps)) { | |
| error = "grid is incomplete."; | |
| return false; | |
| } | |
| if (!read_required_bool(options, "highlight_selected", m_scene_options.highlight_selected)) { | |
| error = "options.highlight_selected is missing."; | |
| return false; | |
| } | |
| int player_count = 0; | |
| for (const auto& entry : objects) { | |
| GameObject object{}; | |
| if (!load_object_from_node(entry, object, error)) { | |
| return false; | |
| } | |
| if (!m_objects.add_loaded_object(object)) { | |
| error = "Failed to restore object. Duplicate or invalid id."; | |
| return false; | |
| } | |
| if (object.is_player) { | |
| +player_count; | |
| } | |
| } | |
| if (m_objects.empty()) { | |
| error = "Save file contains no objects."; | |
| return false; | |
| } | |
| if (player_count != 1) { | |
| error = "Save file must contain exactly one player object."; | |
| return false; | |
| } | |
| m_player_id = loaded_player_id; | |
| GameObject* player = m_objects.get_by_id(m_player_id); | |
| if (!player) { | |
| error = "Saved player_id does not exist."; | |
| return false; | |
| } | |
| if (!player->is_player) { | |
| error = "Saved player_id does not point to a player object."; | |
| return false; | |
| } | |
| m_objects.set_next_id(loaded_next_id); | |
| if (!scene["selected_object_id"]) { | |
| m_objects.set_selected_id(kInvalidGameObjectId); | |
| } | |
| else if (loaded_selected_id != kInvalidGameObjectId && | |
| m_objects.get_by_id(loaded_selected_id) != nullptr) { | |
| m_objects.set_selected_id(loaded_selected_id); | |
| } | |
| else { | |
| m_objects.set_selected_id(m_player_id); | |
| } |
🧰 Tools
🪛 Clang (14.0.6)
[warning] 278-278: variable 'player' is not initialized
(cppcoreguidelines-init-variables)
[warning] 280-280: implicit conversion 'prune::GameObject *' -> bool
(readability-implicit-bool-conversion)
[warning] 292-292: if with identical then and else branches
(bugprone-branch-clone)
[note] 296-296: else branch starts here
(clang)
| [[nodiscard]] bool save_to_file(std::string_view path, std::string& error) const; | ||
| [[nodiscard]] bool load_from_file(std::string_view path, std::string& error); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify string types are used and whether matching standard headers are present.
rg -n 'std::string_view|std::string|#include <string>|#include <string_view>' src/prune/scene/sandbox_scene.hppRepository: deanblackborough/Prune
Length of output: 247
🏁 Script executed:
#!/bin/bash
# Get the complete include section at the top of the file
head -50 src/prune/scene/sandbox_scene.hpp | cat -nRepository: deanblackborough/Prune
Length of output: 1626
Right, listen—this header needs to be self-contained. Always.
You're declaring methods that use std::string_view and std::string on lines 47-48, but you haven't included the headers for those types. Yes, it might compile because something else is pulling those in transitively (perhaps SDL2 or one of your project headers), but relying on that is a recipe for brittle code. In a game engine, these kinds of hidden dependencies will bite you later when you refactor includes.
Add #include <string> and #include <string_view> explicitly at the top of this file. Header files should always be self-sufficient—make it a habit.
Suggested fix
`#include` <array>
+#include <string>
+#include <string_view>🧰 Tools
🪛 Clang (14.0.6)
[warning] 47-47: use a trailing return type for this function
(modernize-use-trailing-return-type)
[warning] 48-48: use a trailing return type for this function
(modernize-use-trailing-return-type)
| if (!m_file_status.empty()) { | ||
| ImGui::Separator(); | ||
| if (m_file_status_is_error) { | ||
| ImGui::TextWrapped("Status: %s", m_file_status.c_str()); | ||
| } | ||
| else { | ||
| ImGui::TextWrapped("Status: %s", m_file_status.c_str()); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect the status-rendering branch to confirm both branches are identical.
rg -n -A8 -B3 'if \(m_file_status_is_error\)' src/prune/tooling/ui.cppRepository: deanblackborough/Prune
Length of output: 524
The m_file_status_is_error branch isn't doing anything useful right now.
You've got identical code in both the if and else branches—lines 49 and 52 render exactly the same thing. So the error flag isn't actually having any effect. This looks like an incomplete feature: you probably wanted to apply different styling (like a warning colour or icon) when there's an error, but left it unfinished.
Either wire up the error styling properly, or just collapse it into a single ImGui::TextWrapped() call for now to keep the code clean.
Quick tidy-up
if (!m_file_status.empty()) {
ImGui::Separator();
- if (m_file_status_is_error) {
- ImGui::TextWrapped("Status: %s", m_file_status.c_str());
- }
- else {
- ImGui::TextWrapped("Status: %s", m_file_status.c_str());
- }
+ ImGui::TextWrapped("Status: %s", m_file_status.c_str());
}📝 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.
| if (!m_file_status.empty()) { | |
| ImGui::Separator(); | |
| if (m_file_status_is_error) { | |
| ImGui::TextWrapped("Status: %s", m_file_status.c_str()); | |
| } | |
| else { | |
| ImGui::TextWrapped("Status: %s", m_file_status.c_str()); | |
| } | |
| if (!m_file_status.empty()) { | |
| ImGui::Separator(); | |
| ImGui::TextWrapped("Status: %s", m_file_status.c_str()); | |
| } |
🧰 Tools
🪛 Clang (14.0.6)
[note] 46-46: +3, including nesting penalty of 2, nesting level increased to 3
(clang)
[note] 48-48: +4, including nesting penalty of 3, nesting level increased to 4
(clang)
[note] 51-51: +1, nesting level increased to 4
(clang)
[warning] 48-48: if with identical then and else branches
(bugprone-branch-clone)
[note] 51-51: else branch starts here
(clang)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
vcpkg.json (1)
4-4:⚠️ Potential issue | 🔴 CriticalPin
builtin-baselineto a real vcpkg commit SHA.Line 4 is currently a build-breaker:
"latest"is not a valid baseline value for manifest mode, and your pipeline failure confirms it. Treat this like pinning asset versions in an engine build—always deterministic.Suggested fix
- "builtin-baseline": "latest", + "builtin-baseline": "<40-char-vcpkg-commit-sha>",#!/bin/bash set -euo pipefail # Verify builtin-baseline is pinned to a 40-char lowercase hex SHA. python - <<'PY' import json, re, sys with open("vcpkg.json", "r", encoding="utf-8") as f: data = json.load(f) baseline = data.get("builtin-baseline", "") print("builtin-baseline:", baseline) ok = bool(re.fullmatch(r"[0-9a-f]{40}", baseline)) print("valid_sha:", ok) sys.exit(0 if ok else 1) PY
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 143693af-8530-4e72-aeec-8d4ecaf48d57
📒 Files selected for processing (2)
.github/workflows/build.ymlvcpkg.json
| configurePreset: 'default' | ||
| buildPreset: 'default' |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate CMakePresets.json (case-insensitive search).
fd -i '^CMakePresets\.json$'
# Validate presence of required preset names.
python - <<'PY'
import json, pathlib, sys
p = pathlib.Path("CMakePresets.json")
if not p.exists():
print("Missing CMakePresets.json at repository root")
sys.exit(1)
data = json.loads(p.read_text(encoding="utf-8"))
cfg = {x.get("name") for x in data.get("configurePresets", [])}
bld = {x.get("name") for x in data.get("buildPresets", [])}
print("configurePresets:", sorted(cfg))
print("buildPresets:", sorted(bld))
missing = []
if "default" not in cfg:
missing.append("configurePresets.default")
if "default" not in bld:
missing.append("buildPresets.default")
if missing:
print("Missing required presets:", ", ".join(missing))
sys.exit(1)
PYRepository: deanblackborough/Prune
Length of output: 110
Create CMakePresets.json with the default presets before merging.
The workflow references configurePresets.default and buildPresets.default, but the CMakePresets.json file doesn't exist at the repository root. Without it, your build pipeline will fail on the next run. You'll need to create the file and define both presets there—think of it like setting up your engine's build configurations before you can actually build anything.
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: 39d6c51b-07ad-4e05-98a5-e2e662630ee6
📒 Files selected for processing (2)
.github/workflows/build.ymlvcpkg.json
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
.github/workflows/build.yml (1)
17-18:⚠️ Potential issue | 🟠 MajorPin
lukka/run-vcpkgto an immutable commit SHA.Good move adding vcpkg, but Line 18 still uses a floating ref (
@v11). For deterministic and safer CI, pin to a full commit SHA.Suggested change
- - name: Set up vcpkg - uses: lukka/run-vcpkg@v11 + - name: Set up vcpkg + uses: lukka/run-vcpkg@<full-commit-sha>What is the full commit SHA for the latest stable lukka/run-vcpkg v11.x release tag?
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 94a3e545-e968-4602-b48a-69b0b10044ba
📒 Files selected for processing (2)
.github/workflows/build.ymlvcpkg.json
| path: | | ||
| build/Release/Prune.exe | ||
| build/Prune.exe | ||
| path: build/Release/Prune.exe |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check whether workflow/cmake config indicates static linking or explicit DLL staging.
rg -n "VCPKG_TARGET_TRIPLET|x64-windows-static|CMAKE_TOOLCHAIN_FILE|upload-artifact|\\.dll|POST_BUILD|install\\(" .github/workflows/build.yml CMakeLists.txtRepository: deanblackborough/Prune
Length of output: 314
🏁 Script executed:
# Get the full build workflow around the artifact upload section
cat -n .github/workflows/build.yml | sed -n '20,40p'Repository: deanblackborough/Prune
Length of output: 667
🏁 Script executed:
# Check for vcpkg manifest and triplet configuration
fd -e json -e txt | grep -E "vcpkg|triplet" | head -20Repository: deanblackborough/Prune
Length of output: 76
🏁 Script executed:
# Examine CMakeLists.txt for dependency handling and installation
cat -n CMakeLists.txt | head -60Repository: deanblackborough/Prune
Length of output: 2127
🏁 Script executed:
# Check if there's any post-build script or explicit DLL copy logic
rg -n "add_custom_command|file\(COPY|install\(|dll|DLL" CMakeLists.txt -A 2Repository: deanblackborough/Prune
Length of output: 48
🏁 Script executed:
# Check vcpkg.json for triplet specification and dependency details
cat vcpkg.jsonRepository: deanblackborough/Prune
Length of output: 161
Runtime DLLs missing from artifact upload.
When vcpkg builds SDL2 and yaml-cpp without an explicit static triplet like x64-windows-static, they compile as dynamic libraries. Your current workflow uploads only Prune.exe, which means anyone downloading the artifact on a clean Windows machine will hit a runtime crash looking for SDL2.dll and yaml-cpp.dll.
Since your vcpkg.json doesn't specify a triplet and your CMake configuration doesn't override it, the Windows runner defaults to dynamic linking. You need to bundle those runtime DLLs with the executable. Either adjust your artifact path to include them:
Suggested fix
- name: Upload artifact
uses: actions/upload-artifact@v4
with:
name: prune-windows
- path: build/Release/Prune.exe
+ path: |
+ build/Release/Prune.exe
+ build/Release/*.dll
retention-days: 30Alternatively, lock the vcpkg triplet to x64-windows-static for fully self-contained binaries.
deanblackborough
dismissed
coderabbitai[bot]’s stale review
This is all initial save and load and it is known to be bad, it wll however do for now
1 participant
Summary by CodeRabbit
New Features
UI
Documentation
Chores