Cleanups #257
Cleanups #257
Conversation
WalkthroughThe pull request introduces significant modifications to the OSRE project, focusing on the removal of the Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (6)
src/Engine/CMakeLists.txt (1)
Line range hint
8-14: Remove unused Unzip include directoryThe
contrib/unzip/directory is still listed inINCLUDE_DIRECTORIESbut the ZIP functionality has been removed from the project. This include path should be removed to maintain consistency with the changes.Apply this diff to remove the unused include directory:
INCLUDE_DIRECTORIES( engine contrib/cppcore/include contrib/glew/include contrib/glm/ - contrib/unzip/ contrib/zlib/ contrib/zlib/ contrib/assimp/include )src/Engine/App/TransformController.cpp (2)
146-147: Consider adding logging for Invalid command typeWhile having an explicit case for
Invalidis good practice, silently breaking might hide issues. Consider adding a warning log here to help with debugging when invalid commands occur.case TransformCommandType::Invalid: + osre_warn(Tag, "Received Invalid transform command"); break;
Line range hint
141-144: Document why Z-axis transforms are ignoredThe code explicitly ignores Z-axis transformations without explaining why. Consider adding a comment explaining the rationale behind this decision to help future maintainers.
case TransformCommandType::TransformCommandZPositive: case TransformCommandType::TransformCommandZNegative: - // ignored + // Z-axis transforms are ignored because [add reason here] break;src/Engine/IO/IOService.cpp (1)
Remove ZIP-related code remnants for consistency
There are still ZIP-related references that need to be cleaned up:
Zip_Extensionconstant insrc/Engine/IO/IOService.cpp- ZIP extension registration in
src/Engine/App/AssetBundle.h- ZIP-related test cases in
test/UnitTests/src/App/AssetBundleTest.cpp🔗 Analysis chain
Line range hint
31-31: Remove unused ZIP-related constant.The
Zip_Extensionconstant appears to be unused after the removal of ZIP file handling capabilities.Let's verify if there are any remaining ZIP-related references that need cleanup:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining ZIP-related code rg -i "zip|\.zip" --type cpp --type hpp --type hLength of output: 78
Script:
#!/bin/bash # Search for any remaining ZIP-related code with correct file extensions rg -i "zip|\.zip" --type cpp --type h # Also search for the specific constant name rg "Zip_Extension" --type cpp --type h # List all files to understand available extensions fd -e cpp -e h -e hppLength of output: 13780
src/Engine/Threading/TAsyncQueue.h (2)
33-35: Fix typo and enhance thread-safety documentationThe class documentation has a typo ("thread-save" should be "thread-safe") and could benefit from more detailed thread-safety guarantees.
-/// @brief This template class implements a thread-save queue. +/// @brief This template class implements a thread-safe queue. +/// +/// This queue provides thread-safe operations through internal synchronization. +/// All public methods are protected by a critical section, ensuring safe +/// concurrent access from multiple threads.
88-91: Remove unnecessary empty commentThe "empty" comment doesn't add value and could be removed for better clarity.
inline TAsyncQueue<T>::TAsyncQueue() : mCriticalSection(), mEnqueueEvent(), - mItemQueue() { - // empty -} + mItemQueue() { +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
CMakeLists.txt(0 hunks)contrib/cppcore(1 hunks)contrib/unzip/CMakeLists.txt(0 hunks)contrib/unzip/ChangeLogUnzip(0 hunks)contrib/unzip/crypt.h(0 hunks)contrib/unzip/ioapi.c(0 hunks)contrib/unzip/ioapi.h(0 hunks)contrib/unzip/unzip.c(0 hunks)contrib/unzip/unzip.h(0 hunks)contrib/vcpkg(1 hunks)samples/01_ModelLoading/ModelLoading.cpp(1 hunks)src/Engine/App/TransformController.cpp(1 hunks)src/Engine/CMakeLists.txt(1 hunks)src/Engine/IO/IOService.cpp(1 hunks)src/Engine/IO/IOService.h(0 hunks)src/Engine/IO/ZipFileStream.cpp(0 hunks)src/Engine/IO/ZipFileStream.h(0 hunks)src/Engine/IO/ZipFileSystem.cpp(0 hunks)src/Engine/IO/ZipFileSystem.h(0 hunks)src/Engine/Threading/TAsyncQueue.h(3 hunks)
💤 Files with no reviewable changes (13)
- contrib/unzip/ChangeLogUnzip
- src/Engine/IO/ZipFileSystem.h
- src/Engine/IO/IOService.h
- contrib/unzip/CMakeLists.txt
- contrib/unzip/unzip.c
- CMakeLists.txt
- src/Engine/IO/ZipFileStream.cpp
- contrib/unzip/unzip.h
- contrib/unzip/ioapi.h
- contrib/unzip/ioapi.c
- src/Engine/IO/ZipFileStream.h
- contrib/unzip/crypt.h
- src/Engine/IO/ZipFileSystem.cpp
✅ Files skipped from review due to trivial changes (2)
- contrib/cppcore
- contrib/vcpkg
🔇 Additional comments (7)
src/Engine/CMakeLists.txt (1)
457-457: LGTM: Library dependencies updated correctly
The removal of the Unzip library from target_link_libraries is consistent with the removal of ZIP functionality from the project.
src/Engine/IO/IOService.cpp (2)
87-89: LGTM! Good modernization of pointer handling.
The changes improve code clarity by:
- Using modern C++ nullptr instead of NULL
- Making the null check more explicit
109-109: LGTM! Good optimization.
The added empty check for mMountedMap is a nice optimization that avoids unnecessary map lookup operations.
samples/01_ModelLoading/ModelLoading.cpp (2)
35-36: LGTM! Include placement follows best practices.
The addition of <iostream> is properly placed after system/project includes and is necessary for the std::cout usage in the output methods.
35-36: Verify model loading compatibility after ZIP removal.
Given the removal of ZIP file handling capabilities mentioned in the PR summary, we should verify that model loading still works correctly for all supported formats.
✅ Verification successful
Let me gather more information about the model loading functionality to make a proper assessment.
Based on the gathered information, I can now provide a final response:
Model loading remains fully functional despite ZIP removal
The codebase shows that model loading is primarily handled through Assimp and doesn't depend on ZIP functionality. The AssimpWrapper implementation uses direct file loading with proper import flags (aiProcess_*) and supports all Assimp-compatible formats. The only ZIP-related code found is in the asset bundle system, which is separate from the core model loading functionality.
Key findings:
- Model loading is implemented in AssimpWrapper with standard Assimp import flags
- Direct file loading is used via Uri system
- ZIP support was only used in asset bundling, not in model loading
- All core model loading tests and functionality remain intact
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining ZIP-related functionality or test cases
rg -i "zip|\.zip" --type cpp --type h
# Look for supported file formats in Assimp wrapper
ast-grep --pattern 'importAsset($_, $_) {
$$$
}'
Length of output: 381
Script:
#!/bin/bash
# Check model loading implementation and supported formats
rg -A 10 "loadAsset|importAsset|LoadModel" --type cpp
# Look for Assimp initialization and format configurations
rg -i "assimp|aiProcess|aiScene" --type cpp --type h
# Check for any model loading tests
fd "test.*\.(cpp|h)$" --exec rg -l "Model|Asset.*Load"
Length of output: 13849
src/Engine/Threading/TAsyncQueue.h (2)
81-83: Excellent improvement in memory management
Converting pointer members to direct instances is a great improvement that:
- Eliminates potential memory leaks
- Follows RAII principles
- Reduces complexity
- Improves exception safety
145-147: LGTM: Utility methods are thread-safe and well-implemented
The size, isEmpty, and clear methods are correctly protected by critical sections and implement their functionality appropriately.
Also applies to: 161-163, 170-174
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
src/Engine/IO/IOService.cpp (2)
98-98: Fix typo in error message.There's a typo in the error message: "Incalid" should be "Invalid".
- osre_error(Tag, "Incalid stream."); + osre_error(Tag, "Invalid stream.");
82-110: Consider documenting the removal of ZIP functionality.Given that these changes are part of a larger cleanup removing ZIP functionality, it would be helpful to:
- Add a comment in the header file documenting the supported file systems
- Update any relevant documentation about supported file formats
- Consider adding migration guides if this is a breaking change for users
This will help maintain clear documentation of the system's capabilities.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Engine/IO/IOService.cpp(1 hunks)src/Engine/Threading/TAsyncQueue.h(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Engine/Threading/TAsyncQueue.h
🔇 Additional comments (4)
src/Engine/IO/IOService.cpp (4)
82-84: LGTM! Improved pointer initialization and null check clarity.
The changes improve code readability with explicit nullptr initialization and clearer null checking syntax.
92-98: Improved error handling with proper null checks.
The addition of explicit null checks with error logging enhances the robustness of the code.
104-104: LGTM! Consistent null check style.
The null check follows the same explicit style used throughout the file.
110-110: LGTM! Good performance optimization.
The addition of the empty check prevents unnecessary map lookup operations when no file systems are mounted.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
src/Engine/IO/IOService.cpp (2)
91-103: Fix typo in error message and verify null checks.The error handling is well-implemented, but there's a typo in the error message ("Incalid" should be "Invalid").
Here's the fix:
if (nullptr == *ppStream) { - osre_error(Tag, "Incalid stream."); + osre_error(Tag, "Invalid stream."); return; }
37-37: Consider adding RAII-based resource management.While the current implementation is functional, consider these architectural improvements:
- Use smart pointers for filesystem management to prevent memory leaks
- Consider adding a filesystem factory for better extensibility
- Add logging for successful operations to aid in debugging
Example implementation using smart pointers:
using FileSystemPtr = std::unique_ptr<AbstractFileSystem>; using MountedMap = std::unordered_map<String, FileSystemPtr>;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Engine/IO/IOService.cpp(2 hunks)
🔇 Additional comments (3)
src/Engine/IO/IOService.cpp (3)
35-37: LGTM! Good use of constexpr for the tag.
The constant definition and constructor initialization are well-implemented.
109-109: LGTM! Good optimization.
The empty check before searching the map is a good optimization.
81-87:
Remove unused variable and simplify the code.
The pStream variable is declared but never used, as flagged by static analysis.
Here's the suggested fix:
Stream *IOService::openStream(const Uri &file, Stream::AccessMode mode) {
- Stream *pStream = nullptr;
if (AbstractFileSystem *fs = getFileSystem(file.getScheme()); fs != nullptr) {
return fs->open( file, mode );
}
return nullptr;
}Likely invalid or redundant comment.
| void IOService::umountFileSystem( const String &schema, AbstractFileSystem *fileSystem ) { | ||
| MountedMap::iterator it = mMountedMap.find(schema); | ||
| if ( mMountedMap.end() == it ) { | ||
| return; | ||
| } | ||
| if (it->second == pFileSystem) { | ||
| if (it->second == fileSystem) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add parameter validation and error logging.
While the unmounting logic is correct, it should validate the input parameter and provide feedback when the filesystem isn't found.
Here's the suggested improvement:
void IOService::umountFileSystem( const String &schema, AbstractFileSystem *fileSystem ) {
+ if (fileSystem == nullptr) {
+ osre_error(Tag, "Null filesystem provided");
+ return;
+ }
MountedMap::iterator it = mMountedMap.find(schema);
if ( mMountedMap.end() == it ) {
+ osre_debug(Tag, "No filesystem mounted for schema: " + schema);
return;
}
if (it->second == fileSystem) {
mMountedMap.erase(it);
+ } else {
+ osre_debug(Tag, "Filesystem mismatch for schema: " + schema);
}
}Committable suggestion skipped: line range outside the PR's diff.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
src/Engine/IO/IOService.cpp (4)
81-86: Add error logging for failed filesystem lookup.While the error handling is correct, adding logging would help with debugging when a filesystem for the given scheme is not found.
Stream *IOService::openStream(const Uri &file, Stream::AccessMode mode) { if (AbstractFileSystem *fs = getFileSystem(file.getScheme()); fs != nullptr) { return fs->open( file, mode ); } + osre_debug(Tag, "No filesystem found for scheme: " + file.getScheme()); return nullptr; }
96-96: Fix typo in error message.There's a typo in the error message: "Incalid" should be "Invalid".
- osre_error(Tag, "Incalid stream."); + osre_error(Tag, "Invalid stream.");
90-102: Add logging for filesystem lookup failure.When closing a stream, we should log an error if we can't find the corresponding filesystem.
const String &schema( (*ppStream)->getUri().getScheme() ); AbstractFileSystem *pFS = getFileSystem( schema ); if (pFS != nullptr) { pFS->close( ppStream ); + } else { + osre_error(Tag, "No filesystem found for scheme: " + schema); }
35-35: Consider standardizing error handling patterns.While error logging has been improved, consider creating a consistent pattern for error handling across all methods:
- Input validation with appropriate error logging
- Operation-specific error logging
- Standard error return values or exceptions
This will make the code more maintainable and debugging easier.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Engine/IO/IOService.cpp(3 hunks)
🔇 Additional comments (3)
src/Engine/IO/IOService.cpp (3)
121-124: LGTM!
The implementation is clean, with proper null checking and const correctness.
67-68:
Add null pointer validation.
The method should validate the fileSystem parameter before storing it in the map to prevent potential null pointer issues.
void IOService::mountFileSystem( const String &schema, AbstractFileSystem *fileSystem ) {
+ if (fileSystem == nullptr) {
+ osre_error(Tag, "Null filesystem provided");
+ return;
+ }
mMountedMap[ schema ] = fileSystem;
}40-40: Review commented-out filesystem initialization.
The commented line suggests that the default filesystem initialization was intentionally disabled. This could lead to issues if code expects a "file" schema handler to be available by default.
Let's check if there are any dependencies on the "file" schema:
✅ Verification successful
The commented-out code is safe to remove as filesystem initialization is properly handled elsewhere
The commented line can be safely removed because:
- The LocaleFileSystem is properly initialized in
IOService::onOpen()where it's created and mounted usingmountFileSystem(pFileSystem->getSchema(), pFileSystem). - The "file" schema is consistently used throughout the codebase and its initialization is not dependent on the commented line.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of "file" schema across the codebase
rg -l '"file".*getSchema|getScheme'
Length of output: 142
Script:
#!/bin/bash
# Let's check the actual usage context of "file" schema in these files
rg '"file".*getSchema|getScheme' -A 3 -B 3
# Also check for any direct usage of LocaleFileSystem
rg "LocaleFileSystem" -A 3 -B 3
# Check if there are any other places where filesystems are mounted
ast-grep --pattern 'mMountedMap[$_] = $_'
Length of output: 8679
| } | ||
|
|
||
| IOService::IOService() : AbstractService( "io/ioserver" ), mMountedMap() { | ||
| IOService::IOService() : AbstractService("io/ioserver"), mMountedMap() { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using smart pointers for better memory management.
The current implementation uses raw pointers for both file systems and streams. Consider using std::unique_ptr for clearer ownership semantics and automatic resource cleanup.
Example refactoring:
using FileSystemPtr = std::unique_ptr<AbstractFileSystem>;
using MountedMap = std::unordered_map<String, FileSystemPtr>;This would eliminate the need for manual deletion in onClose() and prevent potential memory leaks.
| CREATE_SINGLETON( IOService ); | ||
|
|
||
| mMountedMap["file"] = new LocaleFileSystem(); | ||
| // mMountedMap["file"] = new LocaleFileSystem(); |
Check notice
Code scanning / CodeQL
Commented-out code Note
Summary by CodeRabbit
New Features
Bug Fixes
Chores
Refactor
Documentation