Conversation
There was a problem hiding this comment.
Pull request overview
This PR extends the projection Camera API to optionally disable out-of-bounds rejection during world→NDC/screen projection, and adjusts naming around NDC→world unprojection.
Changes:
- Added an
auto_clipboolean option toworld_to_screen()andworld_to_view_port()to control whether out-of-bounds NDC returns an error. - Gated the NDC bounds check in
world_to_view_port()behindauto_clip. - Renamed
view_port_to_screen()toview_port_to_world()and updatedscreen_to_world()accordingly.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (auto_clip && is_ndc_out_of_bounds(projected)) | ||
| return std::unexpected(Error::WORLD_POSITION_IS_OUT_OF_SCREEN_BOUNDS); |
There was a problem hiding this comment.
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.
| 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); |
There was a problem hiding this comment.
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.
| } | ||
| [[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 |
There was a problem hiding this comment.
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.
| world_to_view_port(const Vector3<float>& world_position, const bool auto_clip = true) const noexcept | ||
| { |
There was a problem hiding this comment.
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.
No description provided.