Skip to content

Feature/aabb improvemnt#200

Open
orange-cpp wants to merge 6 commits into
mainfrom
feature/aabb_improvemnt
Open

Feature/aabb improvemnt#200
orange-cpp wants to merge 6 commits into
mainfrom
feature/aabb_improvemnt

Conversation

@orange-cpp

Copy link
Copy Markdown
Owner

No description provided.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.

Suggested change
#include <array>
#include <array>
#include <utility>

Comment on lines 11 to 12
#include <memory>
#include <string_view>

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The file uses std::expected and std::string in the new from_aabb function, but does not directly include <expected> or <string>. Please include them directly to avoid relying on transitive includes.

#include <expected>
#include <memory>
#include <string>
#include <string_view>

Comment on lines +177 to +207
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};

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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};

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant