optimise & abstract application #13
Conversation
This reverts commit eeff4d3.
WalkthroughThis pull request introduces a comprehensive set of changes to the voxel engine project, focusing on enhancing terrain generation, noise generation, shader management, performance profiling, and overall system architecture. The modifications span multiple domains, including noise generation algorithms, rendering systems, input handling, and debugging tools. Key additions include new noise generation interfaces, shader hot-reloading capabilities, improved terrain generation techniques, and advanced performance monitoring features through flame graph visualizations. Changes
Sequence DiagramsequenceDiagram
participant App as Application
participant Renderer
participant TerrainSystem
participant NoiseGen as Noise Generator
participant Shader
participant ShaderHotReload
App->>Renderer: Initialize
Renderer->>NoiseGen: Create Noise Generator
TerrainSystem->>NoiseGen: Generate Heightmap
TerrainSystem->>Renderer: Create Terrain Mesh
Shader->>ShaderHotReload: Watch Shader Files
loop File Change Detection
ShaderHotReload->>Shader: Reload Shader
Shader-->>Renderer: Update Rendering Pipeline
end
The sequence diagram illustrates the initialization and interaction between key components, highlighting the dynamic shader reloading and terrain generation processes. ✨ Finishing Touches
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 (
|
Analysis Details0 Issues
Coverage and Duplications
Project ID: arozx_voxels_d0e4bd76-3798-45bc-a3e0-f1357e3eed04 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 50
🔭 Outside diff range comments (5)
src/Shader/Shader.h (1)
Line range hint
18-119: Ensure correct usage ofstd::enable_shared_from_thisBy inheriting from
std::enable_shared_from_this<Shader>, it's important to ensure that all instances ofShaderare managed bystd::shared_ptr. Creating aShaderinstance without ashared_ptrand then callingshared_from_this()will lead to undefined behavior.Consider making constructors
privateorprotectedand enforcing object creation through theCreateFromFilesandCreateFromSourcefactory methods to guarantee thatShaderinstances are always managed bystd::shared_ptr.src/Events/KeyCodes.h (1)
Line range hint
74-89: Update KeyCodeToString to handle all key codes.The KeyCodeToString function doesn't handle the NUM_* keys, which could lead to "Unknown Key" being returned for valid key codes.
inline std::string KeyCodeToString(int keycode) { if (keycode >= Key_A && keycode <= Key_Z) { return std::string(1, static_cast<char>(keycode)); } - if (keycode >= Key_0 && keycode <= Key_9) { + if ((keycode >= Key_0 && keycode <= Key_9) || + (keycode >= NUM_0 && keycode <= NUM_9)) { return std::string(1, static_cast<char>(keycode)); } switch (keycode) {src/Core/TaskSystem.h (1)
Line range hint
27-39: Enhance initialization robustness and add status query methods.Good addition of initialization check, but consider these improvements:
- Add method to query initialization status
- Add method to get thread count
- Make thread count calculation more robust
+ bool IsInitialized() const noexcept { return m_Initialized; } + size_t GetThreadCount() const noexcept { return m_Workers.size(); } void Initialize(size_t threadCount = 0) { ASSERT(m_Initialized == false && "TaskSystem already initialized!"); if (threadCount == 0) { - threadCount = std::max(1u, std::thread::hardware_concurrency() / 2); + const unsigned int hw_threads = std::thread::hardware_concurrency(); + threadCount = std::max(1u, hw_threads > 0 ? hw_threads / 2 : 2); }src/Renderer/Texture.cpp (2)
Line range hint
22-60: Add error handling for texture loading failures.The texture loading code should handle failures more gracefully and provide detailed error information.
Texture::Texture(const std::string& path) : m_Path(path) { stbi_set_flip_vertically_on_load(1); int width, height, channels; unsigned char* data = stbi_load(path.c_str(), &width, &height, &channels, 0); + + if (!data) { + throw std::runtime_error(std::format("Failed to load texture '{}': {}", + path, stbi_failure_reason())); + }
Line range hint
149-156: Add state validation in Unload method.The Unload method should validate the texture state and handle potential OpenGL errors.
void Texture::Unload() { + if (!m_IsLoaded) { + return; + } + if (m_RendererID) { + GLint currentTexture; + glGetIntegerv(GL_TEXTURE_BINDING_2D, ¤tTexture); glDeleteTextures(1, &m_RendererID); + if (static_cast<GLuint>(currentTexture) == m_RendererID) { + glBindTexture(GL_TEXTURE_2D, 0); + } m_RendererID = 0; } m_IsLoaded = false; }
🧹 Nitpick comments (73)
src/Noise/PerlinNoise/PerlinNoise.cpp (1)
83-98: Optimize the heightmap generation by precomputing constants.Variables like
maxValueandamplitudedepend only on the number of octaves and persistence. Calculating them once before the loops can improve performance.Refactor to compute
maxValueprior to the nested loops:float maxValue = 0.0f; float amplitude = 1.0f; for (int i = 0; i < m_Octaves; i++) { maxValue += amplitude; amplitude *= m_Persistence; } // Inside the loops for (int y = 0; y < height; y++) { for (int x = 0; x < width; x++) { float total = 0.0f; amplitude = 1.0f; frequency = 1.0f; for (int i = 0; i < m_Octaves; i++) { // ... existing code ... } heightmap[y * width + x] = total / maxValue; } }src/VoxelChunk.cpp (1)
89-90: Redundant return statement forBlockType::StoneBoth conditions return
BlockType::Stone. The final return statement on line 90 may be redundant since it returns the same value as the previous condition.Consider simplifying the method:
if (worldY <= STONE_HEIGHT) return BlockType::Stone; - return BlockType::Stone; + // Optional: handle any other cases or remove the redundant returnIf the default
BlockType::Stoneis intended for all other cases, the code is acceptable as is. However, explicitly stating it helps clarify intent.src/Input/InputSystem.h (2)
67-70: Add documentation comments for new public methodsThe methods
ToggleCameraControls,ToggleSmoothCamera,SetMovementSpeed, andGetMovementSpeedare public but lack documentation comments. Adding brief descriptions will improve maintainability and consistency.
105-105: Consider definingFIXED_TIMESTEPas a static constantDefining
FIXED_TIMESTEPas astatic constexprinstead of a member variable can reduce memory usage and emphasize its constant nature.src/TerrainSystem/TerrainSystem.cpp (2)
22-24: Initialize member variables in the constructor initializer listConsider initializing member variables like
m_TerrainTextureandm_TerrainMaterialin the constructor's initializer list to improve performance and code clarity.[performance]
26-26: Movem_TerrainTextureinitialization to the initializer listInitializing
m_TerrainTexturein the constructor's initializer list, as suggested by static analysis tools, can enhance performance and maintainability.[performance]
🧰 Tools
🪛 cppcheck (2.10-2)
[performance] 26-26: Variable 'm_TerrainTexture' is assigned in constructor body. Consider performing initialization in initialization list.
(useInitializationList)
src/Renderer/Renderer.cpp (2)
61-62: Clarify assertion condition with parenthesesThe assertion for validating
primitiveTypewould be clearer with parentheses:ASSERT((primitiveType == GL_TRIANGLES || primitiveType == GL_LINES || primitiveType == GL_POINTS) && "Invalid primitive type");This ensures the assertion message is correctly associated with the condition.
170-171: Clarify assertion condition with parenthesesIn
Renderer::SetCameraType(), the assertion condition can be clarified with parentheses:ASSERT((type == CameraType::Orthographic || type == CameraType::Perspective) && "Invalid camera type");This ensures the assertion message is correctly associated with the condition.
src/Shader/Shader.cpp (3)
135-135: Use consistent logging mechanism instead ofstd::coutIn line 135, you're using
std::coutto output an error message. To maintain consistency and leverage the existing logging system, consider replacingstd::coutwithLOG_ERRORor an equivalent logging function.Apply this diff:
- std::cout << "Shader program linking failed:\n" << infoLog << std::endl; + LOG_ERROR("Shader program linking failed:\n{0}", infoLog);
117-144: Refactor to eliminate code duplication between constructor andLoadFromSourceThe logic for compiling shaders and creating the program in
LoadFromSourceis similar to that in the constructorShader::Shader(const char* vertexSrc, const char* fragmentSrc). Consider refactoring the code to avoid duplication by having the constructor callLoadFromSourceor extracting common logic into a helper method.
180-180: Preferstd::make_sharedover manualnewfor creating shared pointersIn line 180, you are creating a
std::shared_ptrusingnew. It's generally recommended to usestd::make_sharedfor better performance and exception safety unless there is a specific reason not to.Apply this diff:
-auto shader = std::shared_ptr<Shader>(new Shader()); // Use new instead of make_shared +auto shader = std::make_shared<Shader>();src/Debug/Profiler.cpp (2)
159-161: Check file stream before writing JSON outputWhile writing the JSON output to a file, there's a check for the file stream, but exceptions might still occur during the write operation.
Consider wrapping the file writing logic with additional error handling to catch and handle any potential exceptions that may arise during the file operations.
Line range hint
421-421: Potential memory leak with raw pointer allocationIn the
BatchEntryconstructor and elsewhere, raw pointers are used without ownership semantics, which may lead to memory leaks if not managed correctly.Consider using smart pointers like
std::unique_ptrto manage dynamic memory allocations, ensuring proper deallocation and exception safety.src/Debug/Profiler.h (1)
94-98: Signal handlers might not be properly initializedThe signal handlers are initialized via
InitSignalHandlers, which requires an explicit call and may be omitted inadvertently.To prevent missing initialization, consider calling
InitSignalHandlerswithin theProfilerconstructor or ensure it's invoked during the application startup sequence.sandbox/src/SandboxApp.h (2)
10-10: Add a virtual destructor toSandboxAppSince
SandboxAppinherits fromEngine::Application, which may have virtual methods, it's advisable to declare a virtual destructor to ensure proper cleanup in derived classes.Update the destructor as follows:
- ~SandboxApp() = default; + virtual ~SandboxApp() = default;
3-4: Unnecessary inclusion ofEntryPoint.hin headerIncluding
EntryPoint.hin the header file may introduce unnecessary coupling and dependencies.Consider removing
#include "EntryPoint.h"fromSandboxApp.hand include it only in the implementation file if needed.sandbox/src/SandboxApp.cpp (1)
10-12: Consider using smart pointers in CreateApplication.The raw pointer returned by CreateApplication transfers ownership to the caller. Consider returning a unique_ptr to make ownership transfer explicit and prevent memory leaks.
-Application* CreateApplication() { - return new SandboxApp(); +std::unique_ptr<Application> CreateApplication() { + return std::make_unique<SandboxApp>();assets/shaders/lit.vert (2)
17-17: Optimize normal transformation calculation.Computing the inverse transpose matrix in the vertex shader is expensive. Consider:
- Computing it on CPU and passing as uniform if transform is static
- Using a 3x3 matrix uniform directly
- v_Normal = mat3(transpose(inverse(u_Transform))) * aNormal; + // Add uniform for normal matrix +uniform mat3 u_NormalMatrix; + // Use precomputed normal matrix + v_Normal = u_NormalMatrix * aNormal;
4-6: Consider using double precision for position attributes.For large-scale terrain rendering, consider using double precision for position attributes to prevent floating-point precision issues far from the origin.
-layout(location = 0) in vec3 aPosition; +layout(location = 0) in dvec3 aPosition;src/Noise/INoiseGenerator.h (2)
10-16: Consider documenting thread safety guarantees.The
noisemethod is marked as const, suggesting thread safety, but it would be beneficial to explicitly document thread safety guarantees in the interface documentation.
18-25: Document value range constraints for generateHeightmap.While
noise()documents its return value range,generateHeightmap()lacks similar constraints. Consider documenting:
- Expected range of returned heightmap values
- Validation requirements for width/height parameters (e.g., must be positive)
- Impact of the scale parameter on the output
src/Noise/ValueNoise/ValueNoise.h (1)
16-16: Consider using const parameters for rand2D method.The
rand2Dmethod parameters should be marked as const since they're not modified:- float rand2D(int x, int y) const; + float rand2D(const int x, const int y) const;src/Noise/SimplexNoise/SimplexNoise.h (2)
15-15: Optimize dot2 method signature.The
dot2method could be more efficient using const references:- static float dot2(const float* grad, float x, float y); + static float dot2(const float (&grad)[2], const float x, const float y);
17-20: Consider using constexpr for compile-time optimization.The static constants and gradients array could benefit from constexpr:
- unsigned char perm[512]; - static const float F2; - static const float G2; - static const int gradients[12][2]; + unsigned char perm[512]; // Document why 512 is used + static constexpr float F2 = 0.5f * (std::sqrt(3.0f) - 1.0f); // Document the mathematical significance + static constexpr float G2 = (3.0f - std::sqrt(3.0f)) / 6.0f; // Document the mathematical significance + static constexpr int gradients[12][2] = { /* ... */ }; // Document why 12 gradients are usedsrc/UI/ImGuiFlameGraph.h (2)
6-12: Consider adding validation methods to FlameGraphSettings.The settings struct could benefit from validation methods and constexpr defaults:
struct FlameGraphSettings { - float targetFPS = 60.0f; - bool showAverage = true; - bool showTargetFPS = true; - bool enableZoom = true; - float zoomLevel = 1.0f; - float panOffset = 0.0f; + static constexpr float DEFAULT_TARGET_FPS = 60.0f; + static constexpr float MIN_ZOOM = 0.1f; + static constexpr float MAX_ZOOM = 10.0f; + + float targetFPS = DEFAULT_TARGET_FPS; + bool showAverage = true; + bool showTargetFPS = true; + bool enableZoom = true; + float zoomLevel = 1.0f; + float panOffset = 0.0f; + + void validateAndClamp() { + targetFPS = std::max(1.0f, targetFPS); + zoomLevel = std::clamp(zoomLevel, MIN_ZOOM, MAX_ZOOM); + } };
14-19: Document parameter constraints and behavior.The
PlotFlamefunction should document:
- Valid ranges for min_value and max_value
- Behavior when values vector is empty
- Impact of height parameter on visualization
src/Noise/PerlinNoise/PerlinNoise.h (2)
7-12: Consider making the seed value configurable.The default seed value (1234) is hardcoded. Consider making it configurable through a constant or configuration file for better maintainability.
18-31: Implementation looks good with room for minor improvements.The private section is well-structured with appropriate use of
std::arrayand const-correctness. Consider:
- Making
PERM_SIZEa template parameter for more flexibility- Adding documentation for the mathematical methods
assets/shaders/lit.frag (1)
20-39: Consider adding light attenuation and normal validation.The Phong lighting implementation looks good but could be enhanced:
- Add light attenuation based on distance for more realistic lighting
- Add a safety check for normal vectors
void main() { // Ambient vec3 ambient = u_AmbientStrength * u_LightColor; // Diffuse - vec3 norm = normalize(v_Normal); + vec3 norm = normalize(v_Normal); + if (length(norm) < 0.99 || length(norm) > 1.01) { + // Handle invalid normal vector + norm = vec3(0.0, 1.0, 0.0); + } vec3 lightDir = normalize(u_LightPos - v_FragPos); + float distance = length(u_LightPos - v_FragPos); + float attenuation = 1.0 / (1.0 + 0.09 * distance + 0.032 * distance * distance); float diff = max(dot(norm, lightDir), 0.0); - vec3 diffuse = diff * u_LightColor; + vec3 diffuse = diff * u_LightColor * attenuation; // Specular vec3 viewDir = normalize(u_ViewPos - v_FragPos); vec3 reflectDir = reflect(-lightDir, norm); float spec = pow(max(dot(viewDir, reflectDir), 0.0), u_Shininess); - vec3 specular = u_SpecularStrength * spec * u_LightColor; + vec3 specular = u_SpecularStrength * spec * u_LightColor * attenuation;src/Shader/ShaderHotReload.cpp (2)
15-21: Consider using weak_ptr in lambda captures to prevent circular references.The lambda captures could potentially create circular references between the shader and the file watcher callbacks. Consider using
std::weak_ptrfor the shader parameter in the lambda captures.- m_Watcher.WatchFile(vertPath, [this, shader, vertPath, fragPath](const std::string&) { + auto weakShader = std::weak_ptr<Shader>(shader); + m_Watcher.WatchFile(vertPath, [this, weakShader, vertPath, fragPath](const std::string&) { + if (auto shader = weakShader.lock()) { ReloadShader(shader, vertPath, fragPath); + } });
32-32: Consider making the delay configurable.The hardcoded sleep delay of 100ms might not be optimal for all systems or file sizes. Consider making this configurable through a constant or configuration parameter.
+ static constexpr auto RELOAD_DELAY_MS = 100; // ... - std::this_thread::sleep_for(std::chrono::milliseconds(100)); + std::this_thread::sleep_for(std::chrono::milliseconds(RELOAD_DELAY_MS));src/Renderer/VertexArray.h (1)
6-11: Documentation should specify thread safety guarantees.The class documentation should specify whether the methods are thread-safe and any synchronization requirements.
/** * @brief Abstract base class for vertex array objects * * Represents a vertex array object that stores vertex attributes * and their configurations for rendering. + * + * @note This class is not thread-safe. All methods should be called from the same thread + * that owns the OpenGL context. */src/Noise/VoidNoise/VoidNoise.h (1)
Line range hint
37-38: Document the significance of PERM_SIZE constant.The magic number 256 for PERM_SIZE should be documented to explain its significance.
+ /** @brief Size of the permutation table. Must be a power of 2 for efficient bit masking */ static const int PERM_SIZE = 256;src/Core/FPSCounter.h (2)
36-37: Document the rationale behind buffer sizes.The magic numbers for SAMPLE_COUNT and HISTORY_SIZE should be documented to explain their significance and any performance implications.
+ /** @brief Number of samples to keep for statistical calculations. Higher values provide more accurate averages but use more memory */ static constexpr size_t SAMPLE_COUNT = 200; + /** @brief Number of frame times to keep for flame graph visualization. Balances detail with memory usage */ static constexpr size_t HISTORY_SIZE = 100;
31-33: Add noexcept specifier to getter methods.The getter methods are simple accessors that cannot throw exceptions and should be marked as such.
- const std::vector<float>& GetFrameTimeHistory() const { return m_FrameTimeHistory; } - size_t GetHistorySize() const { return HISTORY_SIZE; } + const std::vector<float>& GetFrameTimeHistory() const noexcept { return m_FrameTimeHistory; } + size_t GetHistorySize() const noexcept { return HISTORY_SIZE; }src/Shader/ShaderHotReload.h (2)
50-51: Consider thread safety for file watching operations.The FileWatcher and shader paths map could be accessed from multiple threads if Update() is called from a background thread. Consider adding synchronization.
Consider using:
+ mutable std::mutex m_Mutex; FileWatcher m_Watcher; std::unordered_map<std::shared_ptr<Shader>, std::pair<std::string, std::string>> m_ShaderPaths;
29-31: Consider adding shader change notifications.The current design doesn't notify other parts of the system when shaders are reloaded. Consider implementing the Observer pattern.
Consider adding:
using ShaderReloadCallback = std::function<void(const std::shared_ptr<Shader>&)>; void AddReloadCallback(const ShaderReloadCallback& callback); void RemoveReloadCallback(const ShaderReloadCallback& callback);src/VoxelChunk.h (2)
43-43: Add documentation for getBlockType method.The new method lacks documentation explaining its parameters and return value.
Add documentation:
+ /** + * @brief Determines block type based on world coordinates and height + * @param worldY World Y coordinate + * @param height Generated terrain height at this position + * @return BlockType The type of block at this position + */ BlockType getBlockType(int worldY, int height) const;
46-47: Consider combining voxel data and block types for better cache locality.The current design uses two separate arrays which could lead to cache misses when accessing both properties for the same voxel.
Consider using a struct to combine the data:
struct VoxelData { bool isActive; BlockType type; }; std::array<VoxelData, CHUNK_SIZE * CHUNK_SIZE * CHUNK_SIZE> m_Voxels;src/TerrainSystem/BlockTypes.h (2)
26-27: Define texture atlas dimensions as named constants.The texture size calculations use magic numbers (32x48). These should be named constants for better maintainability.
+ static constexpr int ATLAS_WIDTH = 32; + static constexpr int ATLAS_HEIGHT = 48; + static constexpr int TILE_SIZE = 16; - static constexpr float TEXTURE_SIZE = 0.5f; ///< 16/32 = 0.5 for horizontal - static constexpr float TEXTURE_V_SIZE = 0.333f; ///< 16/48 ≈ 0.333 for vertical + static constexpr float TEXTURE_SIZE = static_cast<float>(TILE_SIZE) / ATLAS_WIDTH; + static constexpr float TEXTURE_V_SIZE = static_cast<float>(TILE_SIZE) / ATLAS_HEIGHT;
35-58: Consider adding runtime validation for texture coordinates.The texture coordinates are hardcoded without validation. Consider adding debug assertions to ensure they're within bounds.
Add validation in debug builds:
#ifdef _DEBUG static_assert(BLOCK_TEXTURES[static_cast<size_t>(BlockType::COUNT)].topU <= 1.0f && BLOCK_TEXTURES[static_cast<size_t>(BlockType::COUNT)].topV <= 1.0f, "Texture coordinates must be normalized (0-1)"); #endifsrc/Renderer/Material.h (1)
Line range hint
52-58: Consider consolidating property maps for better cache locality.Multiple unordered_maps for different property types could lead to cache misses. Consider using a variant-based approach.
struct MaterialProperty { std::variant<float, int, bool, glm::vec4, glm::mat4, std::shared_ptr<Texture>> value; }; std::unordered_map<std::string, MaterialProperty> m_Properties;src/Core/FileWatcher.h (2)
21-28: Consider adding path validation and normalization.The
WatchFilemethod should validate and normalize the path before adding it to the watch list to prevent duplicate entries with different path representations.void WatchFile(const std::string& path, FileChangedCallback callback) { try { - auto time = std::filesystem::last_write_time(std::filesystem::path(path)); - m_WatchList[path] = FileInfo{time, callback}; + auto normalized_path = std::filesystem::canonical(path).string(); + auto time = std::filesystem::last_write_time(normalized_path); + m_WatchList[normalized_path] = FileInfo{time, callback}; } catch (const std::filesystem::filesystem_error& e) { LOG_ERROR("Failed to watch file {}: {}", path, e.what()); } }
34-46: Consider performance optimization for frequent updates.The
Updatemethod checks every file's timestamp on each call, which could be expensive for many files. Consider:
- Adding a configurable update frequency
- Using platform-specific file system events where available
+ private: + std::chrono::steady_clock::time_point m_LastUpdate; + static constexpr auto UPDATE_INTERVAL = std::chrono::milliseconds(100); + public: void Update() { + auto now = std::chrono::steady_clock::now(); + if (now - m_LastUpdate < UPDATE_INTERVAL) { + return; + } + m_LastUpdate = now; + for (auto& [path, info] : m_WatchList) { // ... existing code ... } }src/ImGui/ImGuiLayer.cpp (1)
68-73: Consider performance impact of viewport updates.The viewport update code could impact performance when many windows are present. Consider adding a frame skip mechanism for platform windows update.
+ private: + int m_FrameCount = 0; + static constexpr int VIEWPORT_UPDATE_INTERVAL = 2; + public: void End() { // ... existing code ... if (ImGui::GetIO().ConfigFlags & ImGuiConfigFlags_ViewportsEnable) { + if (++m_FrameCount % VIEWPORT_UPDATE_INTERVAL == 0) { GLFWwindow* backup_current_context = glfwGetCurrentContext(); ImGui::UpdatePlatformWindows(); ImGui::RenderPlatformWindowsDefault(); glfwMakeContextCurrent(backup_current_context); + } } }src/Renderer/RenderObject.h (1)
23-26: Consider adding move constructor and assignment operator.For better performance when handling temporary objects, add move semantics support.
+ /** @brief Move constructor */ + RenderObject(RenderObject&& other) noexcept = default; + + /** @brief Move assignment operator */ + RenderObject& operator=(RenderObject&& other) noexcept = default;src/VoxelTerrain.h (2)
4-4: Consider abstracting noise generation through an interface.The direct inclusion of
VoidNoise.hsuggests tight coupling. Consider introducing anINoiseGeneratorinterface to allow for different noise implementations.-#include "Noise/VoidNoise/VoidNoise.h" +#include "Noise/INoiseGenerator.h"
78-78: Add documentation for the debug method.The
SaveHeightmapDebugmethod lacks documentation. Add a docstring explaining its purpose, parameters, and when it should be used.+ /** + * @brief Saves a debug heightmap for a chunk + * @param chunkX X coordinate of the chunk + * @param chunkZ Z coordinate of the chunk + */ void SaveHeightmapDebug(int chunkX, int chunkZ);src/Noise/ValueNoise/ValueNoise.cpp (1)
27-45: Consider SIMD optimization for noise generation.The noise calculation could benefit from SIMD instructions for parallel processing of multiple noise values. Consider using compiler intrinsics or a SIMD library like xsimd.
src/UI/ImGuiOverlay.h (1)
55-59: Consider using in-class initializers for all members.For consistency and better initialization safety, consider using in-class initializers for all members, not just some.
- Window* m_Window; ///< Pointer to application window - Renderer* m_Renderer; ///< Pointer to renderer + Window* m_Window = nullptr; ///< Pointer to application window + Renderer* m_Renderer = nullptr; ///< Pointer to renderer bool m_ShowEventDebugger = true; ///< Event debugger visibility flag FPSCounter m_FPSCounter; ///< FPS counter ImGuiWidgetFlameGraph::FlameGraphSettings m_FlameGraphSettings; ///< Flame graph settingssrc/Renderer/Material.cpp (1)
Line range hint
27-45: Optimize property binding and add texture slot bounds checking.The current implementation could be more efficient and safer:
- Consider combining property loops to reduce map iterations
- Add bounds checking for texture slots
- Consider caching uniform locations
Consider this optimization:
void Material::Bind() { m_Shader->Bind(); + + // Cache max texture slots if not already cached + static int maxTextureSlots = 0; + if (maxTextureSlots == 0) { + glGetIntegerv(GL_MAX_COMBINED_TEXTURE_IMAGE_UNITS, &maxTextureSlots); + } // Apply all properties for (const auto& [name, value] : m_FloatProperties) m_Shader->SetFloat(name, value); for (const auto& [name, value] : m_IntProperties) m_Shader->SetInt(name, value); for (const auto& [name, value] : m_VectorProperties) m_Shader->SetVector4(name, value); for (const auto& [name, value] : m_MatrixProperties) m_Shader->SetMat4(name, value); // Bind textures m_TextureSlot = 0; for (const auto& [name, texture] : m_Textures) { + if (m_TextureSlot >= maxTextureSlots) { + LOG_ERROR("Exceeded maximum texture slots"); + break; + } texture->Bind(m_TextureSlot); m_Shader->SetInt(name, m_TextureSlot); m_TextureSlot++; } }src/Events/KeyCodes.h (1)
4-4: Consider abstracting GLFW dependency.Direct inclusion of GLFW headers might limit portability. Consider creating an input abstraction layer.
Create an input abstraction layer that maps between your engine's key codes and the underlying window system (GLFW, SDL, etc.).
src/TerrainSystem/TerrainSystem.h (1)
86-88: Document noise generator and use named constants.The noise generator lacks documentation and the class uses magic numbers for default values.
+ // Default values for terrain generation + static constexpr float DEFAULT_BASE_HEIGHT = 32.0f; + static constexpr float DEFAULT_HEIGHT_SCALE = 32.0f; + static constexpr float DEFAULT_NOISE_SCALE = 0.05f; + - float m_BaseHeight = 32.0f; ///< Base terrain height - float m_HeightScale = 32.0f; ///< Height variation scale - float m_NoiseScale = 0.05f; ///< Noise variation scale + float m_BaseHeight = DEFAULT_BASE_HEIGHT; ///< Base terrain height + float m_HeightScale = DEFAULT_HEIGHT_SCALE; ///< Height variation scale + float m_NoiseScale = DEFAULT_NOISE_SCALE; ///< Noise variation scale - // Noise generator - NoiseGenerator<VoidNoise> m_NoiseGen; + /// @brief Noise generator for terrain height calculation + /// @note Uses VoidNoise as the base noise implementation + NoiseGenerator<VoidNoise> m_NoiseGen;src/Renderer/Light.h (2)
19-28: Consider using builder pattern and add parameter validation.The Light class could benefit from a more flexible construction pattern and parameter validation.
Consider implementing a builder pattern and adding parameter validation:
class Light { public: class Builder { public: Builder& setPosition(const glm::vec3& pos) { position = pos; return *this; } // Similar methods for other properties Light build() const { return Light(position, color, ambient, specular, shininess); } private: glm::vec3 position{0.0f}; glm::vec3 color{1.0f}; float ambient{0.1f}; float specular{0.5f}; float shininess{32.0f}; }; static Builder create() { return Builder(); } // ... rest of the class };
80-86: Add factory methods for common light types.Consider adding static factory methods for common light types (directional, point, spot).
// Add these static methods to the Light class static Light createDirectionalLight(const glm::vec3& direction, const glm::vec3& color = glm::vec3(1.0f)); static Light createPointLight(const glm::vec3& position, const glm::vec3& color = glm::vec3(1.0f)); static Light createSpotLight(const glm::vec3& position, const glm::vec3& direction, float cutoffAngle, const glm::vec3& color = glm::vec3(1.0f));src/Events/EventDispatcher.h (1)
17-34: Consider adding noexcept specifications.The
EventDispatcherimplementation looks good, but could benefit from marking the constructor andDispatchmethod asnoexceptsince they don't throw exceptions.- EventDispatcher(Event& event) : m_Event(event) {} + EventDispatcher(Event& event) noexcept : m_Event(event) {} template<typename T, typename F> - bool Dispatch(const F& func) { + bool Dispatch(const F& func) noexcept {src/Core/TaskSystem.h (1)
68-69: Consider adding task queue size limit.The current implementation allows unlimited task queuing which could lead to memory issues under heavy load.
Consider adding a maximum queue size and implementing a way to handle queue overflow (e.g., blocking or returning a failure status).
src/Noise/VoidNoise/VoidNoise.cpp (1)
135-138: Remove or improve debug logging in heightmap generation.Debug logging in the heightmap generation loop could significantly impact performance when generating large heightmaps.
Consider moving this to a separate debug method or using a compile-time flag:
+#ifdef DEBUG_HEIGHTMAP if (x == 0 && y < 2) { - LOG_TRACE_CONCAT("Heightmap value at (", x, ",", y, "): ", heightmap[y * width + x]); + LOG_TRACE("Heightmap value at ({},{}): {}", x, y, heightmap[y * width + x]); } +#endifsrc/Core/Utils/BMPWriter.h (1)
32-35: Consider using constants for magic numbers.The values
54(header size),256(number of colors), and4(bytes per color) should be defined as named constants at the class level for better maintainability and readability.class BMPWriter { + static constexpr uint32_t BMP_HEADER_SIZE = 54; + static constexpr uint32_t PALETTE_COLORS = 256; + static constexpr uint32_t BYTES_PER_COLOR = 4; public: static void SaveGrayscaleBMP(const char* filename, const std::vector<uint8_t>& data, int width, int height) { - uint32_t paletteSize = 256 * 4; - uint32_t pixelDataOffset = 54 + paletteSize; + uint32_t paletteSize = PALETTE_COLORS * BYTES_PER_COLOR; + uint32_t pixelDataOffset = BMP_HEADER_SIZE + paletteSize;src/Core/Utils/AssertLib.h (3)
82-87: Consider using std::format or fmt library for message formatting.The current string concatenation could be improved using modern formatting libraries for better performance and maintainability.
+#include <format> inline std::string createMessage(const std::string& file, int line, const std::string& msg) { - std::ostringstream oss; - oss << "Assertion failed in " << file << ":" << line << ". " << msg; - return oss.str(); + return std::format("Assertion failed in {}:{}: {}", file, line, msg); }
96-103: Add support for custom comparators in assertEquals.The current implementation only uses operator!=. Consider adding support for custom comparison functions.
- template <typename T> - void assertEquals(const T& expected, const T& actual, const std::string& file, int line) { + template <typename T, typename Compare = std::equal_to<T>> + void assertEquals(const T& expected, const T& actual, const std::string& file, int line, Compare comp = Compare{}) { - if (expected != actual) { + if (!comp(expected, actual)) {
112-116: Enhance type comparison error messages.The current type comparison uses typeid().name() which may not be human-readable. Consider using more descriptive type names.
+#include <cxxabi.h> if (typeid(value) != typeid(T)) { std::ostringstream oss; - oss << "Expected type: " << typeid(T).name() << ", but got type: " << typeid(value).name(); + int status; + char* expectedName = abi::__cxa_demangle(typeid(T).name(), nullptr, nullptr, &status); + char* actualName = abi::__cxa_demangle(typeid(value).name(), nullptr, nullptr, &status); + oss << "Expected type: " << (expectedName ? expectedName : typeid(T).name()) + << ", but got type: " << (actualName ? actualName : typeid(value).name()); + free(expectedName); + free(actualName);src/VoxelTerrain.cpp (2)
22-25: Consider making debug functionality configurable.The debug heightmap generation should be controlled by a configuration flag rather than being hardcoded.
+ // Add to class header: + bool m_EnableDebugHeightmap = false; + // Debug: Save heightmap when generating chunk at y=0 - if (chunkY == 0) { + if (m_EnableDebugHeightmap && chunkY == 0) {
123-125: Use std::format for filename formatting and add path safety.The current filename formatting using snprintf could be improved, and the path handling needs to be more robust.
- char filename[100]; - snprintf(filename, sizeof(filename), "heightmap_chunk_%d_%d.bmp", chunkX, chunkZ); - BMPWriter::SaveGrayscaleBMP(filename, heightData, mapSize, mapSize); + const auto filename = std::format("debug/heightmaps/chunk_{}_{}.bmp", chunkX, chunkZ); + std::filesystem::create_directories(std::filesystem::path(filename).parent_path()); + BMPWriter::SaveGrayscaleBMP(filename.c_str(), heightData, mapSize, mapSize);src/Core/AssetManager.h (1)
47-47: Consider using string formatting instead of concatenation.Using
LOG_ERROR_CONCATfor error messages might not be the most maintainable approach. Consider using a format string instead.-LOG_ERROR_CONCAT("Failed to load resource: ", path); +LOG_ERROR("Failed to load resource: {}", path);src/Renderer/Renderer.h (1)
149-162: Consider adding parameter validation.While the viewport and clear screen methods are well-documented, they lack parameter validation.
void SetViewport(int x, int y, int width, int height) const { + if (width <= 0 || height <= 0) { + LOG_ERROR("Invalid viewport dimensions: {}x{}", width, height); + return; + } glViewport(x, y, width, height); }json-viewer/visualiser.ipynb (1)
9-10: Consider using requirements.txt.Instead of installing packages in the notebook, consider creating a
requirements.txtfile for better dependency management.Create a new file
requirements.txt:pandas plotly nbformatsrc/UI/ImGuiFlameGraph.cpp (2)
5-10: Consider using std::clamp.Instead of implementing a custom
Clampfunction, consider usingstd::clampfrom the C++ standard library.-template<typename T> -T Clamp(T value, T min, T max) { - if (value < min) return min; - if (value > max) return max; - return value; -} +#include <algorithm> +// Use std::clamp instead
86-119: Consider performance optimization for large datasets.The bar rendering loop might be inefficient for large datasets. Consider implementing culling for off-screen bars.
for (size_t i = 0; i < values.size(); i++) { + // Skip bars that are completely off-screen + float bar_x = start_x + (i * width_per_item); + if (bar_x + width_per_item < canvas_pos.x || bar_x > canvas_pos.x + canvas_size.x) + continue; + float normalized_value = (values[i] - min_value) / (max_value - min_value);src/UI/ImGuiOverlay.cpp (2)
14-16: Optimize constructor initialization.Consider moving
m_FlameGraphSettingsinitialization to the initialization list for better performance.- ImGuiOverlay::ImGuiOverlay(Window* window) - : m_Window(window), m_Renderer(&Renderer::Get()) { - // Initialize flame graph settings with defaults - m_FlameGraphSettings = ImGuiWidgetFlameGraph::FlameGraphSettings(); - } + ImGuiOverlay::ImGuiOverlay(Window* window) + : m_Window(window) + , m_Renderer(&Renderer::Get()) + , m_FlameGraphSettings(ImGuiWidgetFlameGraph::FlameGraphSettings()) + { + }🧰 Tools
🪛 cppcheck (2.10-2)
[performance] 16-16: Variable 'm_FlameGraphSettings' is assigned in constructor body. Consider performing initialization in initialization list.
(useInitializationList)
149-152: Consider using a named constant for the frame limit.The magic number 1000 could be replaced with a named constant for better maintainability.
+ static constexpr int MAX_FRAMES_TO_PROFILE = 1000; static int framesToProfile = 60; ImGui::SetNextItemWidth(100); ImGui::InputInt("Frames to Profile", &framesToProfile); - framesToProfile = std::max(1, std::min(framesToProfile, 1000)); // Clamp between 1-1000 + framesToProfile = std::max(1, std::min(framesToProfile, MAX_FRAMES_TO_PROFILE));src/Shader/DefaultShaders.h (2)
19-23: Consider extracting shader paths to constants.The shader paths are duplicated across multiple methods in this file. Consider extracting them to static const variables to improve maintainability and reduce the risk of typos.
namespace DefaultShaders { + static const struct ShaderPaths { + static constexpr auto BASIC_MVP_VERT = "assets/shaders/basic_mvp.vert"; + static constexpr auto COLOR_FRAG = "assets/shaders/color.frag"; + static constexpr auto SIMPLE_COLOR_FRAG = "assets/shaders/simple_color.frag"; + static constexpr auto TEXTURED_VERT = "assets/shaders/textured.vert"; + static constexpr auto TEXTURED_FRAG = "assets/shaders/textured.frag"; + } PATHS;
17-32: Consider leveraging preloaded shaders in existing load methods.The individual shader loading methods (LoadBasicShader, LoadSimpleColorShader, etc.) could benefit from using the preloaded shaders instead of creating new instances each time.
Consider this pattern for the existing load methods:
static std::shared_ptr<Shader> LoadBasicShader() { static const std::string vert = "assets/shaders/basic_mvp.vert"; static const std::string frag = "assets/shaders/color.frag"; auto& manager = AssetManager::Get(); // Try to get preloaded shader first if (auto shader = manager.GetResource<Shader>(AssetManager::GenerateShaderKey(vert, frag))) { return shader; } // Fall back to creating new instance return std::shared_ptr<Shader>(Shader::CreateFromFiles(vert, frag)); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
assets/textures/terrain_atlas.pngis excluded by!**/*.png
📒 Files selected for processing (68)
.github/workflows/build.yml(1 hunks).gitignore(1 hunks).gitmodules(1 hunks)CMakeLists.txt(5 hunks)assets/shaders/lit.frag(1 hunks)assets/shaders/lit.vert(1 hunks)external/imgui(1 hunks)imgui.ini(2 hunks)json-viewer/visualiser.ipynb(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/Camera/PerspectiveCamera.h(2 hunks)src/Core/AssetManager.h(8 hunks)src/Core/FPSCounter.cpp(1 hunks)src/Core/FPSCounter.h(1 hunks)src/Core/FileWatcher.h(1 hunks)src/Core/TaskSystem.h(4 hunks)src/Core/Utils/AssertLib.h(1 hunks)src/Core/Utils/BMPWriter.h(1 hunks)src/Debug/Profiler.cpp(2 hunks)src/Debug/Profiler.h(5 hunks)src/EntryPoint.h(1 hunks)src/Events/EventDispatcher.h(3 hunks)src/Events/KeyCodes.h(2 hunks)src/ImGui/ImGuiLayer.cpp(2 hunks)src/ImGui/ImGuiLayer.h(1 hunks)src/Input/InputSystem.cpp(3 hunks)src/Input/InputSystem.h(2 hunks)src/Noise/INoiseGenerator.h(1 hunks)src/Noise/NoiseGenerator.h(1 hunks)src/Noise/PerlinNoise/PerlinNoise.cpp(1 hunks)src/Noise/PerlinNoise/PerlinNoise.h(1 hunks)src/Noise/SimplexNoise/SimplexNoise.cpp(1 hunks)src/Noise/SimplexNoise/SimplexNoise.h(1 hunks)src/Noise/ValueNoise/ValueNoise.cpp(1 hunks)src/Noise/ValueNoise/ValueNoise.h(1 hunks)src/Noise/VoidNoise/VoidNoise.cpp(3 hunks)src/Noise/VoidNoise/VoidNoise.h(3 hunks)src/Renderer/Light.h(1 hunks)src/Renderer/Material.cpp(1 hunks)src/Renderer/Material.h(1 hunks)src/Renderer/RenderAPI.h(1 hunks)src/Renderer/RenderObject.h(1 hunks)src/Renderer/Renderer.cpp(5 hunks)src/Renderer/Renderer.h(1 hunks)src/Renderer/Texture.cpp(4 hunks)src/Renderer/Texture.h(1 hunks)src/Renderer/VertexArray.cpp(1 hunks)src/Renderer/VertexArray.h(1 hunks)src/Shader/DefaultShaders.h(2 hunks)src/Shader/Shader.cpp(2 hunks)src/Shader/Shader.h(3 hunks)src/Shader/ShaderHotReload.cpp(1 hunks)src/Shader/ShaderHotReload.h(1 hunks)src/TerrainSystem/BlockTypes.h(1 hunks)src/TerrainSystem/TerrainSystem.cpp(6 hunks)src/TerrainSystem/TerrainSystem.h(2 hunks)src/UI/ImGuiFlameGraph.cpp(1 hunks)src/UI/ImGuiFlameGraph.h(1 hunks)src/UI/ImGuiOverlay.cpp(4 hunks)src/UI/ImGuiOverlay.h(2 hunks)src/VoxelChunk.cpp(1 hunks)src/VoxelChunk.h(2 hunks)src/VoxelTerrain.cpp(3 hunks)src/VoxelTerrain.h(3 hunks)src/pch.h(1 hunks)
✅ Files skipped from review due to trivial changes (4)
- .gitmodules
- src/Renderer/VertexArray.cpp
- external/imgui
- .gitignore
🧰 Additional context used
🪛 cppcheck (2.10-2)
src/VoxelChunk.cpp
[error] 54-54: Out of bounds access in 'heightMap[x+zCHUNK_SIZE]', if 'heightMap' size is 32 and 'x+zCHUNK_SIZE' is 1023
(containerOutOfBounds)
[error] 61-61: Out of bounds access in 'heightMap[x+zCHUNK_SIZE]', if 'heightMap' size is 32 and 'x+zCHUNK_SIZE' is 1023
(containerOutOfBounds)
src/UI/ImGuiOverlay.cpp
[performance] 16-16: Variable 'm_FlameGraphSettings' is assigned in constructor body. Consider performing initialization in initialization list.
(useInitializationList)
src/TerrainSystem/TerrainSystem.cpp
[performance] 26-26: Variable 'm_TerrainTexture' is assigned in constructor body. Consider performing initialization in initialization list.
(useInitializationList)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build and analyze with SonarQube
🔇 Additional comments (36)
src/Renderer/RenderAPI.h (1)
28-28: Provide implementation for theCreatemethod or clarify its usage.The static method
RenderAPI::Create()is declared but not defined in this file. Ensure that an implementation is provided in a corresponding source file, or provide documentation on how and where it should be implemented.src/Noise/SimplexNoise/SimplexNoise.cpp (1)
63-66:⚠️ Potential issueCorrect permutation index calculations to prevent out-of-bounds access.
In the
noisefunction, the permutation indices may exceed the valid range. Ensure index wrapping is correctly handled using modulo operations.Modify the permutation access to correctly wrap indices:
-int gi0 = perm[(i + perm[j & 255]) & 255] % 12; -int gi1 = perm[(i + i1 + perm[(j + j1) & 255]) & 255] % 12; -int gi2 = perm[(i + 1 + perm[(j + 1) & 255]) & 255] % 12; +int gi0 = perm[(i + perm[j & 255]) & 511] % 12; +int gi1 = perm[(i + i1 + perm[(j + j1) & 255]) & 511] % 12; +int gi2 = perm[(i + 1 + perm[(j + 1) & 255]) & 511] % 12;Ensure that all indices into
permuse& 511ifpermhas 512 elements.Likely invalid or redundant comment.
src/VoxelChunk.cpp (1)
79-91: Good encapsulation of block type determination logicThe addition of the
getBlockTypemethod improves code readability and maintainability by encapsulating the block type determination based onworldYandheight. This separation of concerns makes the code more modular and easier to manage.src/Shader/Shader.h (1)
79-80: Improved resource management withstd::shared_ptrChanging the return type of
CreateFromFilesand introducingCreateFromSourceto returnstd::shared_ptr<Shader>enhances memory management by using smart pointers, reducing the risk of memory leaks.src/Application.h (4)
98-100: Inline definition ofSetViewportThe
SetViewportmethod now has an inline definition that directly callsm_Renderer->SetViewport(). This change improves performance by reducing function call overhead.
107-107: Ensure proper initialization ofm_RendererSince
m_Rendereris now astd::unique_ptr, ensure that it is allocated before use to prevent null pointer dereferences throughout the application.Confirm that
m_Rendereris initialized in theApplicationconstructor or initialization method.
169-169: Effective use ofFPSCounterfor performance monitoringReplacing individual FPS tracking variables with the
FPSCounterclass simplifies performance monitoring and improves code organization.
175-175: Addition ofm_Lightmember for lighting capabilitiesIntroducing
m_Lightenhances the application's rendering capabilities by supporting light management. Ensure thatm_Lightis properly initialized and integrated with the rendering system.src/Input/InputSystem.h (1)
111-111: Verify the value ofm_SlowMultiplier
m_SlowMultiplieris set to1.0f, which might not actually slow down movement. Consider setting it to a value less than1.0fto achieve a slowing effect.src/Debug/Profiler.cpp (2)
291-291: Return type mismatch between declaration and definition ofEndFrameThe implementation of
Profiler::EndFrame()returns abool, but there is a comment indicating to remove the implementation and keep only the declaration.Ensure that the function declaration matches the definition and that any comments or instructions are consistent with the intended implementation.
293-361: Thread safety concerns with custom memory poolThe
ProfileDataPoolclass manages memory allocation with manual locking. There might be risks of concurrency issues if accessed concurrently from multiple threads.Ensure that the locking mechanisms in
ProfileDataPoolare sufficient for thread safety. Alternatively, evaluate using thread-safe standard containers or memory allocators provided by the standard library to handle concurrency more robustly.sandbox/src/SandboxApp.cpp (1)
3-7: Add error handling for terrain system initialization.The terrain system initialization could fail, but there's no error handling. Consider adding try-catch or error checking.
src/Noise/ValueNoise/ValueNoise.h (1)
19-20: Validate permutation table size initialization.The permutation table vector 'p' should be initialized with PERMUTATION_SIZE elements. Consider adding size validation in the constructor.
src/pch.h (1)
42-45: LGTM! Good organization of utility headers.The new includes are well-organized and follow a logical structure:
- Core utilities (Logging, AssertLib)
- Debug utilities (Profiler)
- External dependencies (json.hpp)
assets/shaders/lit.frag (1)
1-18: Well-organized shader inputs and uniforms.The declarations follow good practices:
- Clear naming convention with
v_prefix for vertex inputs andu_for uniforms- Logical grouping of related uniforms
src/Renderer/Material.h (1)
45-49: LGTM! Well-implemented texture retrieval.The GetTexture method properly handles the case when the texture is not found by returning nullptr.
src/ImGui/ImGuiLayer.cpp (1)
37-40: Verify OpenGL version compatibility.The OpenGL version has been updated to 410. Ensure all target platforms support this version.
✅ Verification successful
OpenGL version 410 is compatible with the codebase
The codebase already supports OpenGL versions up to 450 in some shaders, making version 410 a safe choice for ImGui. However, consider:
- Adding explicit OpenGL version checks during initialization
- Standardizing shader versions across the codebase (currently mixed 330/410/450)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any OpenGL version specifications in the codebase rg -A 2 "glsl_version|#version [0-9]+" --type cpp --type glslLength of output: 92
Script:
#!/bin/bash # Search for OpenGL version references without file type restriction rg -i "opengl|glsl.*version|#version" -A 2 # Also check for any ImGui initialization related files fd -e cpp -e h "ImGui" --exec grep -i "opengl\|version" {} \;Length of output: 21456
src/Renderer/Texture.h (1)
8-12: Excellent documentation structure!The class documentation is comprehensive and follows good practices.
src/Camera/PerspectiveCamera.h (1)
29-33: LGTM! Well-documented getter methods with proper const-correctness.The new getter methods are implemented efficiently using const references and follow the existing documentation style.
Also applies to: 69-70
src/Core/AssetManager.h (4)
10-14: Great documentation!The class documentation clearly describes the purpose and implementation details of the
AssetManagerclass.
Line range hint
56-95: Well-designed resource management system!The implementation of frequently used assets management is robust:
- Clear documentation
- Proper reference counting
- Cache management between regular and frequent resources
128-139: Excellent async loading implementation!The async loading implementation is well-designed:
- Uses std::future for asynchronous operations
- Maintains consistency with synchronous loading
- Properly documented
97-99: Verify the texture preloading migration.The texture preloading responsibility has been moved to
TextureManager.src/Renderer/Renderer.h (2)
18-60: Excellent structure documentation!The documentation for
RenderCommand,PreprocessedRenderCommand, andGLCommandstructures is comprehensive and clear. The use of doxygen-style comments with proper descriptions enhances code maintainability.
69-146: Well-designed camera system!The camera management implementation is robust:
- Clear separation between orthographic and perspective cameras
- Type-safe camera switching
- Proper documentation
json-viewer/visualiser.ipynb (1)
46-187: Excellent visualization implementation!The visualizations are well-designed:
- Clear and informative plots
- Good use of color coding
- Interactive features
- Proper labeling and tooltips
src/UI/ImGuiFlameGraph.cpp (1)
47-58: Well-implemented interaction handling!The zoom and pan functionality is well-implemented:
- Smooth zoom using mouse wheel
- Intuitive panning with mouse drag
- Proper bounds checking
src/UI/ImGuiOverlay.cpp (1)
Line range hint
36-114: LGTM! Well-structured UI implementation.The changes demonstrate good UI practices:
- Efficient window sizing with viewport-relative constraints
- Smart initial positioning logic
- Well-organized flame graph settings with compact layout
imgui.ini (1)
Line range hint
4-72: Configuration changes look good.These are auto-generated ImGui window configuration changes.
.github/workflows/build.yml (1)
31-31: LGTM! Good optimization.Adding ccache will improve build times by caching compiled objects.
CMakeLists.txt (4)
10-17: LGTM! Good platform-specific configuration.The changes properly handle:
- Build optimization with ccache
- Platform-specific toolchain configuration
23-29: LGTM! Well-structured GLAD integration.The GLAD library is properly configured as a static library with correct include paths.
Line range hint
43-90: LGTM! Well-organized source structure.Good organization of:
- Engine sources into logical groups
- Library configuration with proper visibility
- Dependencies configuration
93-100: LGTM! Good sandbox setup.The sandbox executable is properly configured and linked with the engine library.
src/Shader/DefaultShaders.h (2)
4-4: LGTM! Include statement is properly placed.The AssetManager include is correctly added and necessary for the new shader preloading functionality.
20-22: Verify shader file paths exist in the repository.Let's ensure all referenced shader files exist in the correct locations.
✅ Verification successful
All shader files verified and found in correct locations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if all referenced shader files exist shaders=( "assets/shaders/basic_mvp.vert" "assets/shaders/color.frag" "assets/shaders/simple_color.frag" "assets/shaders/textured.vert" "assets/shaders/textured.frag" ) for shader in "${shaders[@]}"; do if ! fd -t f "^$(basename "$shader")$" | grep -q "$shader"; then echo "Missing shader: $shader" fi doneLength of output: 874
Script:
#!/bin/bash # First check if assets/shaders directory exists echo "Checking for assets/shaders directory:" if [ -d "assets/shaders" ]; then echo "✓ assets/shaders directory exists" else echo "✗ assets/shaders directory not found" fi echo -e "\nSearching for shader files:" shaders=( "basic_mvp.vert" "color.frag" "simple_color.frag" "textured.vert" "textured.frag" ) for shader in "${shaders[@]}"; do echo -n "Looking for $shader: " find . -type f -name "$shader" 2>/dev/null doneLength of output: 1199
|
|
||
| // Duplicate the permutation table to avoid overflow | ||
| for(int i = 0; i < 512; i++) { | ||
| perm[i] = p[i & 255]; |
There was a problem hiding this comment.
Ensure proper sizing of the permutation array to prevent overflow.
When populating the perm array, you use indices up to 511, but perm must be sized appropriately to hold all values without overflow.
Declare the perm array with a size of 512:
unsigned char perm[512];Also, ensure that the p vector has 256 elements:
std::vector<unsigned char> p(256);|
|
||
| float SimplexNoise::grad(int hash, float x, float y) { | ||
| const int h = hash & 11; | ||
| return dot2(reinterpret_cast<const float*>(gradients[h]), x, y); |
There was a problem hiding this comment.
Avoid invalid type casting between int and float.
In the grad function, you're casting an int array to a float pointer, which leads to undefined behavior due to type punning between int and float.
To fix this issue, change the gradients array to store float values:
-const int SimplexNoise::gradients[12][2] = {
+const float SimplexNoise::gradients[12][2] = {
{1, 1}, {-1, 1}, {1, -1}, {-1, -1},
{1, 0}, {-1, 0}, {1, 0}, {-1, 0},
{0, 1}, {0, -1}, {0, 1}, {0, -1}
};Committable suggestion skipped: line range outside the PR's diff.
| p[i] = p[i + PERM_SIZE] = perm[i]; | ||
| } | ||
|
|
There was a problem hiding this comment.
Prevent out-of-bounds access by correctly sizing the permutation array p.
You are assigning values to p[i + PERM_SIZE], which means p should be of size 2 * PERM_SIZE. If p is only sized PERM_SIZE, this will cause out-of-bounds access.
Ensure that p is declared with double the size:
int p[2 * PERM_SIZE];| int padding = (4 - (width % 4)) % 4; | ||
| std::vector<uint8_t> paddingBytes(padding, 0); | ||
|
|
||
| // Write pixel data (bottom-up) | ||
| for (int y = height - 1; y >= 0; y--) { | ||
| file.write(reinterpret_cast<const char*>(&data[y * width]), width); | ||
| if (padding > 0) { | ||
| file.write(reinterpret_cast<const char*>(paddingBytes.data()), padding); | ||
| } |
There was a problem hiding this comment.
Add input validation for width and height parameters.
The method should validate that the input dimensions match the data size before proceeding with file writing.
static void SaveGrayscaleBMP(const char* filename, const std::vector<uint8_t>& data, int width, int height) {
+ if (data.size() < static_cast<size_t>(width * height)) {
+ throw std::invalid_argument("Input data size is smaller than width * height");
+ }
+ if (width <= 0 || height <= 0) {
+ throw std::invalid_argument("Width and height must be positive values");
+ }Committable suggestion skipped: line range outside the PR's diff.
| void VoxelTerrain::SaveHeightmapDebug(int chunkX, int chunkZ) { | ||
| const int mapSize = VoxelChunk::CHUNK_SIZE * 3; | ||
| std::vector<uint8_t> heightData(mapSize * mapSize); | ||
|
|
||
| for (int x = 0; x < mapSize; x++) { | ||
| for (int z = 0; z < mapSize; z++) { | ||
| float wx = ((chunkX * VoxelChunk::CHUNK_SIZE) + x) * m_NoiseScale; | ||
| float wz = ((chunkZ * VoxelChunk::CHUNK_SIZE) + z) * m_NoiseScale; | ||
|
|
||
| // Generate noise exactly as done in chunk generation | ||
| float continentNoise = m_NoiseGenerator.noise(wx * 0.5f, wz * 0.5f) * 2.0f - 1.0f; | ||
| float terrainNoise = (m_NoiseGenerator.noise(wx * 2.0f, wz * 2.0f) * 2.0f - 1.0f) * 0.5f; | ||
| float detailNoise = (m_NoiseGenerator.noise(wx * 4.0f, wz * 4.0f) * 2.0f - 1.0f) * 0.25f; | ||
|
|
||
| // Calculate height as done in chunk generation | ||
| float combinedNoise = continentNoise + terrainNoise + detailNoise; | ||
| float height = ((combinedNoise + 1.0f) * 32.0f) + m_WaterLevel; | ||
|
|
||
| // Convert to grayscale (0-255) | ||
| // Map height range 0-128 to 0-255 | ||
| uint8_t pixelValue = static_cast<uint8_t>(std::min(255.0f, std::max(0.0f, (height / 128.0f) * 255.0f))); | ||
| heightData[x + z * mapSize] = pixelValue; | ||
|
|
||
| // Debug first few values | ||
| if (x < 2 && z < 2) { | ||
| LOG_TRACE_CONCAT("Heightmap ",x,z,", Height: ",height," Pixel: ",pixelValue); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| char filename[100]; | ||
| snprintf(filename, sizeof(filename), "heightmap_chunk_%d_%d.bmp", chunkX, chunkZ); | ||
| BMPWriter::SaveGrayscaleBMP(filename, heightData, mapSize, mapSize); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize heightmap generation and improve error handling.
The heightmap generation has several areas for improvement:
- Memory allocation could be optimized
- Magic numbers should be constants
- File operations need error handling
void VoxelTerrain::SaveHeightmapDebug(int chunkX, int chunkZ) {
+ static constexpr int MAP_SCALE = 3;
- const int mapSize = VoxelChunk::CHUNK_SIZE * 3;
+ const int mapSize = VoxelChunk::CHUNK_SIZE * MAP_SCALE;
+
+ // Pre-calculate world coordinates for better performance
+ const float baseWX = chunkX * VoxelChunk::CHUNK_SIZE * m_NoiseScale;
+ const float baseWZ = chunkZ * VoxelChunk::CHUNK_SIZE * m_NoiseScale;
std::vector<uint8_t> heightData(mapSize * mapSize);
for (int x = 0; x < mapSize; x++) {
for (int z = 0; z < mapSize; z++) {
- float wx = ((chunkX * VoxelChunk::CHUNK_SIZE) + x) * m_NoiseScale;
- float wz = ((chunkZ * VoxelChunk::CHUNK_SIZE) + z) * m_NoiseScale;
+ float wx = baseWX + (x * m_NoiseScale);
+ float wz = baseWZ + (z * m_NoiseScale);📝 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 VoxelTerrain::SaveHeightmapDebug(int chunkX, int chunkZ) { | |
| const int mapSize = VoxelChunk::CHUNK_SIZE * 3; | |
| std::vector<uint8_t> heightData(mapSize * mapSize); | |
| for (int x = 0; x < mapSize; x++) { | |
| for (int z = 0; z < mapSize; z++) { | |
| float wx = ((chunkX * VoxelChunk::CHUNK_SIZE) + x) * m_NoiseScale; | |
| float wz = ((chunkZ * VoxelChunk::CHUNK_SIZE) + z) * m_NoiseScale; | |
| // Generate noise exactly as done in chunk generation | |
| float continentNoise = m_NoiseGenerator.noise(wx * 0.5f, wz * 0.5f) * 2.0f - 1.0f; | |
| float terrainNoise = (m_NoiseGenerator.noise(wx * 2.0f, wz * 2.0f) * 2.0f - 1.0f) * 0.5f; | |
| float detailNoise = (m_NoiseGenerator.noise(wx * 4.0f, wz * 4.0f) * 2.0f - 1.0f) * 0.25f; | |
| // Calculate height as done in chunk generation | |
| float combinedNoise = continentNoise + terrainNoise + detailNoise; | |
| float height = ((combinedNoise + 1.0f) * 32.0f) + m_WaterLevel; | |
| // Convert to grayscale (0-255) | |
| // Map height range 0-128 to 0-255 | |
| uint8_t pixelValue = static_cast<uint8_t>(std::min(255.0f, std::max(0.0f, (height / 128.0f) * 255.0f))); | |
| heightData[x + z * mapSize] = pixelValue; | |
| // Debug first few values | |
| if (x < 2 && z < 2) { | |
| LOG_TRACE_CONCAT("Heightmap ",x,z,", Height: ",height," Pixel: ",pixelValue); | |
| } | |
| } | |
| } | |
| char filename[100]; | |
| snprintf(filename, sizeof(filename), "heightmap_chunk_%d_%d.bmp", chunkX, chunkZ); | |
| BMPWriter::SaveGrayscaleBMP(filename, heightData, mapSize, mapSize); | |
| void VoxelTerrain::SaveHeightmapDebug(int chunkX, int chunkZ) { | |
| static constexpr int MAP_SCALE = 3; | |
| const int mapSize = VoxelChunk::CHUNK_SIZE * MAP_SCALE; | |
| // Pre-calculate world coordinates for better performance | |
| const float baseWX = chunkX * VoxelChunk::CHUNK_SIZE * m_NoiseScale; | |
| const float baseWZ = chunkZ * VoxelChunk::CHUNK_SIZE * m_NoiseScale; | |
| std::vector<uint8_t> heightData(mapSize * mapSize); | |
| for (int x = 0; x < mapSize; x++) { | |
| for (int z = 0; z < mapSize; z++) { | |
| float wx = baseWX + (x * m_NoiseScale); | |
| float wz = baseWZ + (z * m_NoiseScale); | |
| // Generate noise exactly as done in chunk generation | |
| float continentNoise = m_NoiseGenerator.noise(wx * 0.5f, wz * 0.5f) * 2.0f - 1.0f; | |
| float terrainNoise = (m_NoiseGenerator.noise(wx * 2.0f, wz * 2.0f) * 2.0f - 1.0f) * 0.5f; | |
| float detailNoise = (m_NoiseGenerator.noise(wx * 4.0f, wz * 4.0f) * 2.0f - 1.0f) * 0.25f; | |
| // Calculate height as done in chunk generation | |
| float combinedNoise = continentNoise + terrainNoise + detailNoise; | |
| float height = ((combinedNoise + 1.0f) * 32.0f) + m_WaterLevel; | |
| // Convert to grayscale (0-255) | |
| // Map height range 0-128 to 0-255 | |
| uint8_t pixelValue = static_cast<uint8_t>(std::min(255.0f, std::max(0.0f, (height / 128.0f) * 255.0f))); | |
| heightData[x + z * mapSize] = pixelValue; | |
| // Debug first few values | |
| if (x < 2 && z < 2) { | |
| LOG_TRACE_CONCAT("Heightmap ",x,z,", Height: ",height," Pixel: ",pixelValue); | |
| } | |
| } | |
| } | |
| char filename[100]; | |
| snprintf(filename, sizeof(filename), "heightmap_chunk_%d_%d.bmp", chunkX, chunkZ); | |
| BMPWriter::SaveGrayscaleBMP(filename, heightData, mapSize, mapSize); |
| /** @brief Binds the texture to a specific texture slot | ||
| * @param slot The texture unit slot to bind to | ||
| */ | ||
| void Texture::Bind(uint32_t slot) const { | ||
| glActiveTexture(GL_TEXTURE0 + slot); | ||
| glBindTexture(GL_TEXTURE_2D, m_RendererID); | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add texture slot validation in Bind method.
The Bind method should validate the texture slot against GL_MAX_TEXTURE_IMAGE_UNITS.
void Texture::Bind(uint32_t slot) const {
+ static GLint maxTextureUnits = 0;
+ if (maxTextureUnits == 0) {
+ glGetIntegerv(GL_MAX_TEXTURE_IMAGE_UNITS, &maxTextureUnits);
+ }
+ if (slot >= static_cast<uint32_t>(maxTextureUnits)) {
+ throw std::runtime_error(std::format("Texture slot {} exceeds maximum allowed ({})",
+ slot, maxTextureUnits - 1));
+ }
glActiveTexture(GL_TEXTURE0 + slot);
glBindTexture(GL_TEXTURE_2D, m_RendererID);
}📝 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.
| /** @brief Binds the texture to a specific texture slot | |
| * @param slot The texture unit slot to bind to | |
| */ | |
| void Texture::Bind(uint32_t slot) const { | |
| glActiveTexture(GL_TEXTURE0 + slot); | |
| glBindTexture(GL_TEXTURE_2D, m_RendererID); | |
| } | |
| /** @brief Binds the texture to a specific texture slot | |
| * @param slot The texture unit slot to bind to | |
| */ | |
| void Texture::Bind(uint32_t slot) const { | |
| static GLint maxTextureUnits = 0; | |
| if (maxTextureUnits == 0) { | |
| glGetIntegerv(GL_MAX_TEXTURE_IMAGE_UNITS, &maxTextureUnits); | |
| } | |
| if (slot >= static_cast<uint32_t>(maxTextureUnits)) { | |
| throw std::runtime_error(std::format("Texture slot {} exceeds maximum allowed ({})", | |
| slot, maxTextureUnits - 1)); | |
| } | |
| glActiveTexture(GL_TEXTURE0 + slot); | |
| glBindTexture(GL_TEXTURE_2D, m_RendererID); | |
| } |
| "with open('profile_results.json', 'r') as file:\n", | ||
| " profile_data = json.load(file)\n", | ||
| "\n", | ||
| "with open('profiler_n_frames.json', 'r') as file:\n", | ||
| " frame_data = json.load(file)\n", |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for file loading.
The JSON file loading should include error handling for missing files or invalid JSON.
-with open('profile_results.json', 'r') as file:
- profile_data = json.load(file)
+try:
+ with open('profile_results.json', 'r') as file:
+ profile_data = json.load(file)
+except FileNotFoundError:
+ print("Error: profile_results.json not found")
+ profile_data = {"profiles": []}
+except json.JSONDecodeError:
+ print("Error: Invalid JSON in profile_results.json")
+ profile_data = {"profiles": []}Committable suggestion skipped: line range outside the PR's diff.
| for (const auto& [vert, frag] : frequentShaders) { | ||
| auto shader = Shader::CreateFromFiles(vert, frag); | ||
| if (shader) { | ||
| // Cache the shader and enable hot-reloading in debug builds | ||
| AssetManager::Get().MarkAsFrequentlyUsed<Shader>(vert + ";" + frag); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling and path concatenation.
The current implementation has two potential issues:
- Silent failure when shader creation fails
- Using ";" as a path separator is fragile and could conflict with file paths on some systems
Consider this improved implementation:
- for (const auto& [vert, frag] : frequentShaders) {
- auto shader = Shader::CreateFromFiles(vert, frag);
- if (shader) {
- // Cache the shader and enable hot-reloading in debug builds
- AssetManager::Get().MarkAsFrequentlyUsed<Shader>(vert + ";" + frag);
- }
- }
+ for (const auto& [vert, frag] : frequentShaders) {
+ auto shader = Shader::CreateFromFiles(vert, frag);
+ if (!shader) {
+ // Log error or throw exception
+ LOG_ERROR("Failed to preload shader: {} + {}", vert, frag);
+ continue;
+ }
+ // Use a more robust key generation method
+ std::string key = AssetManager::GenerateShaderKey(vert, frag);
+ AssetManager::Get().MarkAsFrequentlyUsed<Shader>(key);
+ }Committable suggestion skipped: line range outside the PR's diff.
Summary by CodeRabbit
Release Notes
New Features
Improvements
Performance
Debugging