Bugfix/fix several bugs #258
Conversation
WalkthroughThis pull request introduces several modifications across multiple files in the OSRE (Open Source Rendering Engine) project. The changes primarily focus on enhancing asset management, improving camera setup logic, and refining rendering workflows. Key updates include the addition of an asset loading method in the Changes
Sequence DiagramsequenceDiagram
participant App as OsreEdApp
participant UI as User Interface
participant World as World
participant Camera as CameraComponent
participant Asset as ImportAction
App->>World: Create new world
App->>Camera: Setup camera
UI->>App: Request asset import
App->>UI: Open file dialog
UI-->>App: Select model location
App->>Asset: Create ImportAction
Asset->>World: Load asset
App->>Camera: Update bounding box
App->>World: Render scene
Possibly related PRs
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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: 2
🧹 Nitpick comments (3)
src/Engine/Common/Object.cpp (1)
54-56: Consider adding null check for objNameWhile the simplified name assignment is good, consider adding a null check to prevent potential issues with empty strings.
void Object::setName( const String &objName ) { + if (objName.empty()) { + return; + } if( mObjectName != objName ) { mObjectName = objName; } }src/Engine/App/CameraComponent.cpp (1)
Line range hint
94-108: Consider enhancing AABB observation flexibilityWhile the AABB observation logic works, consider these improvements:
- Allow configurable camera position relative to bounding box
- Add support for different up vectors based on scene orientation
- Consider adding margin parameter for better framing
- glm::vec3 eye(-diam *0.5f, -diam *0.5f, diam *0.5f), up(0, 0, 1); + // Allow configurable camera angle and distance + const float cameraAngle = glm::radians(45.0f); // Configurable viewing angle + const float margin = 1.1f; // Add 10% margin for better framing + + glm::vec3 eye( + -diam * margin * cos(cameraAngle), + -diam * margin * cos(cameraAngle), + diam * margin * sin(cameraAngle) + ); + + // Allow dynamic up vector based on scene orientation + glm::vec3 up = determineUpVector(center, eye);src/Engine/Common/TAABB.h (1)
Line range hint
139-159: Improved AABB merging logicThe removal of the
reset()call from the merge method is a good fix. Now the AABB properly expands to encompass new points incrementally, rather than resetting bounds each time. This ensures correct bounding volume calculation when merging multiple points.Consider adding validation to ensure that the AABB is properly initialized before the first merge operation, perhaps through a separate
isValid()method.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/Editor/src/OsreEdApp.cpp(0 hunks)src/Editor/src/main.cpp(1 hunks)src/Engine/App/CameraComponent.cpp(2 hunks)src/Engine/Common/Object.cpp(2 hunks)src/Engine/Common/Object.h(1 hunks)src/Engine/Common/TAABB.h(1 hunks)src/Engine/Common/osre_common.h(3 hunks)src/Engine/RenderBackend/OGLRenderer/OGLRenderBackend.cpp(2 hunks)
💤 Files with no reviewable changes (1)
- src/Editor/src/OsreEdApp.cpp
🔇 Additional comments (9)
src/Editor/src/main.cpp (1)
66-69: LGTM: Improved main loop control flow
The change from done to running improves code readability by making the loop's continuation condition more intuitive. The loop now clearly continues while the application is running, controlled by the event handler's return value.
Let's verify the event handler's return value usage:
✅ Verification successful
Verified: Event handler return value is consistently used across the codebase
The change from done to running aligns with the codebase's established pattern. The search results show that handleEvents() is consistently used as a loop control condition across multiple files (samples and main applications), always following the same pattern where the loop continues while handleEvents() returns true. The base implementation in AppBase confirms this behavior.
src/Engine/App/AppBase.hdefines the virtual methodhandleEvents()returning bool- All sample applications (
HelloWorld.cpp,Demo2D.cpp, etc.) use the same pattern:while(app.handleEvents()) - The editor's implementation follows the same logic, just with an explicit
runningvariable
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if handleEvents() is consistently used for loop control in other files
# Look for similar main loops or event handling patterns
ast-grep --pattern 'handleEvents()' -A 5
Length of output: 42
Script:
#!/bin/bash
# Let's try a more flexible pattern to find handleEvents method and its usage
ast-grep --pattern 'handleEvents'
# Also search with ripgrep for any occurrences including string literals and comments
rg "handleEvents" -A 5
Length of output: 3646
src/Engine/Common/Object.cpp (1)
30-33: LGTM: Simplified object name initialization
The constructor now directly assigns the object name without hashing, which aligns with the PR objective of fixing object name initialization. This change reduces complexity and potential issues with name handling.
src/Engine/Common/Object.h (1)
81-81: LGTM: Simplified string type usage
The change from OsreString to String reduces complexity and improves maintainability by using a standard string type. This change aligns well with the PR objective of fixing object name initialization.
src/Engine/App/CameraComponent.cpp (2)
70-71: LGTM: Consistent member variable naming
The resolution member variable access now follows consistent lowercase naming conventions.
130-131: LGTM: Consistent resolution calculation
The resolution calculation in setOrthoMode correctly uses the new member names and maintains proper width/height calculations.
src/Engine/Common/osre_common.h (2)
79-79: LGTM: Appropriate warning suppression
The addition of warning 4996 suppression is properly scoped within MSVC checks and addresses a common warning about deprecated functions.
481-483: LGTM: Enhanced move semantics control
Good improvement to the OSRE_NON_COPYABLE macro by also deleting move operations. This ensures complete control over object lifetime and follows modern C++ best practices.
src/Engine/RenderBackend/OGLRenderer/OGLRenderBackend.cpp (2)
117-117: LGTM: Improved error logging
Good change to use std::to_string instead of custom string conversion. This simplifies the code by using standard library functionality and improves maintainability.
134-134: LGTM: Consistent error logging
Similar improvement as in setMatrix, using std::to_string for consistent error logging across the codebase.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
test/UnitTests/src/Scene/TAABBTest.cpp (1)
76-79: LGTM! Consider adding a complementary test case.The new test case correctly verifies that merging a point inside the AABB doesn't expand the maximum bounds. This aligns with the PR objective to fix AABB merging.
Consider adding a similar test case to verify that merging an internal point also doesn't affect the minimum bounds:
glm::vec3 isIn(30, 30, 30); aabb.merge(isIn); EXPECT_EQ(newMax, aabb.getMax()); + EXPECT_EQ(newMin, aabb.getMin()); // Verify minimum bounds are unchangedtest/UnitTests/src/Common/CommonTest.cpp (1)
131-134: Consider adding more test cases for TResolutionWhile the current test case verifies basic area calculation, consider adding tests for:
- Edge cases (0x0, max values)
- Different aspect ratios
- Type limits for template parameter T
TEST_F(CommonTest, TResolutionTest) { TResolution<ui32> res(100u, 200u); EXPECT_EQ(res.getArea(), 20000); + + // Edge cases + TResolution<ui32> zero(0u, 0u); + EXPECT_EQ(zero.getArea(), 0); + + // Different aspect ratios + TResolution<ui32> square(100u, 100u); + EXPECT_EQ(square.getArea(), 10000); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/Editor/src/main.cpp(1 hunks)src/Engine/Common/osre_common.h(3 hunks)test/UnitTests/src/Common/CommonTest.cpp(2 hunks)test/UnitTests/src/Scene/TAABBTest.cpp(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Editor/src/main.cpp
🔇 Additional comments (5)
test/UnitTests/src/Common/CommonTest.cpp (1)
49-53: LGTM: Handle member variable renaming is consistent
The renaming of m_idx to idx improves naming consistency while maintaining the same test coverage.
src/Engine/Common/osre_common.h (4)
147-170: LGTM: Handle structure changes are consistent
The renaming of the member variable and corresponding method updates maintain consistency and functionality.
466-474: LGTM: TResolution member variable renaming is complete
The renaming of Width/Height to width/height improves naming consistency, and all related code has been properly updated.
482-484: Verify usage of OSRE_NON_COPYABLE macro
The macro now prevents both copy and move operations. Verify that no existing code relies on move semantics for affected classes.
✅ Verification successful
Based on the search results, I can generate the final response as we have sufficient information:
OSRE_NON_COPYABLE macro changes are safe to proceed
The macro is used in low-level engine components (TransformMatrixBlock, Shader, VertComponent, VertexLayout, BufferData, PrimitiveGroup, Texture, Material, Mesh) that typically shouldn't support move semantics anyway. No std::move usage was found in the codebase that could be affected by this change.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for classes using the OSRE_NON_COPYABLE macro
rg "OSRE_NON_COPYABLE" -A 5 -B 5
# Search for potential move operations
ast-grep --pattern 'std::move($_)'
Length of output: 6982
79-79: Verify the impact of disabling warning 4996
Disabling warning 4996 (deprecated function warnings) might mask important deprecation notices. Consider:
- Using the newer recommended functions instead
- Selectively disabling the warning only around specific code segments
|
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Refactor
Tests