Skip to content

added clip option #175

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
orange-cpp merged 1 commit into from
Mar 21, 2026
+6 −6
Merged

added clip option #175

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions include/omath/projection/camera.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,9 +204,9 @@ namespace omath::projection

template<ScreenStart screen_start = ScreenStart::TOP_LEFT_CORNER>
[[nodiscard]] std::expected<Vector3<float>, Error>
world_to_screen(const Vector3<float>& world_position) const noexcept
world_to_screen(const Vector3<float>& world_position, const bool auto_clip = true) const noexcept
{
const auto normalized_cords = world_to_view_port(world_position);
const auto normalized_cords = world_to_view_port(world_position, auto_clip);
Copy link

Copilot AI Mar 21, 2026

Choose a reason for hiding this comment

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

Typo in variable name: normalized_cords reads like "cords" rather than "coords" (coordinates). Renaming would improve readability, especially since this variable is central to the projection logic.

Copilot uses AI. Check for mistakes.

if (!normalized_cords.has_value())
return std::unexpected{normalized_cords.error()};
Expand Down Expand Up @@ -267,7 +267,7 @@ namespace omath::projection
}

[[nodiscard]] std::expected<Vector3<float>, Error>
world_to_view_port(const Vector3<float>& world_position) const noexcept
world_to_view_port(const Vector3<float>& world_position, const bool auto_clip = true) const noexcept
{
Comment on lines +270 to 271
Copy link

Copilot AI Mar 21, 2026

Choose a reason for hiding this comment

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

The new auto_clip parameter name is misleading: when enabled it doesn't clip coordinates, it rejects out-of-bounds NDC by returning an error. Consider renaming it to something like reject_out_of_bounds/validate_ndc_bounds, or add documentation clarifying that it controls the bounds-check behavior rather than geometric clipping.

Copilot uses AI. Check for mistakes.
auto projected = get_view_projection_matrix()
* mat_column_from_vector<float, Mat4X4Type::get_store_ordering()>(world_position);
Expand All @@ -278,13 +278,13 @@ namespace omath::projection

projected /= w;

if (is_ndc_out_of_bounds(projected))
if (auto_clip && is_ndc_out_of_bounds(projected))
return std::unexpected(Error::WORLD_POSITION_IS_OUT_OF_SCREEN_BOUNDS);
Comment on lines +281 to 282
Copy link

Copilot AI Mar 21, 2026

Choose a reason for hiding this comment

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

The new auto_clip branch changes behavior by allowing out-of-bounds NDC to be returned when auto_clip == false, but there are no unit tests covering this new path. Please add tests that assert: (1) out-of-bounds world positions still error with the default, and (2) the same positions return a value when auto_clip is disabled.

Copilot uses AI. Check for mistakes.

return Vector3<float>{projected.at(0, 0), projected.at(1, 0), projected.at(2, 0)};
}
[[nodiscard]]
std::expected<Vector3<float>, Error> view_port_to_screen(const Vector3<float>& ndc) const noexcept
std::expected<Vector3<float>, Error> view_port_to_world(const Vector3<float>& ndc) const noexcept
Copy link

Copilot AI Mar 21, 2026

Choose a reason for hiding this comment

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

view_port_to_screen was renamed to view_port_to_world, but this is a public method on Camera (in an exported header) and will be a breaking API change for any downstream users. Consider keeping view_port_to_screen as a deprecated forwarding wrapper (or otherwise providing a compatibility alias) and documenting the rename.

Copilot uses AI. Check for mistakes.
{
const auto inv_view_proj = get_view_projection_matrix().inverted();

Expand All @@ -309,7 +309,7 @@ namespace omath::projection
[[nodiscard]]
std::expected<Vector3<float>, Error> screen_to_world(const Vector3<float>& screen_pos) const noexcept
{
return view_port_to_screen(screen_to_ndc<screen_start>(screen_pos));
return view_port_to_world(screen_to_ndc<screen_start>(screen_pos));
}

template<ScreenStart screen_start = ScreenStart::TOP_LEFT_CORNER>
Expand Down
Loading