Implament Lua as a scripting language #14
Conversation
|
Warning Rate limit exceeded@arozx has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 22 minutes and 11 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughThis pull request introduces a comprehensive set of changes to the VoxelEngine project, focusing on enhancing scripting capabilities, scene management, and development workflows. The primary additions include Lua scripting integration, a new scene management system, improved input handling, and various configuration files for development environments. The changes span multiple components of the engine, introducing new APIs, refactoring existing systems, and adding configuration support for different development tools. Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
♻️ Duplicate comments (1)
src/Renderer/Renderer.h (1)
87-90: 🛠️ Refactor suggestionRemove redundant method declaration.
This declaration corresponds to the redundant
Render()method implementation. As suggested earlier, consider removing this method to avoid API confusion with the existingDraw()method.- /** - * @brief Trigger a render operation by flushing the command queue - */ - void Render();
🧹 Nitpick comments (30)
src/Scene/Scene.cpp (3)
12-15: Consider validating child uniqueness.
Adding a child that is already in the hierarchy could cause circular references or repeated registrations. Optionally, check if the child already has a parent to avoid logical errors.
17-23: Consider caching or lazy-updating transformation.
Repeatedly callingGetWorldTransform()in a deep hierarchy could become expensive. A lazy-updated or cached world matrix can improve performance, especially when transformations do not change frequently.
27-31: Use an initialization list for members.
According to static analysis hints,m_RootObjectandm_TerrainSystemcan be initialized in the member initializer list to improve performance.- Scene::Scene(const std::string &name) : m_Name(name) { - m_RootObject = std::make_shared<SceneObject>("Root"); - m_TerrainSystem = std::make_unique<TerrainSystem>(); - LOG_TRACE("Created scene: ", name); - } + Scene::Scene(const std::string &name) + : m_Name(name), + m_RootObject(std::make_shared<SceneObject>("Root")), + m_TerrainSystem(std::make_unique<TerrainSystem>()) { + LOG_TRACE("Created scene: ", name); + }🧰 Tools
🪛 Cppcheck (2.10-2)
[performance] 28-28: Variable 'm_RootObject' is assigned in constructor body. Consider performing initialization in initialization list.
(useInitializationList)
[performance] 29-29: Variable 'm_TerrainSystem' is assigned in constructor body. Consider performing initialization in initialization list.
(useInitializationList)
src/Application.h (3)
67-68: Document ownership implications of returning a raw pointer.
CallingGetScriptSystem()returns a raw pointer, potentially allowing unguarded usage. Ensure the caller understands that theLuaScriptSystemis owned bym_ScriptSystem.
70-70: Similar pointer ownership concern forGetInputSystem().
Return value is also a raw pointer. Ifm_InputSystemis destroyed, the pointer becomes invalid. Consider returning a reference or smart pointer for safer ownership semantics.
189-194: Ensure singleton pattern is carefully controlled.
StoringApplication* s_Instancecan lead to issues if multipleApplicationinstances are constructed inadvertently. Make sure your design enforces single-instance usage.sandbox/src/SandboxApp.cpp (3)
64-78: Expand history logic error handling.
Filtering out commands that contain"Error"on up-arrow retrieval is a bit fragile. Consider tracking command success/failure separately for more robust user experience.
103-111: Limit memory churn in multiline console output.
consoleOutput.clear()is rebuilt each frame. If the command history grows large, performance could degrade. Caching or streaming might be beneficial.
120-141: Consider more detailed error reporting.
ExecuteCommandlogs a single error message for failures. You could add stack traces or highlight syntax errors for improved debugging.json-viewer/visualiser.ipynb (2)
44-51: Gracefully handle functions not in thefunction_order.Currently, if a profile name is missing from
function_order, its sorting order stays asNaN. Ensure that unexpected function names won't break or skew the display.
127-127: Verify the log scale usage.Using
yaxis_type='log'helps visualize wide time ranges, but small values may be harder to see. Confirm this is the intended display.src/Application.cpp (1)
105-108: Catchingstd::exceptionis good, but consider specialized exceptions.Lua errors might not always inherit from
std::exception. Optionally, expand coverage for unexpected errors.src/Scripting/LuaScriptSystem.cpp (2)
13-16: Restrict Libraries to Mitigate Security Risks
Currently, all libraries (includingos,io, etc.) are opened in the Lua state, potentially exposing system-level commands to scripts. Consider only enabling the libraries you actually need (e.g.,base,math) to reduce the attack surface.
326-355: Log Detailed Error Messages When Loading Script Files Fails
The script loading error reporting at lines 349-352 is limited. You can capture and log the specific Lua error message to help users debug script-related issues.For example:
sol::load_result loadResult = m_LuaState->load_file(validPath); if (!loadResult.valid()) { - LOG_ERROR("Failed to execute script: ", validPath); + sol::error err = loadResult; + LOG_ERROR_CONCAT("Failed to load file: ", validPath, " => ", err.what()); return false; } sol::protected_function_result result = loadResult(); if (!result.valid()) { - LOG_ERROR("Failed to execute script: ", validPath); + sol::error err = result; + LOG_ERROR_CONCAT("Failed to execute script: ", validPath, " => ", err.what()); return false; }src/Core/FileSystem.h (1)
8-10: Consider Handling std::filesystem Exceptions
Although unlikely in most cases,std::filesystem::existsmay throw under certain conditions (e.g., insufficient permissions, corrupted paths). Surrounding it with atry/catchblock can prevent crashes in production, though this may be optional based on your engine’s requirements.static bool Exists(const std::string& filepath) { - return std::filesystem::exists(filepath); + try { + return std::filesystem::exists(filepath); + } catch (const std::filesystem::filesystem_error& e) { + LOG_ERROR_CONCAT("Filesystem error checking path: ", filepath, " => ", e.what()); + return false; + } }sandbox/src/SandboxApp.h (1)
14-17: Add Constraints or Capacity for the Command History
Becausem_CommandHistorycan grow unbounded if commands are continuously entered, consider imposing a maximum size or clearing old entries to optimize memory usage.sandbox/assets/scripts/main.lua (2)
5-13: Move debug configuration to a separate file.The debugging configuration should be moved to a separate configuration file for better maintainability.
Consider creating a
config.luafile:-- config.lua return { debug = { enabled = true, showFPS = true, showRenderer = true, showTerrain = true } }
28-33: Extract camera configuration to a separate function.The camera setup uses hard-coded values and should be moved to a configuration file or function.
Consider creating a separate function:
local function setupCamera() -- Load camera config from a file or use defaults local config = { position = {x = 0, y = 10, z = 10}, rotation = {pitch = -45, yaw = 0}, clearColor = {r = 0.2, g = 0.3, b = 0.3, a = 1.0} } engine.setCameraType("perspective") engine.setCameraPosition(config.position.x, config.position.y, config.position.z) engine.setCameraRotation(config.rotation.pitch, config.rotation.yaw) engine.setClearColor(config.clearColor.r, config.clearColor.g, config.clearColor.b, config.clearColor.a) endsrc/UI/ImGuiOverlay.h (2)
52-53: Consider dependency injection for TerrainSystem.The removal of the
TerrainSystemparameter suggests internal retrieval, which could make testing and mocking more difficult. Consider keeping the parameter for better dependency management.
68-72: Consider grouping related visibility flags.The visibility flags could be grouped into a struct for better organization, especially if more UI elements are added in the future.
- bool m_ShowTransformControls = true; - bool m_ShowProfiler = true; - bool m_ShowRendererSettings = true; - bool m_ShowTerrainControls = true; - bool m_ShowFPSCounter = true; + struct UIVisibilityFlags { + bool showTransformControls = true; + bool showProfiler = true; + bool showRendererSettings = true; + bool showTerrainControls = true; + bool showFPSCounter = true; + } m_UIFlags;src/Scene/Scene.h (1)
87-91: Consider lazy initialization for TerrainSystem.The TerrainSystem is always created but might not always be needed. Consider lazy initialization to improve resource usage.
-TerrainSystem* GetTerrainSystem() const { return m_TerrainSystem.get(); } +TerrainSystem* GetTerrainSystem() { + if (!m_TerrainSystem) { + m_TerrainSystem = std::make_unique<TerrainSystem>(); + } + return m_TerrainSystem.get(); +}Also applies to: 104-104
src/Input/InputSystem.h (1)
78-80: Consider adding documentation for action parameter.The OnKey method should document the possible values for the action parameter (GLFW_PRESS, GLFW_RELEASE, GLFW_REPEAT).
-// Key event handling -void OnKey(int key, int scancode, int action, int mods); +/** + * @brief Handles key events + * @param key GLFW key code + * @param scancode Platform-specific scancode + * @param action GLFW_PRESS, GLFW_RELEASE, or GLFW_REPEAT + * @param mods Bit field of modifier keys + */ +void OnKey(int key, int scancode, int action, int mods);src/Core/Utils/Logging.h (1)
169-183: Add file and line information to logs.Consider enhancing the logging macros to include source location:
-#define LOG_TRACE_CONCAT(...) Engine::Logger::Get().LogConcat(Engine::LogLevel::Trace, __VA_ARGS__) +#define LOG_TRACE_CONCAT(...) Engine::Logger::Get().LogConcat(Engine::LogLevel::Trace, "[", __FILE__, ":", __LINE__, "] ", __VA_ARGS__)src/Input/InputSystem.cpp (1)
203-223: Consider enhancing the key callback system with additional safety features.The implementation is functional but could benefit from the following improvements:
- Add key code validation to prevent invalid registrations.
- Implement a mechanism to unregister callbacks.
- Add error handling for invalid callbacks.
- Consider thread safety if callbacks can be registered/executed from different threads.
Here's a suggested implementation:
+using KeyCallback = std::function<void(int)>; + +class KeyCallbackHandle { + friend class InputSystem; + KeyCallbackHandle(int key, size_t id) : key(key), id(id) {} + int key; + size_t id; +}; + +struct CallbackInfo { + size_t id; + KeyCallback callback; +}; + void InputSystem::RegisterKeyCallback(int key, KeyCallback callback) { + if (key < 0 || key > GLFW_KEY_LAST) { + LOG_ERROR("Invalid key code: {}", key); + return; + } + + if (!callback) { + LOG_ERROR("Invalid callback for key: {}", key); + return; + } + + static size_t nextId = 0; + std::lock_guard<std::mutex> lock(m_CallbackMutex); - m_KeyCallbacks[key] = std::move(callback); + m_KeyCallbacks[key].push_back({nextId++, std::move(callback)}); } +KeyCallbackHandle InputSystem::RegisterKeyCallback(int key, KeyCallback callback) { + auto id = RegisterCallbackImpl(key, std::move(callback)); + return KeyCallbackHandle(key, id); +} + +void InputSystem::UnregisterKeyCallback(const KeyCallbackHandle& handle) { + std::lock_guard<std::mutex> lock(m_CallbackMutex); + auto it = m_KeyCallbacks.find(handle.key); + if (it != m_KeyCallbacks.end()) { + auto& callbacks = it->second; + callbacks.erase( + std::remove_if(callbacks.begin(), callbacks.end(), + [&](const CallbackInfo& info) { return info.id == handle.id; }), + callbacks.end()); + } +} void InputSystem::OnKey(int key, int scancode, int action, int mods) { // Process any registered callbacks first + try { OnKeyEvent(key, action); + } catch (const std::exception& e) { + LOG_ERROR("Error executing key callback: {}", e.what()); + } // Handle built-in key processing if (key == GLFW_KEY_ESCAPE && action == GLFW_PRESS) { m_CursorLocked = !m_CursorLocked; UpdateCursorState(); } } void InputSystem::OnKeyEvent(int key, int action) { + std::lock_guard<std::mutex> lock(m_CallbackMutex); auto it = m_KeyCallbacks.find(key); if (it != m_KeyCallbacks.end()) { - it->second(action); + for (const auto& info : it->second) { + try { + info.callback(action); + } catch (const std::exception& e) { + LOG_ERROR("Error executing key callback {}: {}", info.id, e.what()); + } + } } }Also, add these member variables to the class:
private: std::mutex m_CallbackMutex; std::unordered_map<int, std::vector<CallbackInfo>> m_KeyCallbacks;sandbox/assets/scripts/engine.lua (1)
50-58: Enhance the KeyCode class with more key constants and improved documentation.The KeyCode class is missing many common key codes that would be useful for input handling. Consider expanding it to include:
- More letter keys (A-Z)
- Number keys (0-9)
- Function keys (F1-F12)
- Modifier keys (SHIFT, CTRL, ALT)
- Arrow keys
- Special keys (TAB, ENTER, BACKSPACE)
Here's a suggested implementation:
---@class KeyCode ---@field ESCAPE number # ESC key ---@field SPACE number # Spacebar - ---@field W number # W key ---@field A number # A key ---@field S number # S key ---@field D number # D key +---@field Q number # Q key +---@field E number # E key +---@field R number # R key +# ... Add more letter keys + +---@field NUM_0 number # Number key 0 +---@field NUM_1 number # Number key 1 +# ... Add more number keys + +---@field F1 number # Function key F1 +---@field F2 number # Function key F2 +# ... Add more function keys + +---@field LEFT_SHIFT number # Left shift key +---@field RIGHT_SHIFT number # Right shift key +---@field LEFT_CTRL number # Left control key +---@field RIGHT_CTRL number # Right control key +---@field LEFT_ALT number # Left alt key +---@field RIGHT_ALT number # Right alt key + +---@field LEFT number # Left arrow key +---@field RIGHT number # Right arrow key +---@field UP number # Up arrow key +---@field DOWN number # Down arrow key + +---@field TAB number # Tab key +---@field ENTER number # Enter key +---@field BACKSPACE number # Backspace key +---@field DELETE number # Delete key +---@field HOME number # Home key +---@field END number # End key +---@field PAGE_UP number # Page up key +---@field PAGE_DOWN number # Page down key KeyCode = {}src/UI/ImGuiOverlay.cpp (1)
266-303: Consider enhancing the terrain controls UI.The implementation could benefit from the following improvements:
- Initialize the seed with a random value for better user experience.
- Add a tooltip explaining the auto-update feature.
- Consider adding a randomize button for the seed.
Here's a suggested implementation:
-static uint32_t seed = 1234; +static uint32_t seed = static_cast<uint32_t>(std::random_device{}()); + +if (ImGui::Button("Randomize Seed")) { + seed = static_cast<uint32_t>(std::random_device{}()); + terrainSystem->RegenerateTerrain(seed); +} +ImGui::SameLine(); if (ImGui::Button("Regenerate Terrain")) { terrainSystem->RegenerateTerrain(seed); } // Terrain parameters ImGui::Separator(); ImGui::Text("Terrain Parameters:"); static float baseHeight = 32.0f; static float heightScale = 32.0f; static float noiseScale = 0.05f; static bool autoUpdate = false; +if (ImGui::IsItemHovered()) { + ImGui::SetTooltip("When enabled, changes to parameters will be applied immediately"); +} if (ImGui::SliderFloat("Base Height", &baseHeight, 0.0f, 64.0f) && autoUpdate) { terrainSystem->SetBaseHeight(baseHeight); }src/ImGui/ImGuiLayer.cpp (1)
49-49: Consider standardizing log message format.The log message has been updated to be more specific. Consider using a consistent format across all initialization messages in the codebase.
#!/bin/bash # Check for consistency in initialization log messages rg "initialized|initializing" -B 1 -A 1CMakeLists.txt (1)
88-88: Consider enhancing library configuration.While the configuration is functional, it could be more robust:
- Consider versioning sol2
- Be explicit about which Lua libraries are being linked
Apply this diff to improve the configuration:
- ${CMAKE_SOURCE_DIR}/external/sol2 + ${CMAKE_SOURCE_DIR}/external/sol2/include # Be specific about include path - ${LUA_LIBRARIES} # Add Lua libraries + ${LUA_LIBRARIES} # Lua core and standard libraries +if(WIN32) + target_compile_definitions(voxel-engine PUBLIC SOL_ALL_SAFETIES_ON=1) # Enable sol2 safety features +endif()Also applies to: 95-95
.idea/workspace.xml (1)
1-186: Consider excluding IDE configuration files.While sharing IDE settings can help maintain team consistency, it's generally recommended to keep IDE-specific files out of version control:
- They often contain user-specific paths
- They can cause conflicts between different IDE versions
- They may contain sensitive information
Add these entries to your .gitignore file:
+# IDE - JetBrains +.idea/ +*.iml +*.iws.idea/editor.xml (1)
1-244: Consider moving code style settings to clang-format.While the IDE settings are comprehensive, consider:
- Moving code style rules to .clang-format for wider tool support
- Using .editorconfig for basic editor settings
- Excluding IDE-specific settings from version control
Create a .editorconfig file with basic settings:
+# EditorConfig is awesome: https://EditorConfig.org +root = true + +[*] +end_of_line = lf +insert_final_newline = true +charset = utf-8 +trim_trailing_whitespace = true + +[*.{cpp,h,hpp}] +indent_style = space +indent_size = 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (51)
.clang-format(1 hunks).github/workflows/build.yml(1 hunks).gitignore(2 hunks).idea/.name(1 hunks).idea/editor.xml(1 hunks).idea/misc.xml(1 hunks).idea/modules.xml(1 hunks).idea/vcs.xml(1 hunks).idea/voxels.iml(1 hunks).idea/workspace.xml(1 hunks).vs/ProjectSettings.json(1 hunks).vs/VSWorkspaceState.json(1 hunks).vs/launch.vs.json(1 hunks)CMakeLists.txt(4 hunks)CMakeSettings.json(1 hunks)external/sol2/config.hpp(1 hunks)external/sol2/forward.hpp(1 hunks)imgui.ini(1 hunks)json-viewer/visualiser.ipynb(3 hunks)sandbox/.luarc.json(1 hunks)sandbox/assets/scripts/engine.lua(1 hunks)sandbox/assets/scripts/init.lua(1 hunks)sandbox/assets/scripts/main.lua(1 hunks)sandbox/src/SandboxApp.cpp(1 hunks)sandbox/src/SandboxApp.h(1 hunks)src/Application.cpp(7 hunks)src/Application.h(4 hunks)src/Core/FileSystem.h(1 hunks)src/Core/Utils/Logging.h(1 hunks)src/Debug/Profiler.cpp(6 hunks)src/Debug/Profiler.h(3 hunks)src/ImGui/ImGuiLayer.cpp(2 hunks)src/Input/InputSystem.cpp(1 hunks)src/Input/InputSystem.h(3 hunks)src/Renderer/Renderer.cpp(2 hunks)src/Renderer/Renderer.h(1 hunks)src/Scene/GameScene.h(0 hunks)src/Scene/Scene.cpp(1 hunks)src/Scene/Scene.h(2 hunks)src/Scene/SceneManager.h(1 hunks)src/Scripting/LuaScriptSystem.cpp(1 hunks)src/Scripting/LuaScriptSystem.h(1 hunks)src/TerrainSystem/TerrainSystem.cpp(3 hunks)src/TerrainSystem/TerrainSystem.h(2 hunks)src/UI/ImGuiOverlay.cpp(7 hunks)src/UI/ImGuiOverlay.h(1 hunks)src/Window/OpenGLWindow.cpp(1 hunks)src/pch.h(0 hunks)valgrind.supp(1 hunks)vcpkg-configuration.json(1 hunks)vcpkg.json(1 hunks)
💤 Files with no reviewable changes (2)
- src/pch.h
- src/Scene/GameScene.h
✅ Files skipped from review due to trivial changes (17)
- .vs/ProjectSettings.json
- .idea/misc.xml
- sandbox/.luarc.json
- vcpkg-configuration.json
- .idea/modules.xml
- .vs/VSWorkspaceState.json
- .idea/.name
- .clang-format
- vcpkg.json
- .vs/launch.vs.json
- external/sol2/config.hpp
- .idea/vcs.xml
- valgrind.supp
- CMakeSettings.json
- .gitignore
- .idea/voxels.iml
- external/sol2/forward.hpp
🧰 Additional context used
🪛 GitHub Actions: Build and Analyze
CMakeLists.txt
[error] 22-22: Could NOT find Lua (missing: LUA_LIBRARIES LUA_INCLUDE_DIR). The required Lua package dependencies are not installed or not found by CMake.
🪛 Cppcheck (2.10-2)
src/Scene/Scene.cpp
[performance] 28-28: Variable 'm_RootObject' is assigned in constructor body. Consider performing initialization in initialization list.
(useInitializationList)
[performance] 29-29: Variable 'm_TerrainSystem' is assigned in constructor body. Consider performing initialization in initialization list.
(useInitializationList)
🔇 Additional comments (74)
src/Scene/Scene.cpp (3)
33-47: Prefer returning an existing object when it has the same name.
If multiple scene objects can share the same name, consider whether you want to prevent duplicates. If this is intended, no changes needed. Otherwise, search for an object with the same name before creating a new one.
80-86: Double-check repeated on-demand initialization sequence.
InOnRender,OnCreateis called again ifm_Rendereris not set. Ensure that repeated initialization is intentional and doesn’t reset crucial state.
8-10: Confirm usage ofshared_from_this.
Ensure thatSceneObjectinherits fromstd::enable_shared_from_this<SceneObject>if you plan to useshared_from_this()in its methods.✅ Verification successful
shared_from_thisimplementation is correct
The class properly inherits fromstd::enable_shared_from_this<SceneObject>andshared_from_this()is correctly used in the scene graph implementation for parent-child relationships.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the class definition of SceneObject includes `std::enable_shared_from_this<SceneObject>`. rg --context 5 $'class SceneObject : public std::enable_shared_from_this<SceneObject>'Length of output: 861
Script:
#!/bin/bash # Search for usage of shared_from_this() in the codebase rg "shared_from_this\(\)" --type cppLength of output: 187
src/Application.h (1)
120-120: Confirm lifecycle management ofm_TerrainSystem.
m_TerrainSystemchanged from astd::unique_ptrto a raw pointer. Verify that its lifetime is managed safely elsewhere, or revert tostd::unique_ptrif ownership belongs here.sandbox/src/SandboxApp.cpp (1)
60-60: Guard against input truncation.
inputBufferhas a max size of 256. Notify users or handle large commands more gracefully if they exceed this limit.json-viewer/visualiser.ipynb (8)
31-34: Handle potential file-not-found errors when using relative paths.If these files do not exist in the parent directory, this will throw an unhandled exception. Consider adding try-except handling or checking if the paths exist before calling
open(...).
40-43: Check dictionary key usage for 'profiles'.If
profile_datalacks the'profiles'key, accessing it here could raise a KeyError. Consider usingprofile_data.get('profiles', [])to avoid runtime errors.
52-73: Ensure robust handling of missing metrics.You skip adding traces if
'minMs','averageMs', or'maxMs'are missing. That may be correct, but confirm that empty or unusual data sets (e.g., no'maxMs') are handled gracefully without errors.
134-135: Validate 'frame_time_ms' column presence.If
frames_dfis missing'frame_time_ms', this will raise a KeyError. Consider a safer approach with a conditional check.
140-143: Box plot points enabled.All data points and potential outliers are shown. The jitter is helpful for dense data, so this is a fine setting.
149-158: Check for out-of-range access in quartiles loop.When data is sparse,
quartilesmight not supply enough elements, leading to index errors. Ensure there's a minimum dataset size or add a guard clause.
165-174: Confirm assumptions when computing extended box plot range.If
quartilesis incomplete, indexingquartiles.iloc[1]orquartiles.iloc[4]can cause errors. Consider verifying data length to avoid bounds issues.
186-186: Presentation of final figure.Invoking
fig4.show()ensures complete data visualization flow. This logic looks fine.src/Application.cpp (14)
58-59: Static pointer initialization looks fine.
Application::s_Instanceis set tonullptr. This is a straightforward pattern for singletons.
65-66: Assignings_Instancein the constructor is valid.This ensures the singleton is accessible early for other subsystems.
67-67: Increased logging granularity to TRACE.Switching to
LOG_TRACEis helpful for debugging deeper initialization details.Also applies to: 79-80
81-82: Script system creation is required for app initialization.Early creation before other systems is logical. The error path is handled by returning immediately if the system fails to initialize.
Also applies to: 83-86
87-90: Wrap script system initialization in a try/catch block.This is good practice to handle Lua script loading issues without crashing the entire application.
91-97: Verify 'init.lua' path validity.Hardcoding "sandbox/assets/scripts/init.lua" might require adjusting if the directory structure changes. Confirm the path is correct at runtime.
98-104: Check environment assumptions for 'main.lua' in 'build/assets/scripts'.If the environment is different, this might fail. Validate that the file actually exists in the build output before attempting to execute it.
110-110: Initializing remaining systems post script system creation is consistent.This defers dependent systems until scripts are loaded, which can be beneficial if scripts configure engine behavior.
117-118: Explicitly resettingm_TerrainSystem.If you're planning to initialize or replace it later, this is harmless. Otherwise, consider removing the dead field if not used.
141-141: Duplicate profiling sessions?
Profiler::Get().BeginSession("Runtime")was also called in the constructor. Confirm we want to begin a new session here or if it's unintentional repetition.
171-171: Invoke ImGui rendering function.Enabling extra customization steps before presenting the frame is good. No issues spotted.
231-231: Logging window creation with TRACE priority.This matches the more verbose logging style used throughout. No further concerns.
270-271: Rendering throughSceneManagerinstead of direct terrain calls.Shifting terrain out of the main loop can improve maintainability by centralizing rendering in
SceneManager.
286-286: Expose terrain controls in overlay.Good approach for debug or editing features. No immediate concerns.
src/Debug/Profiler.cpp (14)
19-20: Use of thread-local variables for sample rate limiting.These additions should reduce the overhead of frequent profiling writes. Verify that concurrency on multiple threads is safe if multiple threads call
WriteProfile.
47-47: Profiling is disabled by default.Starting with
m_Enabled(false)is a cautious approach, forcing explicit activation when needed.
75-78: Locking onBeginSessionensures thread safety while setting up session data.Guaranteed consistency for
m_CurrentSessionand the JSON structure. No issues found.
79-85: Initialize JSON structure and handle file creation exceptions.Good practice to start with an empty JSON array and handle I/O failures gracefully. This flows well.
Also applies to: 86-93, 94-95, 96-96
123-124: Early return if profiling is disabled.Avoids unnecessary file writes in
WriteCompleteJSON.
126-127: Shared lock for reading profile data.Keeping concurrency overhead minimal while reading is sensible. Confirm usage across multiple writer threads is handled.
132-134: Constructing 'profiles' array and adding samples.Writing multiple samples for each profile is a standard approach. This is a solid pattern for performance analysis.
Also applies to: 135-146, 148-159, 160-161, 163-164
175-175: File output with truncation and error logging.Ensures a clean start for each write. Logging the error if the file cannot be opened is a good fallback.
Also applies to: 178-179, 180-181, 183-185, 187-187
243-243: Bailing out if profiling is disabled or name is empty.Prevents storing meaningless data for unnamed sections or when off. Looks correct.
245-250: Rate-limiting samples to 60Hz.Helps avoid clutter and large file sizes. This is well-chosen for performance overhead reduction.
253-259: Leverage shared lock for quick cache lookups.Reduces locking contention when checking existing profiles. This is a neat use of the fast path.
260-260: Allocate new profile data on first usage.The fallback to a unique lock to create or retrieve the profile is well-structured for concurrency.
Also applies to: 264-269
273-273: Add sample and mark unsaved data.Correctly ensures subsequent flush or JSON write includes the new data.
276-279: Frequent auto-writes for development mode.Every 500ms might be heavy in production but is understandable for debugging or game dev.
src/Scripting/LuaScriptSystem.cpp (1)
308-324: Good Error Handling When Executing Lua Script Strings
TheExecuteScriptmethod provides explicit error logging and returns a boolean to indicate success or failure, which is a solid approach. Looks good.sandbox/src/SandboxApp.h (1)
19-20: Review Console Input Handling for Edge Cases
EnsureExecuteCommandandHandleConsoleInputhandle potential corner cases, such as blank input, trailing whitespace, or extremely long commands. Input sanitization or length checks can help avoid unexpected behavior.src/Scripting/LuaScriptSystem.h (1)
17-19: Consider adding script validation and sandboxing.The script execution methods should include validation to prevent malicious code execution.
Consider:
- Adding a method to validate scripts before execution
- Implementing a sandbox environment for script execution
- Adding rate limiting for script execution
src/UI/ImGuiOverlay.h (1)
54-60: LGTM! Well-structured visibility controls.The visibility control methods follow a consistent pattern and are well-organized. This will be useful for Lua scripting to toggle UI elements.
src/TerrainSystem/TerrainSystem.h (2)
76-89: LGTM! Well-organized private members.The private members are well-documented and logically organized. The configuration parameters will be useful for Lua scripting integration.
4-13:⚠️ Potential issueRemove redundant include of Renderer.h.
The Renderer.h header is included twice (lines 12 and 13). Remove the redundant include.
-#include "Renderer/Renderer.h"Likely invalid or redundant comment.
src/Scene/Scene.h (2)
12-44: LGTM! Well-designed scene object hierarchy.The SceneObject class is well-structured with:
- Smart pointer usage for memory safety
- Clear parent-child relationships
- Transform hierarchy support
This will integrate well with Lua scripting for scene manipulation.
62-75: 🛠️ Refactor suggestionConsider adding error handling for lifecycle methods.
The virtual lifecycle methods should include error handling to catch and report Lua script errors.
-virtual void OnCreate(); +virtual void OnCreate() { + try { + // Implementation + } catch (const std::exception& e) { + // Log error and handle gracefully + } +}Likely invalid or redundant comment.
src/Input/InputSystem.h (2)
74-76: LGTM! Well-designed callback system.The key callback system using std::function enables flexible integration with Lua scripts.
142-145: Consider thread safety for callback map.The callback map could be accessed from multiple threads if Lua scripts run asynchronously. Consider adding mutex protection.
src/TerrainSystem/TerrainSystem.cpp (2)
55-58: LGTM! Improved logging level.Good change moving debug information from INFO to TRACE level, with a static flag to prevent log spam.
137-138: LGTM! Consistent logging level change.Appropriate change to use TRACE level for mesh generation statistics.
src/Renderer/Renderer.cpp (1)
2-6: LGTM! Well-organized includes.Good organization of includes: system headers first, followed by project headers.
src/Core/Utils/Logging.h (2)
13-19: LGTM! Well-structured logging enhancements.Good implementation of LogLevel enum with clear documentation and comprehensive ToString specializations, including smart pointer support.
Also applies to: 28-50
98-131: LGTM! Comprehensive logging implementation.Well-implemented logging system with:
- Proper timestamp formatting
- Color-coded log levels
- Structured logging capabilities via LogValue methods
Also applies to: 140-158
src/Window/OpenGLWindow.cpp (1)
2-7: LGTM!The include directives are well-organized with system headers first, followed by project headers, and appropriate blank lines for improved readability.
sandbox/assets/scripts/engine.lua (2)
1-48: LGTM! Well-documented API with comprehensive type information.The engine class definition is well-organized and thoroughly documented using EmmyLua annotations, making it easy for developers to understand the available functionality.
60-248: LGTM! Comprehensive function definitions with clear documentation.The engine functions are well-organized by category (terrain, input, logging, profiling, camera, UI) with clear parameter descriptions and appropriate type annotations.
src/UI/ImGuiOverlay.cpp (6)
36-36: LGTM! Efficient early return for hidden FPS counter.The visibility check prevents unnecessary rendering when the FPS counter is disabled.
128-128: LGTM! Efficient early return for hidden transform controls.The visibility check prevents unnecessary rendering when transform controls are disabled.
143-143: LGTM! Efficient early return for hidden profiler.The visibility check prevents unnecessary rendering when the profiler is disabled.
202-202: LGTM! Efficient early return for hidden renderer settings.The visibility check prevents unnecessary rendering when renderer settings are disabled.
222-222: LGTM! Efficient early return for hidden event debugger.The visibility check prevents unnecessary rendering when the event debugger is disabled.
258-264: LGTM! Robust scene and terrain system validation.The method now correctly validates both the active scene and terrain system before proceeding with rendering.
src/Debug/Profiler.h (3)
133-136: LGTM! The IsInitialized check is well-implemented.The method provides a clear way to verify the profiler's state by checking both the enabled flag and session existence.
297-310: LGTM! The operator[] overloads are well-implemented.The implementation correctly handles both inline and heap storage cases, maintaining the small vector optimization.
434-434: Consider thread safety implications of mutable mutex.Making the mutex mutable allows it to be locked in const methods, which is a common pattern. However, ensure all const methods that access shared data are properly synchronized using this mutex.
✅ Verification successful
Thread safety implementation verified and correct
The mutable mutex is properly utilized with appropriate lock types throughout the codebase. The implementation correctly uses shared locks for read operations and unique locks for write operations, with proper lock upgrade patterns where necessary. All shared data access appears to be properly synchronized.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for const methods that might need synchronization ast-grep --pattern 'const { $$$ m_Profiles $$$ }'Length of output: 57
Script:
#!/bin/bash # Find the class definition and its const methods ast-grep --pattern 'class Profiler { $$$ $_($$$) const { $$$ } $$$ }' # Also search for any mutex usage rg "m_Mutex" -A 3 -B 3Length of output: 4611
imgui.ini (1)
65-67: LGTM! The Lua Console window configuration is appropriate.The window size (520x600) provides adequate space for the Lua console interface, and its position doesn't overlap with other critical windows.
.github/workflows/build.yml (1)
31-31: Verify Lua version compatibility.The addition of lua5.4 is appropriate for the scripting implementation. However, ensure that this version is compatible with any Lua scripts and libraries you plan to use.
✅ Verification successful
Lua 5.4 is fully compatible with the codebase
The Sol2 binding library explicitly supports Lua 5.4, and no version-specific dependencies or incompatibilities were found in the Lua scripts.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for Lua version requirements in the codebase rg -l "lua_version|LUA_VERSION|Lua [0-9]"Length of output: 65
Script:
#!/bin/bash # Check Sol2's Lua version requirements and search for Lua files echo "=== Sol2 Lua Version Requirements ===" rg "Lua [0-9]|LUA_VERSION" -A 3 external/sol2/sol.hpp echo -e "\n=== Searching for Lua files ===" fd -e luaLength of output: 11811
Script:
#!/bin/bash # Check Lua scripts for version-specific features or requirements echo "=== Checking Lua scripts for version-specific features ===" rg -p "require|module|version" sandbox/assets/scripts/*.luaLength of output: 257
src/ImGui/ImGuiLayer.cpp (1)
26-32: LGTM! Platform-specific viewport handling is well-implemented.The conditional compilation block correctly handles viewport configuration based on the platform, preventing potential issues on Linux while maintaining functionality on other platforms.
CMakeLists.txt (3)
13-16: LGTM! Good use of environment variables for portability.The toolchain configuration is well-structured, using environment variables for Windows paths and standard system directories for Unix.
73-74: LGTM! Source files properly integrated.The new Lua scripting and Scene management source files are correctly added to the engine sources.
106-115: LGTM! Well-structured post-build commands.The post-build commands are properly implemented:
- Use CMake's cross-platform file operations
- Correctly handle path construction
- Ensure resources are available in the build directory
Docstrings generation was requested by @arozx. * #14 (comment) The following files were modified: * `external/sol2/forward.hpp` * `sandbox/src/SandboxApp.cpp` * `src/Application.cpp` * `src/Debug/Profiler.cpp` * `src/ImGui/ImGuiLayer.cpp` * `src/Input/InputSystem.cpp` * `src/Renderer/Renderer.cpp` * `src/Scene/Scene.cpp` * `src/Scripting/LuaScriptSystem.cpp` * `src/TerrainSystem/TerrainSystem.cpp` * `src/UI/ImGuiOverlay.cpp` * `src/Window/OpenGLWindow.cpp`
|
Note Generated docstrings for this pull request, at #15 |
📝 Add docstrings to `Lua`
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 13
🔭 Outside diff range comments (2)
src/Application.cpp (1)
Missing scene validation in Application.cpp for both Update and Render calls
The review comment is valid and should be extended. Both scene operations need validation:
- SceneManager::Get().Update(deltaTime); + auto& sceneManager = SceneManager::Get(); + if (auto activeScene = sceneManager.GetActiveScene()) { + sceneManager.Update(deltaTime); + } - SceneManager::Get().Render(*m_Renderer); + if (activeScene) { // Reuse the same check + sceneManager.Render(*m_Renderer); + } else { + LOG_WARN("No active scene to render"); + }🔗 Analysis chain
Line range hint
316-332: Verify scene rendering in BeginScene.The scene rendering logic assumes the SceneManager always has a valid scene to render. This could lead to undefined behavior if no scene is active.
Add validation before rendering:
- SceneManager::Get().Render(*m_Renderer); + auto& sceneManager = SceneManager::Get(); + if (auto activeScene = sceneManager.GetActiveScene()) { + sceneManager.Render(*m_Renderer); + } else { + LOG_WARN("No active scene to render"); + }Let's verify the scene management implementation:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check scene management implementation for proper null checks # Search for direct scene access without validation rg -A 2 "SceneManager::Get\(\)\.(?:Render|Update|GetActiveScene)" --type cppLength of output: 1535
src/Debug/Profiler.cpp (1)
Line range hint
197-261: Optimize JSON serialization for large datasets.The current JSON serialization loads all profile data into memory before writing. This could be memory-intensive for large profiling sessions.
Implement streaming JSON serialization:
void Profiler::WriteCompleteJSON() const { if (!m_Enabled) return; - using nlohmann::json; - json output; + std::ofstream file(m_JSONOutputPath); + if (!file) { + LOG_ERROR_CONCAT("Failed to open profile output file: ", m_JSONOutputPath); + return; + } std::shared_lock lock(m_Mutex); - output["session"] = m_CurrentSession; - output["timestamp"] = std::to_string(std::chrono::system_clock::now().time_since_epoch().count()); - output["profiles"] = json::array(); + // Write JSON header + file << "{\n" + << " \"session\": \"" << m_CurrentSession << "\",\n" + << " \"timestamp\": \"" << std::chrono::system_clock::now().time_since_epoch().count() << "\",\n" + << " \"profiles\": [\n"; - // Add profile data first + // Stream profile data + bool firstProfile = true; for (const auto& [name, data] : m_Profiles) { if (!data || data->calls == 0) continue; - json profile = { - {"name", std::string(std::string_view(name))}, - // ... - }; + if (!firstProfile) file << ",\n"; + firstProfile = false; + + file << " {\n" + << " \"name\": \"" << std::string(std::string_view(name)) << "\",\n" + << " \"calls\": " << data->calls << ",\n" + // ... write other fields + << " }"; } + + file << "\n ]\n}\n";
♻️ Duplicate comments (4)
src/Scene/SceneManager.h (1)
10-13:⚠️ Potential issueMake singleton implementation thread-safe.
The current singleton implementation is not thread-safe and could lead to race conditions in multi-threaded environments.
src/Renderer/Renderer.cpp (1)
177-186: 🛠️ Refactor suggestionRemove redundant Render method.
The
Render()method appears to be redundant as it simply callsFlush(). Consider removing this method to avoid API confusion.src/TerrainSystem/TerrainSystem.cpp (1)
184-189: 🛠️ Refactor suggestionRemove duplicate functionality.
The
GenerateMesh(uint32_t seed)method duplicates the functionality ofRegenerateTerrain. Consider removing this method and usingRegenerateTerraininstead to avoid code duplication.src/Scripting/LuaScriptSystem.cpp (1)
87-371: 🛠️ Refactor suggestionDecompose RegisterEngineAPI for better maintainability.
The RegisterEngineAPI method is too large (284 lines) and handles multiple concerns. This makes it difficult to maintain and test.
Split into smaller, focused methods:
void LuaScriptSystem::RegisterEngineAPI() { auto engine = m_LuaState->create_named_table("engine"); - // Terrain API... - // Renderer API... - // Scene API... - // Input API... - // Debug/Logging API... - // Profiling... - // File System API... - // Camera... - // ImGui Overlay controls... + RegisterTerrainAPI(engine); + RegisterRendererAPI(engine); + RegisterSceneAPI(engine); + RegisterInputAPI(engine); + RegisterDebugAPI(engine); + RegisterProfilingAPI(engine); + RegisterFileSystemAPI(engine); + RegisterCameraAPI(engine); + RegisterImGuiAPI(engine); } +void LuaScriptSystem::RegisterTerrainAPI(sol::table& engine) { + engine.set_function("setTerrainHeight", [this](float height) { + // Implementation... + }); + // Other terrain functions... +}
🧹 Nitpick comments (11)
sandbox/assets/scripts/init.lua (3)
4-5: Consider making directory paths configurable.The build and script directories are hardcoded. Consider making these paths configurable through environment variables or a configuration file for better flexibility across different environments.
-local buildDir = "build/assets/scripts" -local scriptDir = "sandbox/assets/scripts" +local buildDir = os.getenv("VOXEL_BUILD_DIR") or "build/assets/scripts" +local scriptDir = os.getenv("VOXEL_SCRIPT_DIR") or "sandbox/assets/scripts"
57-63: Improve script management and validation.The scripts list is hardcoded and there's no validation of script existence before starting the copy process.
-local scripts = { "main.lua", "engine.lua" } +-- Load script list from configuration +local function loadScriptList() + local configPath = scriptDir .. "/scripts.json" + local file = io.open(configPath, "r") + if file then + local content = file:read("*all") + file:close() + local success, scripts = pcall(require("json").decode, content) + if success then return scripts end + end + return { "main.lua", "engine.lua" } -- fallback to defaults +end + +-- Validate all scripts exist before starting +local scripts = loadScriptList() +for _, script in ipairs(scripts) do + local path = scriptDir .. "/" .. script + local file = io.open(path, "r") + if not file then + engine.error(string.format("Script %s not found", script)) + return + end + file:close() +end + for _, script in ipairs(scripts) do
65-65: Enhance completion logging with summary information.Consider adding summary statistics to the completion message.
-engine.trace("init.lua execution complete") +engine.trace(string.format("init.lua execution complete - copied %d scripts", #scripts))src/Renderer/Renderer.cpp (2)
103-115: Improve parallel processing efficiency.The current implementation could be optimized:
- The batch size is hardcoded
- No load balancing between tasks
Consider this improvement:
- const size_t batchSize = 64; // Number of commands to process per task + const size_t batchSize = std::max(64UL, commands.size() / TaskSystem::Get().GetWorkerCount());
121-143: Add error handling for OpenGL calls.The rendering code should handle potential OpenGL errors to aid in debugging rendering issues.
Add error checking:
// Execute render commands on main thread for (const auto& command : commands) { command.material->Bind(); auto shader = command.material->GetShader(); ASSERT(shader != nullptr && "Null shader in material"); if (m_CameraType == CameraType::Orthographic) { shader->SetMat4("u_ViewProjection", m_Camera->GetViewProjectionMatrix()); } else { shader->SetMat4("u_ViewProjection", m_PerspectiveCamera->GetViewProjectionMatrix()); } shader->SetMat4("u_Model", command.modelMatrix); command.vertexArray->Bind(); glDrawElements(command.primitiveType, command.vertexArray->GetIndexBuffer()->GetCount(), GL_UNSIGNED_INT, nullptr); + GLenum error = glGetError(); + if (error != GL_NO_ERROR) { + LOG_ERROR_CONCAT("OpenGL error during rendering: ", error); + } command.vertexArray->Unbind(); command.material->Unbind(); }src/TerrainSystem/TerrainSystem.cpp (1)
Line range hint
145-170: Add error handling for mesh generation.The mesh generation code should handle potential errors during vertex and index buffer creation.
Add error handling:
// Create vertex array with mesh data m_TerrainVA.reset(VertexArray::Create()); if (!vertices.empty()) { + try { std::shared_ptr<VertexBuffer> vertexBuffer( VertexBuffer::Create(vertices.data(), vertices.size() * sizeof(float))); BufferLayout layout = { { ShaderDataType::Float3, "aPosition" }, { ShaderDataType::Float2, "aTexCoord" } }; vertexBuffer->SetLayout(layout); m_TerrainVA->AddVertexBuffer(vertexBuffer); std::shared_ptr<IndexBuffer> indexBuffer( IndexBuffer::Create(indices.data(), indices.size())); m_TerrainVA->SetIndexBuffer(indexBuffer); + } catch (const std::exception& e) { + LOG_ERROR_CONCAT("Failed to create terrain mesh: ", e.what()); + m_TerrainVA.reset(); + return; + } }src/Input/InputSystem.cpp (2)
264-269: Add error handling for callback execution.The callback execution should be wrapped in a try-catch block to prevent exceptions from propagating up the call stack.
void InputSystem::OnKeyEvent(int key, int action) { auto it = m_KeyCallbacks.find(key); if (it != m_KeyCallbacks.end()) { + try { it->second(action); + } catch (const std::exception& e) { + // Log the error and optionally remove the failing callback + LOG_ERROR("Key callback failed: {}", e.what()); + } } }
Line range hint
194-269: Consider documenting the threading model.The addition of callbacks for key events is a good architectural choice for supporting Lua scripting. However, since input handling can occur from different threads (GLFW callbacks run on the main thread, while Lua scripts might run on different threads), it would be valuable to:
- Document the threading model
- Consider adding thread safety mechanisms for callback registration/execution
- Add a note about callback execution order guarantees
src/UI/ImGuiOverlay.cpp (1)
327-361: Add state persistence for UI visibility controls.The UI visibility controls don't persist their state across application sessions. Consider adding serialization support.
Add configuration persistence:
+void ImGuiOverlay::SaveState() { + json state; + state["transform_controls"] = m_ShowTransformControls; + state["profiler"] = m_ShowProfiler; + state["renderer_settings"] = m_ShowRendererSettings; + state["event_debugger"] = m_ShowEventDebugger; + state["terrain_controls"] = m_ShowTerrainControls; + state["fps_counter"] = m_ShowFPSCounter; + + std::ofstream file("imgui_state.json"); + file << state.dump(2); +} + +void ImGuiOverlay::LoadState() { + try { + std::ifstream file("imgui_state.json"); + if (file) { + json state = json::parse(file); + m_ShowTransformControls = state["transform_controls"]; + m_ShowProfiler = state["profiler"]; + m_ShowRendererSettings = state["renderer_settings"]; + m_ShowEventDebugger = state["event_debugger"]; + m_ShowTerrainControls = state["terrain_controls"]; + m_ShowFPSCounter = state["fps_counter"]; + } + } catch (const std::exception& e) { + LOG_WARN_CONCAT("Failed to load ImGui state: ", e.what()); + } +}src/Debug/Profiler.cpp (2)
19-20: Make sampling rate configurable.The sampling rate is hardcoded to 60Hz. This should be configurable to support different profiling needs.
Make the rate configurable:
-thread_local static std::chrono::steady_clock::time_point s_LastSampleTime = std::chrono::steady_clock::now(); -static constexpr std::chrono::duration<float> SAMPLE_INTERVAL{1.0f/60.0f}; // 60Hz sampling +thread_local static std::chrono::steady_clock::time_point s_LastSampleTime = std::chrono::steady_clock::now(); + +class ProfilerConfig { +public: + static float GetSamplingRate() { + static float rate = Configuration::Get().GetFloat("Profiler.SamplingRate", 60.0f); + return rate; + } + + static std::chrono::duration<float> GetSampleInterval() { + return std::chrono::duration<float>{1.0f / GetSamplingRate()}; + } +};
355-391: Enhance thread safety of profile data collection.The current implementation uses shared locks but could be optimized to reduce lock contention.
Consider using lock-free data structures:
+template<typename T> +class LockFreeQueue { + // Implementation of a lock-free queue for profile data +}; + void Profiler::WriteProfile(std::string_view name, float duration) { if (!m_Enabled || name.empty()) return; // Rate limit samples to 60Hz auto currentTime = std::chrono::steady_clock::now(); if (currentTime - s_LastSampleTime < SAMPLE_INTERVAL) { return; } s_LastSampleTime = currentTime; + // Use lock-free queue for measurements + m_ProfileQueue.push({name, duration, currentTime}); + - // Current implementation with locks - std::shared_lock readLock(m_Mutex); - // ... + // Process queue periodically in a separate thread + if (ShouldFlushQueue()) { + ProcessProfileQueue(); + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
sandbox/assets/scripts/init.lua(1 hunks)sandbox/src/SandboxApp.cpp(1 hunks)src/Application.cpp(7 hunks)src/Debug/Profiler.cpp(6 hunks)src/ImGui/ImGuiLayer.cpp(3 hunks)src/Input/InputSystem.cpp(2 hunks)src/Renderer/RenderableObject.h(1 hunks)src/Renderer/Renderer.cpp(1 hunks)src/Renderer/Renderer.h(3 hunks)src/Scene/Scene.cpp(1 hunks)src/Scene/SceneManager.h(1 hunks)src/Scripting/LuaScriptSystem.cpp(1 hunks)src/Scripting/LuaScriptSystem.h(1 hunks)src/TerrainSystem/TerrainSystem.cpp(4 hunks)src/UI/ImGuiOverlay.cpp(7 hunks)src/Window/OpenGLWindow.cpp(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- src/ImGui/ImGuiLayer.cpp
- src/Window/OpenGLWindow.cpp
- src/Renderer/Renderer.h
- src/Scene/Scene.cpp
- src/Scripting/LuaScriptSystem.h
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build and analyze with SonarQube
🔇 Additional comments (10)
src/Renderer/RenderableObject.h (1)
23-24: LGTM! Consistent with renderer refactoring.The change to use
GetModelMatrix()instead of passing the transform directly aligns well with the broader refactoring of the rendering pipeline to use model matrices consistently.src/Scene/SceneManager.h (5)
15-18: LGTM! Thread-safe scene addition.The implementation properly uses mutex protection for thread safety and maintains a clean interface.
20-32: LGTM! Well-implemented scene activation with proper logging.The implementation includes comprehensive error handling and informative logging at appropriate levels.
58-63: LGTM! Thread-safe Update method.The implementation correctly protects the active scene with a mutex lock during updates.
65-70: LGTM! Thread-safe Render method.The implementation properly synchronizes access to the active scene during rendering.
46-56: 🛠️ Refactor suggestionImprove error handling in RemoveScene.
The current implementation could lead to memory leaks or undefined behavior if scene cleanup fails.
Apply this diff to add proper cleanup and error handling:
bool RemoveScene(const std::string& name) { std::lock_guard<std::mutex> lock(sceneMutex); if (scenes.find(name) == scenes.end()) { LOG_ERROR_CONCAT("Cannot remove scene: ", name, " (not found)"); return false; } if (activeScene && activeScene->GetName() == name) { + try { + activeScene->OnDeactivate(); + } catch (const std::exception& e) { + LOG_ERROR_CONCAT("Error during scene cleanup: ", e.what()); + } activeScene = nullptr; } return scenes.erase(name) > 0; }Likely invalid or redundant comment.
src/Renderer/Renderer.cpp (1)
79-94: LGTM! Thread-safe command queue processing.The implementation properly handles command queue synchronization and prevents recursive frame processing.
src/TerrainSystem/TerrainSystem.cpp (1)
66-69: LGTM! Improved logging granularity.The change from INFO to TRACE level for terrain transform logging is appropriate for detailed debugging.
src/Input/InputSystem.cpp (2)
194-203: Well-documented implementation!The documentation clearly explains the cursor state management functionality and its interaction with GLFW.
213-228: Add input parameter validation.While the implementation is efficient using
std::move, consider adding validation:
- Check if
keyis a valid GLFW key code- Verify that
callbackis not nullLet's verify the KeyCallback type definition and usage:
| Application* Application::s_Instance = nullptr; | ||
|
|
||
| /** | ||
| * @brief Initialize the application and all subsystems | ||
| */ | ||
| Application::Application() | ||
| { | ||
| LOG_INFO("Creating Application"); | ||
|
|
||
| s_Instance = this; |
There was a problem hiding this comment.
Ensure thread-safety of the singleton instance.
The static instance pointer s_Instance is assigned in the constructor without any synchronization mechanism. This could lead to race conditions if multiple threads attempt to create Application instances simultaneously.
Consider using the following thread-safe singleton pattern:
-Application* Application::s_Instance = nullptr;
+std::atomic<Application*> Application::s_Instance = nullptr;
+std::mutex Application::s_InstanceMutex;
Application::Application()
{
- s_Instance = this;
+ std::lock_guard<std::mutex> lock(s_InstanceMutex);
+ s_Instance.store(this, std::memory_order_release);Committable suggestion skipped: line range outside the PR's diff.
| // Initialize script system first | ||
| m_ScriptSystem = std::make_unique<LuaScriptSystem>(); | ||
| if (!m_ScriptSystem) { | ||
| LOG_ERROR("Failed to create script system"); | ||
| return; | ||
| } | ||
|
|
||
| try { | ||
| m_ScriptSystem->Initialize(); | ||
|
|
||
| // Only execute init.lua once from the source directory | ||
| LOG_INFO("Loading init.lua script..."); | ||
| if (!m_ScriptSystem->ExecuteFile("sandbox/assets/scripts/init.lua")) { | ||
| LOG_ERROR("Failed to execute init.lua"); | ||
| return; | ||
| } | ||
|
|
||
| // Only execute main.lua once from the build directory | ||
| LOG_INFO("Loading main.lua script..."); | ||
| if (!m_ScriptSystem->ExecuteFile("build/assets/scripts/main.lua")) { | ||
| LOG_ERROR("Failed to execute main.lua"); | ||
| return; | ||
| } | ||
|
|
||
| } catch (const std::exception& e) { | ||
| LOG_ERROR_CONCAT("Failed to initialize script system: ", e.what()); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Improve error handling in script system initialization.
The script system initialization has several issues:
- Early returns in error cases may leave the application in an inconsistent state
- No cleanup is performed when initialization fails
- Hardcoded script paths could be configuration-driven
Consider refactoring the initialization to use RAII and configuration:
- // Initialize script system first
- m_ScriptSystem = std::make_unique<LuaScriptSystem>();
- if (!m_ScriptSystem) {
- LOG_ERROR("Failed to create script system");
- return;
- }
-
- try {
- m_ScriptSystem->Initialize();
-
- // Only execute init.lua once from the source directory
- LOG_INFO("Loading init.lua script...");
- if (!m_ScriptSystem->ExecuteFile("sandbox/assets/scripts/init.lua")) {
- LOG_ERROR("Failed to execute init.lua");
- return;
- }
-
- // Only execute main.lua once from the build directory
- LOG_INFO("Loading main.lua script...");
- if (!m_ScriptSystem->ExecuteFile("build/assets/scripts/main.lua")) {
- LOG_ERROR("Failed to execute main.lua");
- return;
- }
-
- } catch (const std::exception& e) {
- LOG_ERROR_CONCAT("Failed to initialize script system: ", e.what());
- return;
- }
+ // Initialize script system first
+ try {
+ m_ScriptSystem = std::make_unique<LuaScriptSystem>();
+ m_ScriptSystem->Initialize();
+
+ // Load scripts from configuration
+ const auto& config = Configuration::Get();
+ for (const auto& script : config.GetInitializationScripts()) {
+ if (!m_ScriptSystem->ExecuteFile(script)) {
+ throw std::runtime_error("Failed to execute script: " + script);
+ }
+ }
+ } catch (const std::exception& e) {
+ LOG_ERROR_CONCAT("Failed to initialize script system: ", e.what());
+ Cleanup(); // Ensure proper cleanup
+ throw; // Propagate error to prevent inconsistent state
+ }Committable suggestion skipped: line range outside the PR's diff.
| void ImGuiOverlay::RenderTerrainControls() { | ||
| auto activeScene = Engine::SceneManager::Get().GetActiveScene(); | ||
| if (!activeScene) return; | ||
|
|
||
| auto terrainSystem = activeScene->GetTerrainSystem(); | ||
| if (!terrainSystem) return; | ||
|
|
||
| if (ImGui::Begin("Terrain Controls")) { | ||
| // Chunk range control | ||
| int chunkRange = terrainSystem.GetChunkRange(); | ||
| int chunkRange = terrainSystem->GetChunkRange(); | ||
| if (ImGui::SliderInt("Chunk Range", &chunkRange, 0, 5)) { | ||
| terrainSystem.SetChunkRange(chunkRange); | ||
| terrainSystem->SetChunkRange(chunkRange); | ||
| } | ||
|
|
||
| // Terrain seed control | ||
| static uint32_t seed = 1234; | ||
| if (ImGui::InputScalar("Terrain Seed", ImGuiDataType_U32, &seed)) { | ||
| // No need for validation since uint32_t can't be negative | ||
| } | ||
|
|
||
| if (ImGui::Button("Regenerate Terrain")) { | ||
| terrainSystem.RegenerateTerrain(seed); | ||
| terrainSystem->RegenerateTerrain(seed); | ||
| } | ||
|
|
||
| // Terrain parameters | ||
| ImGui::Separator(); | ||
| ImGui::Text("Terrain Parameters:"); | ||
|
|
||
| static float baseHeight = 32.0f; | ||
| static float heightScale = 32.0f; | ||
| static float noiseScale = 0.05f; | ||
| static bool autoUpdate = false; | ||
|
|
||
| if (ImGui::SliderFloat("Base Height", &baseHeight, 0.0f, 64.0f) && autoUpdate) { | ||
| terrainSystem.SetBaseHeight(baseHeight); | ||
| terrainSystem->SetBaseHeight(baseHeight); | ||
| } | ||
| if (ImGui::SliderFloat("Height Scale", &heightScale, 1.0f, 64.0f) && autoUpdate) { | ||
| terrainSystem.SetHeightScale(heightScale); | ||
| terrainSystem->SetHeightScale(heightScale); | ||
| } | ||
| if (ImGui::SliderFloat("Noise Scale", &noiseScale, 0.01f, 0.2f) && autoUpdate) { | ||
| terrainSystem.SetNoiseScale(noiseScale); | ||
| terrainSystem->SetNoiseScale(noiseScale); | ||
| } | ||
|
|
||
| ImGui::Checkbox("Auto Update", &autoUpdate); | ||
|
|
||
| if (!autoUpdate && ImGui::Button("Apply Parameters")) { | ||
| terrainSystem.SetBaseHeight(baseHeight); | ||
| terrainSystem.SetHeightScale(heightScale); | ||
| terrainSystem.SetNoiseScale(noiseScale); | ||
| terrainSystem.RegenerateTerrain(seed); // Fixed: Added seed parameter | ||
| terrainSystem->SetBaseHeight(baseHeight); | ||
| terrainSystem->SetHeightScale(heightScale); | ||
| terrainSystem->SetNoiseScale(noiseScale); | ||
| terrainSystem->RegenerateTerrain(seed); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve terrain controls error handling and state management.
The terrain controls implementation has several areas for improvement:
- No error handling for invalid terrain parameters
- Static variables for terrain parameters lose state when UI is hidden
- No validation of parameter ranges before applying
Consider the following improvements:
void ImGuiOverlay::RenderTerrainControls() {
auto activeScene = Engine::SceneManager::Get().GetActiveScene();
if (!activeScene) return;
auto terrainSystem = activeScene->GetTerrainSystem();
if (!terrainSystem) return;
+ // Store parameters as member variables instead of static
+ if (!m_TerrainParams.initialized) {
+ m_TerrainParams = {
+ terrainSystem->GetBaseHeight(),
+ terrainSystem->GetHeightScale(),
+ terrainSystem->GetNoiseScale(),
+ false, // autoUpdate
+ true // initialized
+ };
+ }
if (ImGui::Begin("Terrain Controls")) {
int chunkRange = terrainSystem->GetChunkRange();
if (ImGui::SliderInt("Chunk Range", &chunkRange, 0, 5)) {
terrainSystem->SetChunkRange(chunkRange);
}
- static uint32_t seed = 1234;
+ if (ImGui::InputScalar("Seed", ImGuiDataType_U32, &m_TerrainParams.seed)) {
+ // Validate seed range if needed
+ }
if (ImGui::Button("Regenerate Terrain")) {
- terrainSystem->RegenerateTerrain(seed);
+ terrainSystem->RegenerateTerrain(m_TerrainParams.seed);
}
ImGui::Separator();
ImGui::Text("Terrain Parameters:");
- static float baseHeight = 32.0f;
- static float heightScale = 32.0f;
- static float noiseScale = 0.05f;
- static bool autoUpdate = false;
- if (ImGui::SliderFloat("Base Height", &baseHeight, 0.0f, 64.0f) && autoUpdate) {
+ if (ImGui::SliderFloat("Base Height", &m_TerrainParams.baseHeight, 0.0f, 64.0f)) {
+ if (m_TerrainParams.autoUpdate && ValidateTerrainParams()) {
+ terrainSystem->SetBaseHeight(m_TerrainParams.baseHeight);
+ }
}
// Similar changes for other parameters...
- ImGui::Checkbox("Auto Update", &autoUpdate);
+ ImGui::Checkbox("Auto Update", &m_TerrainParams.autoUpdate);
- if (!autoUpdate && ImGui::Button("Apply Parameters")) {
+ if (!m_TerrainParams.autoUpdate && ImGui::Button("Apply Parameters")) {
+ if (ValidateTerrainParams()) {
terrainSystem->SetBaseHeight(baseHeight);
// Apply other parameters...
+ }
}
}
ImGui::End();
}
+bool ImGuiOverlay::ValidateTerrainParams() const {
+ if (m_TerrainParams.baseHeight < 0.0f || m_TerrainParams.baseHeight > 64.0f) {
+ LOG_ERROR("Invalid base height value");
+ return false;
+ }
+ // Validate other parameters...
+ return true;
+}Committable suggestion skipped: line range outside the PR's diff.
| bool LuaScriptSystem::ExecuteFile(const std::string& originalPath) { | ||
| std::vector<std::string> searchPaths = { | ||
| originalPath, "../" + originalPath, "../../" + originalPath, | ||
| "../sandbox/assets/scripts/" + originalPath.substr(originalPath.find_last_of("/\\") + 1), | ||
| "sandbox/assets/scripts/" + originalPath.substr(originalPath.find_last_of("/\\") + 1)}; | ||
|
|
||
| std::string validPath; | ||
| bool found = false; | ||
|
|
||
| for (const auto& path : searchPaths) { | ||
| if (FileSystem::Exists(path)) { | ||
| validPath = path; | ||
| found = true; | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| if (!found) { | ||
| LOG_ERROR("Script not found: ", originalPath); | ||
| return false; | ||
| } | ||
|
|
||
| sol::load_result loadResult = m_LuaState->load_file(validPath); | ||
| if (!loadResult.valid() || !loadResult().valid()) { | ||
| LOG_ERROR("Failed to execute script: ", validPath); | ||
| return false; | ||
| } | ||
|
|
||
| return true; | ||
| } |
There was a problem hiding this comment.
Improve script file path handling.
The script file path handling is brittle and could lead to path traversal vulnerabilities. Consider using a more robust approach.
Enhance path handling:
bool LuaScriptSystem::ExecuteFile(const std::string& originalPath) {
+ // Sanitize and validate path
+ if (!IsPathSafe(originalPath)) {
+ LOG_ERROR_CONCAT("Invalid script path: ", originalPath);
+ return false;
+ }
+
+ // Use configured script directories
+ const auto& scriptDirs = Configuration::Get().GetScriptDirectories();
std::vector<std::string> searchPaths = {
- originalPath, "../" + originalPath, "../../" + originalPath,
- "../sandbox/assets/scripts/" + originalPath.substr(originalPath.find_last_of("/\\") + 1),
- "sandbox/assets/scripts/" + originalPath.substr(originalPath.find_last_of("/\\") + 1)};
+ originalPath
};
+
+ // Add configured directories
+ const auto filename = FileSystem::GetFilename(originalPath);
+ for (const auto& dir : scriptDirs) {
+ searchPaths.push_back(FileSystem::JoinPaths(dir, filename));
+ }
std::string validPath;
bool found = false;
for (const auto& path : searchPaths) {
- if (FileSystem::Exists(path)) {
+ if (FileSystem::Exists(path) && IsScriptFile(path)) {
validPath = path;
found = true;
break;
}
}Committable suggestion skipped: line range outside the PR's diff.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
sandbox/assets/scripts/init.lua (1)
39-39:⚠️ Potential issueFix incorrect string.format usage.
The string.format calls are incorrectly formatted. The format specifiers are missing, which will result in incorrect error messages.
- engine.error(string.format("Could not open source script ", sourcePath, ": ", sourceErr)) + engine.error(string.format("Could not open source script %s: %s", sourcePath, sourceErr)) - engine.error(string.format("Failed to read from ", sourcePath)) + engine.error(string.format("Failed to read from %s", sourcePath)) - engine.error(string.format("Failed to create destination file", destPath, ": ", destErr)) + engine.error(string.format("Failed to create destination file %s: %s", destPath, destErr)) - engine.error(string.format("Failed to write to ", destPath)) + engine.error(string.format("Failed to write to %s", destPath)) - engine.trace(string.format("Successfully copied: ", name)) + engine.trace(string.format("Successfully copied: %s", name))Also applies to: 56-56, 63-63, 70-70, 76-76
🧹 Nitpick comments (2)
sandbox/assets/scripts/init.lua (2)
37-41: Enhance error messages with more context.The error messages could provide more context about the operation being performed and potential solutions.
local source, sourceErr = io.open(sourcePath, "rb") if not source then - engine.error(string.format("Could not open source script %s: %s", sourcePath, sourceErr)) + engine.error(string.format("Could not open source script %s: %s. Please verify the file exists and you have read permissions.", sourcePath, sourceErr)) return false end local dest, destErr = io.open(destPath, "wb") if not dest then - engine.error(string.format("Failed to create destination file %s: %s", destPath, destErr)) + engine.error(string.format("Failed to create destination file %s: %s. Please verify you have write permissions in the build directory.", destPath, destErr)) closeFiles(source) return false endAlso applies to: 61-66
80-87: Improve script management and error handling.Consider the following improvements:
- Move the script list to a configuration file for better maintainability
- Add pre-validation of all scripts before starting the copy process
- Provide more detailed error reporting
+-- Load script list from config +local function loadScriptConfig() + local configPath = scriptDir .. "/script_config.lua" + local config = loadfile(configPath) + if not config then + engine.error("Failed to load script configuration") + return nil + end + return config() +end + +-- Validate all scripts exist before copying +local function validateScripts(scripts) + for _, script in ipairs(scripts) do + local path = scriptDir .. "/" .. script + local file = io.open(path, "rb") + if not file then + engine.error(string.format("Required script %s not found", script)) + return false + end + file:close() + end + return true +end + -local scripts = { "main.lua", "engine.lua" } +local scripts = loadScriptConfig() +if not scripts then + return +end + +if not validateScripts(scripts) then + return +end + for _, script in ipairs(scripts) do if not copyScript(script) then - engine.error("Failed to copy " .. script) + engine.error(string.format("Failed to copy %s. Aborting script copy process.", script)) return end end
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
sandbox/assets/scripts/init.lua(1 hunks)
🔇 Additional comments (1)
sandbox/assets/scripts/init.lua (1)
1-90: Overall implementation is solid but requires attention to identified issues.The implementation follows good practices for file operations, error handling, and security. However, please address the following issues:
- Fix string.format usage throughout the file
- Enhance path sanitization
- Consider moving script configuration to a separate file
- Improve error messages with more context
Once these issues are addressed, the implementation will be more robust and maintainable.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Summary by CodeRabbit
Release Notes
New Features
engineLua module for terrain and camera control.Improvements
Bug Fixes
Development Tools