Feature/aabb improvemnt#200
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces template parameters for UpAxis in the Aabb struct, adds a vertices() method to retrieve all corners of an AABB, adds a default constructor to CanvasBox, and implements a static from_aabb factory method in EntityOverlay to construct overlays from 3D bounding boxes. The review feedback suggests improving header includes by explicitly including <utility>, <expected>, and <string> to avoid relying on transitive includes. Additionally, it recommends optimizing the from_aabb projection logic by tracking scalar coordinate values instead of copying full vectors, and returning descriptive error messages instead of empty strings in std::unexpected for better debuggability.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
|
||
| #pragma once | ||
| #include "omath/linear_algebra/vector3.hpp" | ||
| #include <array> |
There was a problem hiding this comment.
The file uses std::unreachable() in top() and bottom(), but does not directly include <utility>. Relying on transitive includes can cause compilation failures on different compilers or standard library implementations. Please include <utility> directly.
| #include <array> | |
| #include <array> | |
| #include <utility> |
| #include <memory> | ||
| #include <string_view> |
There was a problem hiding this comment.
| Vector3<float> top; | ||
| Vector3<float> bottom; | ||
| bool has_projected_vertex = false; | ||
|
|
||
| for (const auto& vertex: aabb.vertices()) | ||
| { | ||
| if (auto projected = camera.world_to_screen_unclipped(vertex)) | ||
| { | ||
| if (!has_projected_vertex) | ||
| { | ||
| top = *projected; | ||
| bottom = *projected; | ||
| has_projected_vertex = true; | ||
| continue; | ||
| } | ||
|
|
||
| if (projected->y < top.y) | ||
| top = *projected; | ||
| if (projected->y > bottom.y) | ||
| bottom = *projected; | ||
| } | ||
| } | ||
|
|
||
| if (!has_projected_vertex) | ||
| return std::unexpected(""); | ||
|
|
||
| auto center = camera.world_to_screen_unclipped(aabb.center()); | ||
| if (!center) | ||
| return std::unexpected(""); | ||
|
|
||
| return EntityOverlay{{center->x, top.y}, {center->x, bottom.y}, aspect, renderer}; |
There was a problem hiding this comment.
In from_aabb, top and bottom are declared as Vector3<float> and updated by copying the entire projected vector, even though only their y coordinates are used at the end. We can optimize this by tracking float top_y and float bottom_y directly, which avoids unnecessary vector copies and default-initialization. Additionally, returning descriptive error messages in std::unexpected instead of empty strings will greatly improve debuggability.
float top_y = 0.f;
float bottom_y = 0.f;
bool has_projected_vertex = false;
for (const auto& vertex : aabb.vertices())
{
if (auto projected = camera.world_to_screen_unclipped(vertex))
{
if (!has_projected_vertex)
{
top_y = projected->y;
bottom_y = projected->y;
has_projected_vertex = true;
continue;
}
if (projected->y < top_y)
top_y = projected->y;
if (projected->y > bottom_y)
bottom_y = projected->y;
}
}
if (!has_projected_vertex)
return std::unexpected("No vertices were projected");
auto center = camera.world_to_screen_unclipped(aabb.center());
if (!center)
return std::unexpected("Failed to project AABB center");
return EntityOverlay{{center->x, top_y}, {center->x, bottom_y}, aspect, renderer};
No description provided.