Skip to content
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.

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

Improve Segment2d/Segment3d API and docs #18206

Merged
merged 11 commits into from
Mar 9, 2025

Conversation

Jondolf
Copy link
Contributor

@Jondolf Jondolf commented Mar 9, 2025

Objective

#17404 reworked the Segment2d and Segment3d types to be defined by two endpoints rather than a direction and half-length. However, the API is still very minimal and limited, and documentation is inconsistent and outdated.

Solution

Add the following helper methods for Segment2d and Segment3d:

  • from_scaled_direction
  • from_ray_and_length
  • length_squared
  • direction
  • try_direction
  • scaled_direction
  • transformed
  • reversed

Segment2d has a few 2D-specific methods:

  • left_normal
  • try_left_normal
  • scaled_left_normal
  • right_normal
  • try_right_normal
  • scaled_right_normal

There are now also From implementations for converting [Vec2; 2] and (Vec2, Vec2) to a Segment2d, and similarly for 3D.

I have also updated documentation to be more accurate and consistent, and simplified a few methods.


Prior Art

Parry's Segment type has a lot of similar methods, though my implementation is a bit more comprehensive. A lot of these methods can be useful for various geometry algorithms.

@Jondolf Jondolf added C-Docs An addition or correction to our documentation C-Usability A targeted quality-of-life change that makes Bevy easier to use A-Math Fundamental domain-agnostic mathematical operations X-Uncontroversial This work is generally agreed upon D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Mar 9, 2025
Copy link
Contributor

@greeble-dev greeble-dev left a comment

Choose a reason for hiding this comment

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

Suggested some small changes.

@alice-i-cecile alice-i-cecile added this to the 0.16 milestone Mar 9, 2025
@alice-i-cecile alice-i-cecile added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Mar 9, 2025
@alice-i-cecile alice-i-cecile enabled auto-merge March 9, 2025 19:24
@Jondolf
Copy link
Contributor Author

Jondolf commented Mar 9, 2025

@alice-i-cecile Those only changed the 2D methods so 3D is now inconsistent. I can fix it in a sec though

auto-merge was automatically disabled March 9, 2025 19:35

Head branch was pushed to by a user without write access

@alice-i-cecile
Copy link
Member

Thank you very much @Jondolf :)

@alice-i-cecile alice-i-cecile enabled auto-merge March 9, 2025 19:35
Copy link
Contributor

github-actions bot commented Mar 9, 2025

Your PR caused a change in the graphical output of an example or rendering test. This might be intentional, but it could also mean that something broke!
You can review it at https://pixel-eagle.com/project/B04F67C0-C054-4A6F-92EC-F599FEC2FD1D?filter=PR-18206

If it's expected, please add the M-Deliberate-Rendering-Change label.

If this change seems unrelated to your PR, you can consider updating your PR to target the latest main branch, either by rebasing or merging main into it.

@Jondolf
Copy link
Contributor Author

Jondolf commented Mar 9, 2025

Also I would prefer if the reversing method memswapped like what I originally had, since that's what I personally need for my use cases (e.g. I have a segment clipping algo where I need to sort features, which includes swapping things around).

We could have both reverse (in-place mutation) and reversed (returns a new segment) to match the existing API of Triangle2d/Triangle3d. I can do that either here or in a follow-up, either is fine for me

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Mar 9, 2025
@alice-i-cecile alice-i-cecile removed this pull request from the merge queue due to a manual request Mar 9, 2025
@alice-i-cecile
Copy link
Member

@Jondolf let's add both here.

@Jondolf
Copy link
Contributor Author

Jondolf commented Mar 9, 2025

Done! Also added #[must_use] to reversed to be more explicit about it returning a new segment instead of mutating the original segment

@alice-i-cecile alice-i-cecile enabled auto-merge March 9, 2025 20:13
@alice-i-cecile
Copy link
Member

Much appreciated: you always do such excellent work.

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Mar 9, 2025
auto-merge was automatically disabled March 9, 2025 21:17

Pull Request is not mergeable

Merged via the queue into bevyengine:main with commit 9f6d628 Mar 9, 2025
31 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Math Fundamental domain-agnostic mathematical operations C-Docs An addition or correction to our documentation C-Usability A targeted quality-of-life change that makes Bevy easier to use D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it X-Uncontroversial This work is generally agreed upon
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants