Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds an axis-aligned bounding box (AABB) primitive and introduces frustum-culling support for AABBs on the generic projection camera, with unit tests covering several culling scenarios across engine camera variants.
Changes:
- Add
omath::primitives::Aabbtype. - Add
Camera::is_aabb_culled_by_frustum(...)using view-projection plane extraction. - Add unit tests validating AABB frustum culling for Source/Unity/OpenGL camera conventions.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
tests/general/unit_test_projection.cpp |
Adds a suite of AABB/frustum culling unit tests across multiple engine camera types. |
include/omath/projection/camera.hpp |
Adds AABB frustum culling API to the core templated camera. |
include/omath/3d_primitives/aabb.hpp |
Introduces a new AABB struct (min/max corners). |
Comments suppressed due to low confidence (1)
include/omath/projection/camera.hpp:11
camera.hppusesstd::array(including in the newly added frustum/AABB code) but does not include<array>directly, relying on transitive includes via other headers. This makes the header fragile if includes change; add#include <array>here.
#include "omath/3d_primitives/aabb.hpp"
#include "omath/linear_algebra/mat.hpp"
#include "omath/linear_algebra/triangle.hpp"
#include "omath/linear_algebra/vector3.hpp"
#include "omath/projection/error_codes.hpp"
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| [[nodiscard]] bool is_aabb_culled_by_frustum(const primitives::Aabb<float>& aabb) const noexcept | ||
| { |
There was a problem hiding this comment.
The API naming is inconsistent: this introduces is_aabb_culled_by_frustum(...) while an existing overload uses is_culled_by_frustum(...) for triangles. Consider making this an overload named is_culled_by_frustum(const primitives::Aabb<float>&) (or otherwise aligning the naming) to improve discoverability and consistency.
| TEST(UnitTestProjection, AabbCulledUnityEngine) | ||
| { | ||
| constexpr auto fov = omath::projection::FieldOfView::from_degrees(60.f); | ||
| const auto cam = omath::unity_engine::Camera({0, 0, 0}, {}, {1280.f, 720.f}, fov, 0.03f, 1000.f); | ||
|
|
||
| // Box in front — not culled | ||
| const omath::primitives::Aabb<float> inside{{-1.f, -1.f, 50.f}, {1.f, 1.f, 100.f}}; | ||
| EXPECT_FALSE(cam.is_aabb_culled_by_frustum(inside)); | ||
|
|
There was a problem hiding this comment.
This test name is misleading: AabbCulledUnityEngine contains both an "inside" (not culled) assertion and a "behind" (culled) assertion. Consider renaming it to reflect both behaviors or splitting into two focused tests so failures are easier to interpret.
| TEST(UnitTestProjection, AabbUnityEngineSideCulled) | ||
| { | ||
| constexpr auto fov = omath::projection::FieldOfView::from_degrees(60.f); | ||
| const auto cam = omath::unity_engine::Camera({0, 0, 0}, {}, {1280.f, 720.f}, fov, 0.03f, 1000.f); | ||
|
|
||
| // Box far above (Unity: +Y up) | ||
| const omath::primitives::Aabb<float> aabb{{-1.f, 5000.f, 50.f}, {1.f, 6000.f, 100.f}}; |
There was a problem hiding this comment.
AabbUnityEngineSideCulled appears to be testing an AABB far above the frustum (per the comment and Y values), not a generic "side" cull. Renaming the test to match what it checks (e.g., above/top culling) would reduce confusion when reading failures.
No description provided.