feat: add back-end for Asset management #63
Conversation
There was a problem hiding this comment.
Copilot reviewed 59 out of 76 changed files in this pull request and generated no suggestions.
Files not reviewed (17)
- CMakeLists.txt: Language not supported
- config/default-layout.ini: Language not supported
- editor/CMakeLists.txt: Language not supported
- editor/main.cpp: Language not supported
- editor/src/ADocumentWindow.hpp: Language not supported
- editor/src/DocumentWindows/AssetManagerWindow.cpp: Language not supported
- editor/src/DocumentWindows/AssetManagerWindow.hpp: Language not supported
- editor/src/DocumentWindows/ConsoleWindow.cpp: Language not supported
- editor/src/DocumentWindows/ConsoleWindow.hpp: Language not supported
- editor/src/DocumentWindows/MainScene.cpp: Language not supported
- editor/src/DocumentWindows/MainScene.hpp: Language not supported
- editor/src/DocumentWindows/SceneTreeWindow.cpp: Language not supported
- editor/src/DocumentWindows/SceneTreeWindow.hpp: Language not supported
- editor/src/Editor.cpp: Language not supported
- editor/src/Editor.hpp: Language not supported
- editor/src/IDocumentWindow.hpp: Language not supported
- editor/src/SceneManagerBridge.cpp: Language not supported
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis pull request delivers a large-scale update across the codebase. It overhauls the UI window layout and editor instantiation while introducing a comprehensive backend for asset management. New modules for asset registration, importing, referencing, naming validation, and JSON‐based serialization are added under the engine’s asset subsystem. Changes also include enhancements to the OpenGL renderer (e.g. support for texture creation from memory), updates to build configurations and dependency lists, and additional unit tests verifying the new asset management functionality and renderer API. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant AI as AssetImporter
participant Imp as SpecificImporter
participant AC as AssetCatalog
App->>AI: importAssetAuto(location, inputVariant)
AI->>Imp: canRead(inputVariant)
Imp-->>AI: (true/false)
alt Importer accepts input
AI->>Imp: importAssetUsingImporter(location, inputVariant)
Imp-->>AI: asset data
AI->>AC: registerAsset(location, asset)
AC-->>AI: Registered AssetRef
else No importer supports input
AI-->>App: Return null asset ref
end
AI-->>App: AssetRef (or null)
Assessment against linked issues
🪧 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 (
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![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 (2)
engine/src/assets/Asset.cpp (1)
20-21: 🧹 Nitpick (assertive)Namespace Comment Mismatch
The namespace is declared asnexo::assets, but the closing comment on line 20 incorrectly statesnexo::editor. For clarity and consistency, please update the closing comment.
Suggested fix:-} // namespace nexo::editor +} // namespace nexo::assetsengine/src/assets/ValidatedName.cpp (1)
1-24: 🧹 Nitpick (assertive)New File: ValidatedName Implementation Shell
This new file sets up the implementation for the
ValidatedNameclass. It includes appropriate headers and namespace encapsulation but currently has no functional implementation.Consider adding TODO comments or a stub implementation to clarify future intentions for this file.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between f95a8e7 and dd978d6453b1dd0c5a18e27f38e65573b7808f33.
📒 Files selected for processing (73)
config/default-layout.ini(3 hunks)editor/CMakeLists.txt(2 hunks)editor/main.cpp(3 hunks)editor/src/ADocumentWindow.hpp(2 hunks)editor/src/DocumentWindows/AssetManagerWindow.cpp(1 hunks)editor/src/DocumentWindows/AssetManagerWindow.hpp(1 hunks)editor/src/DocumentWindows/ConsoleWindow.cpp(1 hunks)editor/src/DocumentWindows/ConsoleWindow.hpp(2 hunks)editor/src/DocumentWindows/MainScene.cpp(1 hunks)editor/src/DocumentWindows/MainScene.hpp(2 hunks)editor/src/DocumentWindows/SceneTreeWindow.cpp(1 hunks)editor/src/DocumentWindows/SceneTreeWindow.hpp(1 hunks)editor/src/Editor.cpp(1 hunks)editor/src/Editor.hpp(2 hunks)editor/src/IDocumentWindow.hpp(2 hunks)editor/src/SceneManagerBridge.cpp(2 hunks)editor/src/SceneManagerBridge.hpp(2 hunks)editor/src/backends/ImGuiBackend.cpp(1 hunks)engine/CMakeLists.txt(1 hunks)engine/include/json.hpp(1 hunks)engine/src/assets/Asset.cpp(1 hunks)engine/src/assets/Asset.hpp(1 hunks)engine/src/assets/AssetCatalog.cpp(1 hunks)engine/src/assets/AssetCatalog.hpp(1 hunks)engine/src/assets/AssetImporter.cpp(1 hunks)engine/src/assets/AssetImporter.hpp(1 hunks)engine/src/assets/AssetImporterBase.hpp(1 hunks)engine/src/assets/AssetImporterContext.cpp(1 hunks)engine/src/assets/AssetImporterContext.hpp(1 hunks)engine/src/assets/AssetImporterInput.hpp(1 hunks)engine/src/assets/AssetLocation.cpp(1 hunks)engine/src/assets/AssetLocation.hpp(1 hunks)engine/src/assets/AssetManager.cpp(1 hunks)engine/src/assets/AssetManager.hpp(1 hunks)engine/src/assets/AssetName.hpp(1 hunks)engine/src/assets/AssetPackName.hpp(1 hunks)engine/src/assets/AssetRef.cpp(1 hunks)engine/src/assets/AssetRef.hpp(1 hunks)engine/src/assets/Assets/Model/Model.hpp(1 hunks)engine/src/assets/Assets/Model/ModelImporter.hpp(1 hunks)engine/src/assets/Assets/Model/ModelParameters.hpp(1 hunks)engine/src/assets/Assets/Texture/Texture.hpp(1 hunks)engine/src/assets/Assets/Texture/TextureImporter.hpp(1 hunks)engine/src/assets/Assets/Texture/TextureParameters.hpp(1 hunks)engine/src/assets/FilenameValidator.hpp(1 hunks)engine/src/assets/ValidatedName.cpp(1 hunks)engine/src/assets/ValidatedName.hpp(1 hunks)engine/src/renderer/Buffer.cpp(1 hunks)engine/src/renderer/Framebuffer.cpp(1 hunks)engine/src/renderer/GraphicsBackends/opengl/OpenGlBuffer.hpp(1 hunks)engine/src/renderer/GraphicsBackends/opengl/OpenGlTexture2D.cpp(2 hunks)engine/src/renderer/GraphicsBackends/opengl/OpenGlTexture2D.hpp(3 hunks)engine/src/renderer/RenderCommand.cpp(1 hunks)engine/src/renderer/Renderer2D.cpp(1 hunks)engine/src/renderer/Shader.cpp(1 hunks)engine/src/renderer/Texture.cpp(2 hunks)engine/src/renderer/Texture.hpp(2 hunks)engine/src/renderer/VertexArray.cpp(1 hunks)engine/src/renderer/Window.cpp(1 hunks)scripts/CMakeCPackOptions.cmake.in(1 hunks)tests/CMakeLists.txt(0 hunks)tests/engine/CMakeLists.txt(1 hunks)tests/engine/assets/AssetLocation.test.cpp(1 hunks)tests/engine/assets/AssetName.test.cpp(1 hunks)tests/renderer/Buffer.test.cpp(1 hunks)tests/renderer/CMakeLists.txt(1 hunks)tests/renderer/Framebuffer.test.cpp(1 hunks)tests/renderer/RendererAPI.test.cpp(2 hunks)tests/renderer/Shader.test.cpp(1 hunks)tests/renderer/Texture.test.cpp(1 hunks)tests/renderer/VertexArray.test.cpp(1 hunks)tests/test_main.cpp(1 hunks)vcpkg.json(2 hunks)
💤 Files with no reviewable changes (1)
- tests/CMakeLists.txt
🧰 Additional context used
🪛 Cppcheck (2.10-2)
engine/src/assets/AssetImporterContext.cpp
[style] 19-19: The function 'setMainAsset' is never used.
(unusedFunction)
[style] 24-24: The function 'getMainAsset' is never used.
(unusedFunction)
[style] 29-29: The function 'addDependency' is never used.
(unusedFunction)
[style] 34-34: The function 'getDependencies' is never used.
(unusedFunction)
[style] 44-44: The function 'getParameters' is never used.
(unusedFunction)
tests/engine/assets/AssetLocation.test.cpp
[error] 34-34: syntax error
(syntaxError)
engine/src/assets/AssetImporter.cpp
[style] 29-29: The function 'importAssetAuto' is never used.
(unusedFunction)
[style] 102-102: The function 'unregisterAllImportersForType' is never used.
(unusedFunction)
engine/src/renderer/GraphicsBackends/opengl/OpenGlTexture2D.cpp
[style] 81-81: The function 'setData' is never used.
(unusedFunction)
tests/engine/assets/AssetName.test.cpp
[error] 29-29: syntax error
(syntaxError)
editor/src/DocumentWindows/AssetManagerWindow.cpp
[style] 26-26: The function 'setup' is never used.
(unusedFunction)
[style] 56-56: The function 'shutdown' is never used.
(unusedFunction)
[style] 60-60: The function 'show' is never used.
(unusedFunction)
[style] 77-77: The function 'update' is never used.
(unusedFunction)
engine/src/assets/AssetCatalog.cpp
[style] 38-38: The function 'getAsset' is never used.
(unusedFunction)
[style] 55-55: The function 'getAssets' is never used.
(unusedFunction)
[style] 73-73: The function 'registerAsset' is never used.
(unusedFunction)
🪛 GitHub Check: Build, test and Package (ubuntu-latest, gcc)
editor/src/DocumentWindows/AssetManagerWindow.cpp
[warning] 115-115:
unused variable ‘drawList’ [-Wunused-variable]
🔇 Additional comments (135)
editor/src/SceneManagerBridge.hpp (2)
9-9: Updated Author Attribution:
The author comment on this line has been updated to "Guillaume HEIN." This change is consistent with the author updates across the codebase.
102-102: Namespace Closure Consistency:
The closing brace at line 102 is now explicitly marked, ensuring proper formatting and consistent namespace termination.editor/src/SceneManagerBridge.cpp (2)
9-9: Updated Author Attribution:
The header author comment now reflects "Guillaume HEIN," aligning with the updated author information across the project.
119-119: Formatting Improvement:
The adjustment (or addition) at line 119 ensures that the namespace block is correctly closed and that the file's formatting adheres to consistency guidelines.editor/src/DocumentWindows/MainScene.cpp (1)
9-9: Updated Author Attribution:
The author comment has been changed to "Guillaume HEIN." This update maintains consistency with similar changes in other parts of the codebase.editor/src/DocumentWindows/SceneTreeWindow.cpp (1)
9-9: Updated Author Attribution:
The author information on line 9 has been updated to "Guillaume HEIN," ensuring consistency with the updated metadata across the project.editor/src/DocumentWindows/SceneTreeWindow.hpp (1)
9-9: Updated Author Attribution:
The header now correctly lists "Guillaume HEIN" as the author. This change is in line with the uniform author updates made throughout the pull request.editor/src/Editor.cpp (1)
9-9: Updated Author Attribution
The author attribution on line 9 has been updated to "Guillaume HEIN." This change is consistent with other updates across the codebase.editor/src/DocumentWindows/ConsoleWindow.cpp (1)
9-11: Updated Author Attribution
The updated author information ("Guillaume HEIN") on lines 9–11 brings consistency with the revised author details in related files.engine/src/assets/Asset.cpp (1)
1-13: New Asset Base Class Implementation
This new file introduces the Asset base class along with proper header documentation, providing a solid foundation for asset management functionality.editor/src/backends/ImGuiBackend.cpp (1)
78-79: Formatting Improvement: Newline Addition
A newline has been added at the end of the file. This minor formatting improvement enhances file consistency.engine/src/assets/AssetRef.cpp (1)
1-21: New Asset Reference Implementation
The newAssetRef.cppfile is well-structured and clearly documented. It lays a clean foundation for managing asset references within the asset management system.editor/src/DocumentWindows/MainScene.hpp (2)
9-9: Updated Author AttributionThe header’s author attribution has been updated from "Mehdy MORVAN" to "Guillaume HEIN". This update is consistent with the overall changes in the PR.
1-137: Overall File Structure and ConsistencyThe remaining content for the
MainScene.hppfile remains unchanged and well-structured. Ensure that similar attribution updates across all header comments are uniformly applied.editor/src/IDocumentWindow.hpp (2)
9-9: Consistent Author Attribution in HeaderThe author comment has been updated to "Guillaume HEIN" as intended. This helps maintain consistency across the project's files.
1-35: No Other Functional ChangesThere are no other modifications in this file affecting functionality. The interface definitions remain intact.
editor/src/ADocumentWindow.hpp (2)
9-9: Author Attribution UpdatedThe change to "Guillaume HEIN" for the author header is correctly applied in this file, maintaining consistency with the updated author details across the project.
1-52: File Integrity CheckThe file structure and class declarations remain unchanged. All public and protected members are properly defined, ensuring no unintended side effects from this author update.
engine/src/assets/ValidatedName.cpp (1)
15-24: Include and Namespace UsageThe inclusion of
"ValidatedName.hpp"and the wrapping within thenexo::assetsnamespace look correct. Ensure that the corresponding header file defines the expected interface for this implementation.engine/src/renderer/VertexArray.cpp (2)
16-18: Updated OpenGL Include PathThe OpenGL include directive has been updated from the old path to
"GraphicsBackends/opengl/OpenGlVertexArray.hpp", reflecting the reorganized directory structure.Ensure that the build configuration (e.g., CMakeLists.txt) and any dependent modules are updated to reflect this new path.
13-31: Overall File FunctionalityThe rest of the file remains functionally unchanged, and the conditional compilation for the OpenGL backend is correctly maintained.
🧰 Tools
🪛 Cppcheck (2.10-2)
[style] 22-22: The function 'createVertexArray' is never used.
(unusedFunction)
engine/src/renderer/Buffer.cpp (1)
17-17: Update include path for OpenGL buffer header.
The include directive forOpenGlBuffer.hpphas been updated to use the new directory structure (GraphicsBackends/opengl/OpenGlBuffer.hpp). This change is consistent with the broader refactor of the graphics backend and should be verified against all dependent modules.editor/src/DocumentWindows/ConsoleWindow.hpp (2)
9-10: Update author attribution.
The author attribution has been changed from "Mehdy MORVAN" to "Guillaume HEIN" to reflect the new ownership or contribution standards. This update appears intentional and consistent with similar changes in other files.
64-64: Ensure file formatting consistency.
A newline was added at the end of the file, which is good practice and maintains consistency across the codebase.tests/engine/CMakeLists.txt (1)
37-38: Include new asset management tests.
Two new test files (AssetName.test.cppandAssetLocation.test.cpp) have been added to the engine test suite. This enhances test coverage for the asset management features introduced in this PR. Please ensure that these tests run successfully as part of the build process.engine/src/renderer/Window.cpp (1)
18-18: Update include path for OpenGL window header.
The include directive has been updated from"renderer/opengl/OpenGlWindow.hpp"to"GraphicsBackends/opengl/OpenGlWindow.hpp", aligning the file with the new project structure for OpenGL resources. Verify that the new path is correct and that any downstream dependencies are updated accordingly.tests/renderer/Shader.test.cpp (1)
23-23: Update include path for OpenGL shader header in tests.
The reference toOpenGlShader.hppnow uses the new location underGraphicsBackends/opengl/, ensuring consistency with the refactored directory structure. This change is non-functional but important for maintaining organized code.engine/src/renderer/RenderCommand.cpp (1)
17-17: Path update for OpenGL renderer API is appropriate.This change correctly updates the include path to reflect the new directory structure where OpenGL backend files have been moved to a dedicated
GraphicsBackendsdirectory. This is consistent with the similar changes in other renderer files and improves the organization of graphics-specific implementations.tests/renderer/Buffer.test.cpp (1)
20-20: Updated include path is correct.The include path has been updated to reflect the new directory structure, moving OpenGL components from a direct
openglfolder to a nestedGraphicsBackends/openglfolder. This change aligns with the broader reorganization of renderer components.engine/include/json.hpp (1)
15-29: Well-structured JSON utilities with proper modern C++ concepts.The implementation:
- Properly includes the nlohmann/json library
- Creates a useful type alias within the nexo namespace
- Defines a concept for JSON serializable types using C++20 features
This provides a solid foundation for JSON handling throughout the engine.
tests/renderer/CMakeLists.txt (1)
43-49: Path updates correctly reflect the new directory structure.The updated paths for OpenGL-related source files properly reflect the reorganization of these components into the GraphicsBackends directory. This ensures proper test compilation with the new structure.
engine/src/renderer/Shader.cpp (1)
18-18: Update to include path looks good.The path modification from "opengl/OpenGlShader.hpp" to "GraphicsBackends/opengl/OpenGlShader.hpp" reflects a reorganization of the graphics backend structure. This change maintains consistency with the project's architecture updates.
tests/renderer/Texture.test.cpp (1)
20-20: Include path update is consistent with backend reorganization.This change aligns with the architectural updates to place OpenGL implementations under the GraphicsBackends directory, consistent with the changes made in other renderer files.
engine/src/assets/Assets/Model/Model.hpp (1)
24-28: Address the TODO comment with actual model data implementation.The ModelData struct is currently a placeholder with only a pointer to an aiScene object. Consider documenting the ownership semantics of this pointer - who is responsible for allocating/freeing this memory?
Is there a timeline for implementing the complete model data structure? The current implementation might lead to memory management issues if not properly handled.
vcpkg.json (2)
20-20: LGTM: Dependency on boost-uuid addedThe addition of
boost-uuidwill enable UUID generation capabilities, which is likely needed for the new asset management system.
29-30: LGTM: Dependencies updated appropriatelyThe formatting update for the assimp dependency and addition of nlohmann-json are good changes. The nlohmann-json library will provide robust JSON parsing capabilities, which will be useful for asset metadata handling.
tests/renderer/RendererAPI.test.cpp (3)
17-17: Path updated correctly for OpenGlVertexArray.hppThe include path has been properly updated to reflect the new directory structure with GraphicsBackends organization.
20-20: Path updated correctly for OpenGlRendererAPI.hppThe include path has been properly updated to reflect the new directory structure with GraphicsBackends organization.
122-122: LGTM: Namespace closing braceThe namespace closing brace is properly formatted.
engine/src/assets/AssetManager.hpp (3)
26-28: LGTM: Asset manager class in appropriate namespaceThe AssetManager class is correctly placed in the nexo::assets namespace.
30-35: Placeholder constructor implementationThe constructor currently has an empty implementation. This is acceptable for an initial PR that sets up the structure, but ensure that proper initialization logic is added in subsequent changes.
37-64: Commented out code needs implementationMost of the functionality is commented out, which suggests this is a work in progress. The design looks solid with functions for creating, loading, unloading and removing assets.
Please confirm if this commented code is intended to be implemented in a follow-up PR or if it should be uncommented and implemented now.
tests/renderer/Framebuffer.test.cpp (1)
20-22: LGTM: Include paths updated correctly for OpenGL backendThe include paths for OpenGlVertexArray, OpenGlBuffer, and OpenGlFramebuffer have been properly updated to reflect the new directory structure with GraphicsBackends organization.
engine/src/renderer/Texture.hpp (1)
74-91: LGTM! Well-documented texture creation from memory bufferThe added method with comprehensive documentation allows creating textures directly from memory buffers, which is essential for the asset management system. The documentation clearly explains parameters, expected formats, and includes a useful example.
engine/src/assets/AssetName.hpp (1)
16-27: LGTM! Good type safety approach for asset namesCreating a strongly-typed wrapper for asset names with validation is a sound design decision. This approach enforces naming conventions and provides type safety throughout the codebase.
editor/CMakeLists.txt (2)
11-23: Build configuration aligned with new asset management functionality.The source files are now properly included in the build process with the removal of the minus sign prefixes. The addition of
AssetManagerWindow.cppaligns with the PR objective of implementing an Asset Manager window within the editor.
83-84: Adding Boost UUID dependency is appropriate for asset management.Boost UUID is a good choice for generating unique identifiers for assets, which is essential for a robust asset management system.
tests/renderer/VertexArray.test.cpp (1)
20-21: Updated include paths reflect directory restructuring.The include paths have been correctly updated to match the new directory structure where OpenGL components are now located under the
GraphicsBackendsdirectory.#!/bin/bash # Verify that these files exist in the new location fd "OpenGlVertexArray.hpp|OpenGlBuffer.hpp" --type fscripts/CMakeCPackOptions.cmake.in (1)
44-44: Enhanced Debian package dependencies for X11/OpenGL compatibility.Adding
libxrandr2andlibxrender1as dependencies is appropriate for ensuring proper functionality of OpenGL applications on Linux systems. The version constraint forlibxrandr2helps ensure compatibility with the required features.engine/src/assets/AssetImporterInput.hpp (2)
23-32: Well-structured input definitions for the asset importer system.The
ImporterFileInputandImporterMemoryInputstructures provide a clean interface for supporting both file-based and memory-based asset importing, which is essential for a flexible asset management system.
34-34: Good use of std::variant for flexible input handling.Using
std::variantto represent different input types is a modern C++ approach that provides type safety while allowing for flexible input handling in the asset importing process.editor/src/Editor.hpp (1)
52-68: Excellent implementation of the Singleton pattern!The changes follow the Meyers' Singleton Pattern which provides thread-safety guarantees in C++11 and later. You've correctly:
- Made the constructor and destructor private
- Provided a static getInstance() method that returns a reference to the single instance
- Deleted the copy constructor and assignment operator to prevent copying
This is a clean and proper implementation of the Singleton pattern.
engine/src/renderer/Texture.cpp (1)
19-19: Path update looks good.The include path has been correctly updated to reflect the new directory structure reorganization.
engine/src/renderer/GraphicsBackends/opengl/OpenGlBuffer.hpp (2)
196-196: Good use of [[nodiscard]] attribute.Adding the
[[nodiscard]]attribute to thegetId()method is a good practice as it indicates that the return value shouldn't be ignored, enforcing proper API usage.
199-199: Proper variable initialization.Initializing
_countto 0 ensures the variable has a defined state before it's used, which is good practice to avoid undefined behavior.engine/CMakeLists.txt (1)
64-70: Path updates are consistent with directory structure changes.The paths for OpenGL-related source files have been correctly updated to reflect the new directory structure, which is consistent with changes seen in other files.
editor/main.cpp (2)
29-29: Refactor to Singleton patternThe Editor instantiation has been changed to use a Singleton pattern. This is a good design change that ensures a single instance of the Editor is used throughout the application.
33-35: Added Asset Manager windowThe addition of the Asset Manager window integrates well with the existing window registration pattern.
engine/src/assets/Assets/Model/ModelImporter.hpp (1)
71-72:⚠️ Potential issuePotential null pointer dereference
The code
memInput.fileExtension->c_str()could lead to a null pointer dereference iffileExtensionis empty. The ternary operator correctly checks iffileExtensionhas a value, but then accesses the value directly without checking.-scene = m_importer.ReadFileFromMemory(memInput.memoryData.data(), memInput.memoryData.size(), flags, memInput.fileExtension ? memInput.fileExtension->c_str() : nullptr); +scene = m_importer.ReadFileFromMemory(memInput.memoryData.data(), memInput.memoryData.size(), flags, memInput.fileExtension ? memInput.fileExtension.value().c_str() : nullptr);Likely an incorrect or invalid review comment.
engine/src/assets/AssetImporterContext.cpp (5)
1-13: LGTM - Clear header documentationThe header documentation clearly states the author, date, and purpose of the file. The formatting is consistent with project standards.
19-27: Straightforward implementation of asset management methodsThe main asset setter and getter methods are simple and work as expected. The implementation is clear and concise.
🧰 Tools
🪛 Cppcheck (2.10-2)
[style] 19-19: The function 'setMainAsset' is never used.
(unusedFunction)
[style] 24-24: The function 'getMainAsset' is never used.
(unusedFunction)
29-37: Appropriate dependency management implementationThe methods for adding and retrieving dependencies are implemented correctly. Using a vector to store multiple dependencies is a good approach.
🧰 Tools
🪛 Cppcheck (2.10-2)
[style] 29-29: The function 'addDependency' is never used.
(unusedFunction)
[style] 34-34: The function 'getDependencies' is never used.
(unusedFunction)
39-47: JSON parameter handling is well-implementedThe methods for setting and getting JSON parameters are implemented correctly, providing a flexible way to configure assets.
🧰 Tools
🪛 Cppcheck (2.10-2)
[style] 44-44: The function 'getParameters' is never used.
(unusedFunction)
19-47:❓ Verification inconclusive
Consider implementing proper error handling
Static analysis indicates that these methods aren't currently used in the codebase. This is expected for new features, but you should ensure proper error handling is implemented once they're integrated with the rest of the asset management system.
🏁 Script executed:
#!/bin/bash # Check for implementations that might use these methods echo "Searching for potential uses of AssetImporterContext methods..." rg -A 3 "AssetImporterContext" --type cpp --type hppLength of output: 217
Below is an updated shell script that adds a custom file type for “hpp” files. Please run it to verify if there are any references to the AssetImporterContext methods in both CPP and HPP files:
The previous command failed due to an unrecognized file type for “hpp”. Once you run this script and review the output, we can better assess whether the methods are being used (and thus whether additional error handling needs to be implemented).
Please run this script and provide a manual verification of the output if needed.
🏁 Script executed:
#!/bin/bash # Re-run search for AssetImporterContext methods with proper support for .hpp files rg -A 3 "AssetImporterContext" --type-add="hpp:*.hpp" --type=cpp --type=hppLength of output: 8570
Action: Revisit Error Handling in AssetImporterContext Methods Upon Integration
The current implementations in
engine/src/assets/AssetImporterContext.cppare simple setters and getters without any error checks. While these methods are now referenced indirectly in asset importer implementations (evidenced by their inclusion and use in files such asAssetImporterBase.hppand within asset importers for textures and models), they lack defensive coding—for example, there’s no validation for null inputs or unexpected states.
Location:
engine/src/assets/AssetImporterContext.cpp(methods:setMainAsset,getMainAsset,addDependency,getDependencies,setParameters,getParameters)Notes:
- The static analysis comment about these methods being "unused" no longer holds, as they are integrated within the asset importer system.
- Although their simplicity may suffice for now, please ensure that once these methods are used in more complex scenarios, appropriate error handling (such as validating inputs or flagging errors when dependencies are missing) is incorporated.
- Consider adding unit tests or runtime assertions as the asset management system evolves.
🧰 Tools
🪛 Cppcheck (2.10-2)
[style] 19-19: The function 'setMainAsset' is never used.
(unusedFunction)
[style] 24-24: The function 'getMainAsset' is never used.
(unusedFunction)
[style] 29-29: The function 'addDependency' is never used.
(unusedFunction)
[style] 34-34: The function 'getDependencies' is never used.
(unusedFunction)
[style] 44-44: The function 'getParameters' is never used.
(unusedFunction)
tests/engine/assets/AssetName.test.cpp (9)
20-27: Well-structured tests for valid asset namesThe valid asset name tests cover a good range of valid name formats, including names with underscores, numbers, hyphens, and periods.
29-32: Good test for empty name validationThe test for empty name validation is straightforward and verifies that an exception is thrown as expected.
🧰 Tools
🪛 Cppcheck (2.10-2)
[error] 29-29: syntax error
(syntaxError)
34-40: Thorough test for name length validationThis test correctly creates a name that exceeds the maximum length and verifies that an exception is thrown.
42-48: Comprehensive tests for invalid charactersGood coverage of various invalid characters that shouldn't be allowed in asset names.
50-57: Thorough check of reserved keywordsThis test properly iterates through all forbidden keywords to ensure none of them can be used as asset names.
59-64: Complete validator function testsThese tests thoroughly validate both the static validation methods, covering both positive and negative cases, as well as verifying correct error messages.
Also applies to: 66-71, 73-79
81-91: Complete string conversion and comparison testsThese tests verify string conversion functionality, equality/inequality operators, and data access methods.
Also applies to: 93-103, 105-112
114-123: Effective renaming functionality testThe test properly verifies both successful renaming and error handling when attempting to rename with an invalid name.
125-132: Excellent exception message testingThe tests for exception messages are thorough, and the helper function
testExceptionis a good approach to avoid code duplication when testing various invalid scenarios.Also applies to: 134-151
engine/src/renderer/GraphicsBackends/opengl/OpenGlTexture2D.hpp (4)
35-35: Good practice - Adding explicit destructorAdding an explicit destructor ensures proper cleanup of OpenGL resources, which is essential for preventing memory leaks in GPU applications.
71-88: Well-documented constructor for loading from memoryThe new constructor for loading textures from memory buffers is thoroughly documented with clear parameter descriptions and usage examples.
152-170: Well-documented helper method for STB image dataThe
ingestDataFromStbhelper method is clearly documented with appropriate warnings and examples, which will help prevent misuse.
35-35:✅ Verification successful
Consider implementing texture resource tracking
When adding OpenGL resource management like textures, it's important to ensure proper tracking of resources. Verify that the implementation of the destructor properly cleans up GL resources to prevent leaks.
Also applies to: 71-88, 152-170
🏁 Script executed:
#!/bin/bash # Check for destructor implementation and GL resource cleanup echo "Searching for OpenGlTexture2D destructor implementation..." rg -A 5 "OpenGlTexture2D::~OpenGlTexture2D" --type cpp echo "\nChecking for glDeleteTextures calls..." rg "glDeleteTextures" --type cppLength of output: 2077
Texture Resource Cleanup Validated
The implementation of the destructor inengine/src/renderer/GraphicsBackends/opengl/OpenGlTexture2D.cppnow correctly callsglDeleteTextures(1, &m_id), ensuring that OpenGL texture resources are properly released and preventing leaks. Please confirm that similar resource cleanup patterns, as referenced in the additional code sections (lines 71-88 and 152-170), are also consistently applied throughout the codebase.engine/src/assets/AssetImporterBase.hpp (2)
30-33: Good use of virtual destructor for the base class.Defining a virtual destructor ensures that derived class destructors are invoked properly when disposing of derived objects via base pointers.
62-74: Exception handling is well-structured.Wrapping the call to
importImpl(ctx)in atryblock and logging errors on exception aids in clarity and maintainability. The fallback path returning early whengetMainAsset()is null also protects against incomplete import setups.tests/engine/assets/AssetLocation.test.cpp (2)
22-32: Excellent coverage for typical asset location usage.These tests cover the scenario of a full location string with both pack name and path. The assertions thoroughly verify correct parsing into each component.
34-34: Likely a Cppcheck false positive.The static analysis warning of a syntax error at line 34 appears to be a false positive. The test declaration syntax is correct and should compile as expected.
🧰 Tools
🪛 Cppcheck (2.10-2)
[error] 34-34: syntax error
(syntaxError)
engine/src/assets/AssetCatalog.cpp (1)
24-29: Graceful handling of non-existent asset IDs.Returning early if an asset ID is not found prevents unnecessary lookups or errors, ensuring robust deletion logic.
engine/src/assets/AssetImporterContext.hpp (1)
100-121: EnsureASSET_MAX_DEPENDENCIESis a well-defined safeguard.The loop tries to generate a unique name until
m_depUniqueIdexceedsASSET_MAX_DEPENDENCIES. Verify that the value forASSET_MAX_DEPENDENCIESis suitably large and is visible in this compilation unit. Otherwise, there’s a risk of collision if the catalog grows quickly.editor/src/DocumentWindows/AssetManagerWindow.cpp (1)
28-30: Confirm type logic fori % 3.You alternate asset types with
i % 3but only handleTEXTURE=1andMODEL=2ingetAssetTypeOverlayColor(). The case for 0 defaults to no overlay, which may be intentional or an oversight.engine/src/assets/AssetImporter.cpp (2)
29-38:importAssetAutousage
Static analysis indicates thatimportAssetAutois currently unused. Although this may be intentional for future expansions, it’s worth verifying that the function is either needed or can be safely removed if no longer planned.Do you plan to use this function in upcoming features? If not, consider removing it to reduce code bloat.
🧰 Tools
🪛 Cppcheck (2.10-2)
[style] 29-29: The function 'importAssetAuto' is never used.
(unusedFunction)
102-106: UnusedunregisterAllImportersForType
Static analysis reports this function is never used. If you plan on supporting dynamic unregistration later, the implementation is fine. Otherwise, removing this unused method might avoid confusion.🧰 Tools
🪛 Cppcheck (2.10-2)
[style] 102-102: The function 'unregisterAllImportersForType' is never used.
(unusedFunction)
engine/src/assets/ValidatedName.hpp (4)
18-25: Exception message clarity
By throwingInvalidNamewith a formatted message, the exception helps diagnose naming issues quickly. This design effectively raises a descriptive error, improving debuggability.
27-31: Validator concept correctness
TheValidatorconcept ensures that any type used forTValidatorimplements a staticvalidatemethod returningstd::optional<std::string>. This is a solid approach to maintain compile-time guarantees.
38-105: Graceful fallback on invalid names
When validation fails in the constructor, the name is set to "Unnamed". This fallback, combined with an exception, provides a clear method to handle invalid input without leaving the object in an undefined state.
106-137: Consistent validation in assignments and rename
All assignment operators and therenamemethod use the same validation logic. This ensures consistency and reduces the chance of bypassing the validation checks.engine/src/assets/AssetCatalog.hpp (2)
31-48: Meyers’ Singleton approach
Using a private constructor with a staticgetInstance()ensures a singleAssetCatalog. This design is straightforward while limiting the possibility of accidental copies. However, be mindful of potential threading implications (e.g., double-checked locking is no longer typically required in C++11+).
49-60: Asset deletion interface
Providing two overloads for deleting assets (deleteAsset(AssetID)anddeleteAsset(const GenericAssetRef&)) enhances convenience. Verify in implementation that the latter fetches the correct ID or handles an invalid reference gracefully.config/default-layout.ini (23)
1-5: Consider verifying the viewport size on lower-resolution displays.
These lines add a large dock space at (0,24) with size (1920,1056). Ensure it works properly for users with screens smaller than 1920×1080, as the window could exceed screen boundaries.
7-7: No functional issues with repositioning this window.
Moving the Debug window to (118,38) appears harmless and doesn't conflict with other windows in this layout.
11-15: Scene Tree docking placement looks acceptable.
Placing the Scene Tree at (1499,45) suggests a right-side layout on a 1920-wide screen. Confirm usability on different resolutions.
21-21: Dock ID update for ImGui Demo.
No issues detected; this simply reassigns the dock.
23-27: Console window placement.
Dock ID changed to 0x0000000A. Verify it doesn’t overlap with the Default scene window.
29-33: Default scene docking.
This newly assigned DockId=0x00000009 is standard. No specific issues found.
35-39: New WindowOverViewport usage.
Likely used for overlay. Verify correct layering if the user toggles this window.
40-44: Example: Assets Browser window.
No concerns; it’s placed at (60,60), size (800,480). Layout looks fine.
46-46: Property editor window changes.
Collapsed is set to 0, so it stays open. This is a typical usage.Also applies to: 48-49
50-53: Nested child window addition.
The new child window “##tree_73339443” is sized at (300,410). Appears correct for a property tree.
59-59: Reintroducing the Example: Console window.
Just confirm this doesn’t conflict with your newly introduced “[Window][Console]”.
64-66: Example: Simple layout changes.
No issues; the window is at (60,60), size (500,440).
69-69: Left pane child window.
Size=150,376. A small side panel. Looks reasonable.Also applies to: 71-71
75-75: Adjusted size for Example: Auto-resizing window.
398×320 is a typical test dimension. No issues.
80-80: Create New Scene window location.
Sited at (732,456). Verify it is visible by default.
83-83: “test” window
Placing at (802,42) remains consistent with top-left adjacency.
85-85: Size update for “test” window
(838,721) is fairly large but presumably intended.
89-90: Dear ImGui ID Stack Tool
Window at (605,120), size (839,766). This overlaps less with the console. No immediate issues.
119-123: New Asset Manager window.
Docked at 0x0000000A,1. This is crucial to the new asset-management feature. Looks good.
125-128: Added table #0xDDC24BCA.
RefScale=18 with default columns. No concerns.
129-133: Added table #0xAC9A836A.
Column 0 width=4 is extremely narrow. Confirm if that’s intentional.
265-265: Docking data block
Specifies multiple DockNodes. No direct issues spotted.
271-271: DockNode ID=0x0000000A
Now references the console or asset manager arrangement. Ensure it’s correct if the console or manager is expanded.engine/src/assets/AssetLocation.hpp (9)
1-16: Header comment and file banner.
No issues. This thoroughly describes the purpose of the file and author details.
28-35: Custom exception class definition.
TheInvalidAssetLocationclass extendsException. This is reasonable. Just ensure consistency with other exceptions in your codebase.
37-47: Constructor callssetLocation(fullLocation).
Be aware iffullLocationis invalid, this throws during construction. This is acceptable but might complicate error handling if partially constructed objects exist. Otherwise looks fine.
49-66: setName, setPath, setPackName chaining.
Method-chaining style is neat. No functional issues.
67-72: Getter methods.
These straightforward getters look correct and self-explanatory.
85-90: OverloadedsetLocation(...)declared but not shown.
Ensure it’s implemented in.cppor marked inline. If the definition is missing, this could break linking.
91-107: Parsing and storing location insetLocation(const std::string&).
The try/catch rethrows a specialized exception for invalid names, aligning with your domain rules. Looks robust.
114-123: operator==(const std::string&)
Re-parses the string and compares the result. Fine for a convenience operator. Just ensure performance is acceptable if used frequently.
157-161: Private member variables.
Defaulting_nameto"Unnamed"is sensible. No issues.engine/src/assets/AssetRef.hpp (7)
1-18: File banner and includes.
No issues in the initial documentation.
37-69:GenericAssetRefconstruction &isValid,lock
Usingstd::weak_ptr<IAsset>is a typical approach.isValid()andlock()look correct.
70-85: Template cast methodas<TAsset>()
Implementation is deferred below. Declaring it here is fine. The approach withAssetRef<TAsset>is consistent.
86-93:null()factory method & load/unload stubs.
The load/unload placeholders rely on futureAssetCataloglogic. Logging a warning is a good interim measure.
128-176:AssetRef<TAsset>
Encapsulates the typed reference, providinglock()andisLoaded(). This fosters a type-safe approach. Looks correct.
178-186: Definition ofGenericAssetRef::as<TAsset>()
Usesstd::dynamic_pointer_cast<TAsset>to wrap the returning pointer. Good for safely converting IAsset derived classes.
187-191: Definition ofAssetRef<TAsset>::lock()
Relies onGenericAssetRef::lock(). This is straightforward and type-casts properly.engine/src/assets/AssetImporter.hpp (1)
173-186:✅ Verification successful
Validate priority insertion logic.
The loop at line 183 iterates until finding a lower priority, then inserts the new importer at that position. This is correct for ascending insertion, but make sure it aligns with your intended priority order (i.e., if higher priority should come first or last). Verify that all call sites and expected usage follow this logic.
🏁 Script executed:
#!/bin/bash # Description: Search for all internal calls to `registerImporter` to verify consistent usage of the priority parameter. rg -A 3 'registerImporter'Length of output: 1864
Priority Insertion Verification Summary
- The insertion loop in
engine/src/assets/AssetImporter.hpp(lines 173–186) uses an ascending order based on the numericpriorityvalue. In other words, elements with lower priority numbers are positioned earlier in the vector.- The search revealed call sites (e.g., in
engine/src/assets/AssetImporter.cpp) where importers are registered with priority values of 100 and 90. As a result, the importer with priority 90 is placed before the one with 100.- Please confirm that this ascending order (i.e., treating lower numeric values as higher priority) is the intended behavior for all call sites and that no adjustments to the ordering logic are needed.
engine/src/renderer/Framebuffer.cpp (1)
17-17: Header path update looks goodThe change to include the OpenGL framebuffer header from the new "GraphicsBackends" directory is appropriate and consistent with the PR's objectives. This structural change improves organization and maintainability, especially if additional graphics backends will be supported in the future.
tests/test_main.cpp (1)
14-14: Good optimization of test executionThis change correctly returns the stored result variable
rvinstead of callingRUN_ALL_TESTS()a second time. The improvement prevents tests from running twice, which optimizes execution time and avoids potential side effects or confusion from duplicate test runs. The proper return code is still propagated to the caller.engine/src/assets/Assets/Texture/Texture.hpp (1)
21-25: Implement the TODO for TextureDataThe TextureData struct is currently marked with a TODO and only contains a shared pointer to a renderer::Texture. Consider fully implementing the texture data structure to include all necessary properties for texture assets.
What additional properties should be included in the TextureData struct to fully support texture assets? For example, dimensions, format, mip levels, etc.
Thyodas
force-pushed
the
feat/asset-back
branch
from
a2afc03 to
55674e0
Compare
|
@coderabbitai generate docstrings |
✅ Actions performedDocstrings generation started. |
Docstrings generation was requested by @Thyodas. * #63 (comment) The following files were modified: * `editor/main.cpp` * `editor/src/DocumentWindows/AssetManagerWindow.cpp` * `editor/src/DocumentWindows/AssetManagerWindow.hpp` * `editor/src/Editor.hpp` * `editor/src/SceneManagerBridge.cpp` * `editor/src/backends/ImGuiBackend.cpp` * `engine/src/assets/Asset.hpp` * `engine/src/assets/AssetCatalog.cpp` * `engine/src/assets/AssetCatalog.hpp` * `engine/src/assets/AssetImporter.cpp` * `engine/src/assets/AssetImporter.hpp` * `engine/src/assets/AssetImporterBase.hpp` * `engine/src/assets/AssetImporterContext.cpp` * `engine/src/assets/AssetImporterContext.hpp` * `engine/src/assets/AssetLocation.cpp` * `engine/src/assets/AssetLocation.hpp` * `engine/src/assets/AssetRef.hpp` * `engine/src/assets/Assets/Model/Model.hpp` * `engine/src/assets/Assets/Model/ModelImporter.hpp` * `engine/src/assets/Assets/Texture/Texture.hpp` * `engine/src/assets/Assets/Texture/TextureImporter.hpp` * `engine/src/assets/FilenameValidator.hpp` * `engine/src/assets/ValidatedName.hpp` * `engine/src/renderer/GraphicsBackends/opengl/OpenGlBuffer.hpp` * `engine/src/renderer/GraphicsBackends/opengl/OpenGlTexture2D.cpp` * `engine/src/renderer/GraphicsBackends/opengl/OpenGlTexture2D.hpp` * `engine/src/renderer/Renderer2D.cpp` * `engine/src/renderer/Texture.cpp` * `tests/test_main.cpp`
|
Note Generated docstrings for this pull request at #141 |
The use of NEXO_TESTING could be useful for test specific code, such as friends. However, it is complicated to recompile all of our libs for tests only. Moreover it can lead to bad practice, tests might not test the same code as production.
…cyLocation and add formatUniqueName method
…ts for improved test discovery
…er logic in AssetImporter
…ssetImporter tests
…AssetImporter tests
test: add test for Assets
Docstrings generation was requested by @Thyodas. * #166 (comment) The following files were modified: * `engine/src/assets/Asset.hpp` * `engine/src/assets/AssetCatalog.cpp` * `engine/src/assets/AssetCatalog.hpp` * `engine/src/assets/AssetImporter.cpp` * `engine/src/assets/AssetImporter.hpp` * `engine/src/assets/AssetImporterContext.hpp` * `engine/src/assets/AssetLocation.hpp`
📝 Add docstrings to `test/asset-back`
|
@coderabbitai generate docstrings |
✅ Actions performedDocstrings generation started. |
Docstrings generation was requested by @Thyodas. * #63 (comment) The following files were modified: * `editor/main.cpp` * `editor/src/DocumentWindows/AssetManagerWindow.cpp` * `editor/src/Editor.cpp` * `editor/src/Editor.hpp` * `editor/src/backends/ImGuiBackend.cpp` * `engine/src/assets/Asset.hpp` * `engine/src/assets/AssetCatalog.cpp` * `engine/src/assets/AssetCatalog.hpp` * `engine/src/assets/AssetImporter.cpp` * `engine/src/assets/AssetImporter.hpp` * `engine/src/assets/AssetImporterBase.hpp` * `engine/src/assets/AssetImporterContext.cpp` * `engine/src/assets/AssetImporterContext.hpp` * `engine/src/assets/AssetLocation.cpp` * `engine/src/assets/AssetLocation.hpp` * `engine/src/assets/AssetRef.hpp` * `engine/src/assets/Assets/Model/Model.hpp` * `engine/src/assets/Assets/Model/ModelImporter.hpp` * `engine/src/assets/Assets/Texture/Texture.hpp` * `engine/src/assets/Assets/Texture/TextureImporter.hpp` * `engine/src/assets/FilenameValidator.hpp` * `engine/src/assets/ValidatedName.hpp` * `engine/src/renderer/Renderer2D.cpp` * `engine/src/renderer/Texture.cpp` * `engine/src/renderer/opengl/OpenGlBuffer.hpp` * `engine/src/renderer/opengl/OpenGlTexture2D.cpp` * `engine/src/renderer/opengl/OpenGlTexture2D.hpp` * `tests/test_main.cpp`
|
Note Generated docstrings for this pull request at #168 |
Closes #61
This pull request includes multiple changes aimed at improving the build process, adding new features, and updating documentation. The most important changes include adding a new Asset Manager window to the editor, updating the build configuration, and enhancing the documentation with a troubleshooting guide.
New Features:
Build Configuration Updates:
CMakeLists.txtto setNEXO_BUILD_TESTStoONand prevent Visual Studio from creating per-config subdirectories [1] [2]..github/workflows/build.yml. Huge bug that made the tests always pass on the Windows Runner.Documentation Enhancements:
docs/troubleshooting/troubleshooting.mdand referenced it inREADME.md[1] [2].Summary by CodeRabbit
New Features
Tests
Chores