Feature: Enable frustum in camera #255
Conversation
|
Warning Rate limit exceeded@kimkulling has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 18 minutes and 59 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe pull request introduces several modifications across multiple files in the project. Key changes include enhanced error handling and logging in 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 (
|
|
This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (5)
src/Engine/App/World.h (2)
Line range hint
43-44: Complete the missing documentationThe following methods need proper documentation:
getEntityByName: Missing parameter and return value descriptionsgetEntityArray: Missing parameter descriptionsetSceneRoot: Missing parameter descriptionAlso applies to: 45-46, 47-48
Line range hint
142-142: Fix typo in member variable nameThe member variable
mDirtryappears to have a typo and should bemDirty.- bool mDirtry; + bool mDirty;test/UnitTests/src/Common/FrustumTest.cpp (1)
45-58: Ensure comprehensive test coverage for frustum inclusion tests.The
isInTestfunction effectively tests points inside and outside the frustum. Consider adding more test cases to cover edge cases, such as points exactly on the frustum planes, and testing with different frustum configurations to ensure robustness.Apply this diff to add an edge case test:
+ // Test point exactly on the near plane + glm::vec3 pt3(0, 0, 0.1f); + bool onNearPlane = f.isIn(pt3); + EXPECT_TRUE(onNearPlane);src/Engine/Common/Frustum.h (1)
Line range hint
77-85: Optimize theisInmethod by early exit on plane test failure.The current implementation checks all planes even if the point is outside one of them. You are already breaking the loop when
inbecomesfalse, but you can return immediately to improve performance.Apply this diff to return immediately upon detection:
for (size_t i = 0; i < mPlanes.size(); ++i) { const Plane &plane = mPlanes[i]; const f32 d = plane.param.x * point.x + plane.param.y * point.y + plane.param.z * point.z + plane.param.w; if (d < 0.0f) { - in = false; - break; + return false; } } -return in; +return true;src/Engine/App/CameraComponent.h (1)
165-166: Maintain consistent initialization order in the constructor.Ensure that the new member variables
mViewProjectionandmFrustumare properly initialized in the constructor ofCameraComponent. This helps prevent any uninitialized variable usage and maintains consistency.Add the initialization to the constructor:
CameraComponent::CameraComponent(Entity *owner) : Component(owner, ComponentType::CameraComponentType), // Existing initializations... mUp(0, 0, 1), mView(1), mProjection(1), + mViewProjection(1), + mFrustum() { // empty }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
src/Engine/App/AssetRegistry.cpp(0 hunks)src/Engine/App/CameraComponent.cpp(1 hunks)src/Engine/App/CameraComponent.h(2 hunks)src/Engine/App/TransformComponent.h(1 hunks)src/Engine/App/World.cpp(0 hunks)src/Engine/App/World.h(1 hunks)src/Engine/Common/Frustum.h(2 hunks)test/UnitTests/src/Common/FrustumTest.cpp(2 hunks)
💤 Files with no reviewable changes (2)
- src/Engine/App/World.cpp
- src/Engine/App/AssetRegistry.cpp
✅ Files skipped from review due to trivial changes (1)
- src/Engine/App/TransformComponent.h
🧰 Additional context used
🪛 cppcheck (2.10-2)
test/UnitTests/src/Common/FrustumTest.cpp
[error] 35-35: syntax error
(syntaxError)
🔇 Additional comments (3)
src/Engine/App/World.h (1)
57-57: Verify memory management with defaulted destructor
The change to use a defaulted destructor requires careful consideration as the class manages several pointer members (mEntities, mActiveCamera, mRoot, mPipeline). Please ensure:
- No memory leaks occur from pointer members
- Base class (Common::Object) cleanup is properly handled
- Resource ownership is clearly documented
Let's verify the memory management requirements:
✅ Verification successful
Default destructor is safe to use
The defaulted destructor is appropriate here because:
- All member pointers (mEntities, mActiveCamera, mRoot, mPipeline) are non-owning pointers
- The base class
Common::Objecthas a virtual destructor with default implementation - The container
TArraymanages its own cleanup - Entity ownership is managed externally through explicit
addEntity/removeEntitymethods
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if Common::Object has virtual destructor and memory management requirements
rg -A 5 "class.*Object" src/Engine/Common/Object.h
# Check if there are any explicit delete/cleanup calls in the old implementation
rg -A 5 "World::~World" src/Engine/App/World.cpp
# Look for ownership documentation or memory management patterns
rg -A 5 "ownership|cleanup|delete|release" src/Engine/App/World.
Length of output: 397
Script:
#!/bin/bash
# Check World class definition and member variables
ast-grep --pattern 'class World {
$$$
}'
# Check if there are any explicit cleanup patterns in World.cpp
rg -A 5 "delete|cleanup|clear" src/Engine/App/World.cpp
# Look for smart pointer usage in World.h
rg -A 5 "unique_ptr|shared_ptr|weak_ptr" src/Engine/App/World.h
# Check member variable declarations in World class
rg -A 10 "class World" src/Engine/App/World.h
Length of output: 216
Script:
#!/bin/bash
# Check World.h content directly
cat src/Engine/App/World.h
# Check World.cpp content directly
cat src/Engine/App/World.cpp
# Look for any files with World in the name to ensure we're looking at the right paths
fd World
Length of output: 10021
src/Engine/App/CameraComponent.cpp (2)
159-159: Ensure mFrustum.extractFrom handles the updated matrix correctly.
After updating mViewProjection, you're extracting the frustum planes using mFrustum.extractFrom(mViewProjection);. Verify that the extractFrom method expects the matrix in the order you've provided and that it's correctly processing the combined view-projection matrix.
Run the following script to find all usages of extractFrom to confirm consistent usage:
✅ Verification successful
Let me search for more context about the Frustum class and its implementation.
The extractFrom method correctly handles the view-projection matrix
The implementation in src/Engine/Common/Frustum.h shows that extractFrom method correctly processes the combined view-projection matrix by:
- Extracting rows from the matrix using
glm::row - Computing the frustum planes through standard view-frustum extraction formulas
- Properly normalizing each plane
The usage in CameraComponent.cpp matches the expected input parameter of const glm::mat4 &vp and follows the common practice for frustum extraction from a view-projection matrix.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for all instances of mFrustum.extractFrom to ensure consistent matrix usage.
# Expected results: Consistent passing of mViewProjection to extractFrom.
rg --type cpp "mFrustum\.extractFrom\((.*?)\);"
Length of output: 127
Script:
#!/bin/bash
# Search for Frustum class definition and implementation
ast-grep --pattern 'class Frustum {
$$$
}'
# Also search for any header files containing Frustum
fd -e hpp -e h . -x rg -l "class Frustum"
# Search for extractFrom method implementation
ast-grep --pattern 'extractFrom($$$) {
$$$
}'
Length of output: 3990
158-159: Confirm the correctness of the matrix multiplication order.
The calculation of mViewProjection as mProjection * mView may need verification to ensure it aligns with the rest of the codebase and the conventions used. In graphics programming, the order of matrix multiplication is crucial and may vary depending on whether the matrices are column-major or row-major. Since GLM uses column-major matrices by default, and the multiplication is right-associative, the current order might be correct. However, it's important to verify that this order matches the conventions used elsewhere in the project.
Run the following script to check for consistency in the calculation of the view-projection matrix throughout the codebase:
✅ Verification successful
Let me search for more variations of view-projection matrix calculations to ensure consistency.
Let me check the GLM common header to confirm the matrix conventions.
Based on the gathered evidence, I can now provide the final response:
The matrix multiplication order is correct and consistent with the codebase.
The order mProjection * mView is correct because:
- GLM is being used (confirmed via glm_common.h) which follows column-major matrix convention
- This multiplication order is consistent with other view-projection calculations in the codebase, as seen in:
- MaterialBuilder.cpp:
mat4 u_mvp = Projection * View * Model - TransformMatrixBlock.cpp:
mMvp = mProjection * modelView
- MaterialBuilder.cpp:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for instances of view-projection matrix calculations to confirm consistent multiplication order.
# Expected results: Consistent use of mProjection * mView across the codebase.
rg --type cpp "mViewProjection\s*=\s*mProjection\s*\*\s*mView"
Length of output: 142
Script:
#!/bin/bash
# Search for any matrix calculations involving view and projection matrices
rg --type cpp -i "projection.*view|view.*projection" -A 2
# Search for GLM include to verify matrix conventions
rg --type cpp -i "include.*glm"
# Search for any matrix multiplication involving view matrices
rg --type cpp "view.*\*|=.*view"
Length of output: 7259
Script:
#!/bin/bash
# Check the content of glm_common.h to verify matrix conventions
cat src/Engine/Common/glm_common.h
Length of output: 1994
| @@ -32,7 +32,7 @@ class FrustumTest : public ::testing::Test { | |||
| // empty | |||
| }; | |||
|
|
|||
| TEST_F( FrustumTest, createTest ) { | |||
| TEST_F(FrustumTest, createTest) { | |||
There was a problem hiding this comment.
Resolve the syntax error in the test case declaration.
The static analysis tool reports a syntax error at line 35: TEST_F(FrustumTest, createTest) {. Ensure that all necessary headers for the Google Test framework are included and that the TEST_F macro is correctly used.
It seems that the inclusion of the Google Test headers might be missing or incorrect. Do you want assistance in identifying and including the correct headers?
🧰 Tools
🪛 cppcheck (2.10-2)
[error] 35-35: syntax error
(syntaxError)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
src/Engine/Common/Frustum.h (2)
90-90: Useglm::dotfor calculating distance to planeInstead of manually computing the dot product, consider using the
glm::dotfunction for better readability and potential optimization.Apply this diff to refactor the distance calculation:
- const f32 d = plane.param.x * point.x + plane.param.y * point.y + plane.param.z * point.z + plane.param.w; + const f32 d = glm::dot(glm::vec3(plane.param), point) + plane.param.w;
116-117: Simplify plane parameter reset using a zero vectorYou can simplify resetting the plane parameters by assigning a zero vector to
plane.param, enhancing code readability.Apply this diff:
- plane.param.x = plane.param.y = plane.param.z = plane.param.w = 0.0f; + plane.param = glm::vec4(0.0f);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/Engine/App/CameraComponent.h(2 hunks)src/Engine/Common/Frustum.h(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Engine/App/CameraComponent.h
🔇 Additional comments (2)
src/Engine/Common/Frustum.h (2)
63-63: Pass Plane objects by const reference to avoid raw pointers
The constructor Frustum(const Plane *planes) uses a raw pointer, which may lead to ownership issues and potential memory safety problems. Consider passing the planes by const cppcore::TStaticArray<Plane, 6>& to improve safety and clarity.
Apply this diff to modify the constructor declaration:
-explicit Frustum(const Plane *planes);
+explicit Frustum(const cppcore::TStaticArray<Plane, 6> &planes);And update the constructor implementation accordingly:
-inline Frustum::Frustum(const Plane *planes) {
+inline Frustum::Frustum(const cppcore::TStaticArray<Plane, 6> &planes) {
for (ui32 i = 0; i < Count; ++i) {
mPlanes[i] = planes[i];
}
}106-111: Qualify normalize function with glm:: namespace
To ensure proper resolution and maintain consistency with other GLM function calls, the normalize function should be explicitly qualified with the glm:: namespace.
Update the code as follows:
-mPlanes[0].param = normalize(rowW + rowX);
-mPlanes[1].param = normalize(rowW - rowX);
-mPlanes[2].param = normalize(rowW + rowY);
-mPlanes[3].param = normalize(rowW - rowY);
-mPlanes[4].param = normalize(rowW + rowZ);
-mPlanes[5].param = normalize(rowW - rowZ);
+mPlanes[0].param = glm::normalize(rowW + rowX);
+mPlanes[1].param = glm::normalize(rowW - rowX);
+mPlanes[2].param = glm::normalize(rowW + rowY);
+mPlanes[3].param = glm::normalize(rowW - rowY);
+mPlanes[4].param = glm::normalize(rowW + rowZ);
+mPlanes[5].param = glm::normalize(rowW - rowZ);
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
src/Engine/Common/Frustum.h (2)
33-35: Enhance the plane equation documentation.The comment could be more precise about the plane equation format. Consider adding:
- The relationship between the normal vector (a,b,c) and point position
- The geometric interpretation of the distance term d
/// @brief Plane representation -/// Will use the form 0 = a*x + b*y + c*z + d where a, b, c are the norma components -/// and d is the distance to zero. +/// Represents a plane using the equation: ax + by + cz + d = 0, where: +/// - (a,b,c) forms the plane's unit normal vector +/// - d is the signed distance from the origin to the plane along the normal
Line range hint
98-107: Consider optimizing the point-plane distance calculation.The current implementation can be optimized by using vector dot product.
inline bool Frustum::isIn(const glm::vec3 &point ) { bool in = true; for (size_t i = 0; i < mPlanes.size(); ++i) { const Plane &plane = mPlanes[i]; - const f32 d = plane.param.x * point.x + plane.param.y * point.y + plane.param.z * point.z + plane.param.w; + const f32 d = glm::dot(glm::vec3(plane.param), point) + plane.param.w; if (d < 0.0f) { in = false; break; } } return in; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/Engine/Common/Frustum.h(2 hunks)
🔇 Additional comments (5)
src/Engine/Common/Frustum.h (5)
28-28: LGTM!
The addition of the TStaticArray header is appropriate for the new implementation.
36-51: LGTM!
The Plane struct is well-designed with:
- Good encapsulation of plane parameters
- Helpful accessor methods
- Correct implementation of comparison operators
60-91: LGTM!
The Frustum class modifications improve the implementation with:
- Better error handling through Invalid enum
- Safer array bounds checking with Count enum
- More efficient parameter passing in isIn()
- Better encapsulation using TStaticArray
118-123: Qualify normalize with glm:: namespace.
As noted in previous reviews, normalize should be explicitly qualified with glm:: namespace.
- mPlanes[0].param = normalize(rowW + rowX);
- mPlanes[1].param = normalize(rowW - rowX);
- mPlanes[2].param = normalize(rowW + rowY);
- mPlanes[3].param = normalize(rowW - rowY);
- mPlanes[4].param = normalize(rowW + rowZ);
- mPlanes[5].param = normalize(rowW - rowZ);
+ mPlanes[0].param = glm::normalize(rowW + rowX);
+ mPlanes[1].param = glm::normalize(rowW - rowX);
+ mPlanes[2].param = glm::normalize(rowW + rowY);
+ mPlanes[3].param = glm::normalize(rowW - rowY);
+ mPlanes[4].param = glm::normalize(rowW + rowZ);
+ mPlanes[5].param = glm::normalize(rowW - rowZ);127-130: LGTM!
The clear() implementation correctly initializes all plane parameters.
|
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Refactor
Tests