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

Add winding order for Triangle2d #10620

Merged
merged 6 commits into from
Nov 20, 2023

Conversation

Jondolf
Copy link
Contributor

@Jondolf Jondolf commented Nov 17, 2023

Objective

This PR adds some helpers for Triangle2d to work with its winding order. This could also be extended to polygons (and Triangle3d once it's added).

Solution

  • Add WindingOrder enum with Clockwise, CounterClockwise and Invalid variants
    • Invalid is for cases where the winding order can not be reliably computed, i.e. the points lie on a single line and the area is zero
  • Add Triangle2d::winding_order method that uses a signed surface area to determine the winding order
  • Add Triangle2d::reverse method that reverses the winding order by swapping the second and third vertices

The API looks like this:

let mut triangle = Triangle2d::new(
    Vec2::new(0.0, 2.0),
    Vec2::new(-0.5, -1.2),
    Vec2::new(-1.0, -1.0),
);
assert_eq!(triangle.winding_order(), WindingOrder::Clockwise);

// Reverse winding order
triangle.reverse();
assert_eq!(triangle.winding_order(), WindingOrder::CounterClockwise);

I also added tests to make sure the methods work correctly. For now, they live in the same file as the primitives.

Open questions

  • Should it be Counterclockwise or CounterClockwise? The first one is more correct but perhaps a bit less readable. Counter-clockwise is also a valid spelling, but it seems to be a lot less common than counterclockwise.
  • Is WindingOrder::Invalid a good name? Parry uses TriangleOrientation::Degenerate, but I'm not a huge fan, at least as a non-native English speaker. Any better suggestions?
  • Is WindingOrder fine in bevy_math::primitives? It's not specific to a dimension, so I put it there for now.

Copy link
Contributor

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

@Jondolf Jondolf added C-Feature A new feature, making something new possible A-Math Fundamental domain-agnostic mathematical operations labels Nov 17, 2023
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

Should it be Counterclockwise or CounterClockwise? The first one is more correct but perhaps a bit less readable. Counter-clockwise is also a valid spelling, but it seems to be a lot less common than counterclockwise.

Mild preference for CounterClockwise, but that's definitely bikeshedding.

Is WindingOrder::Invalid a good name? Parry uses TriangleOrientation::Degenerate, but I'm not a huge fan, at least as a non-native English speaker. Any better suggestions?

This is clearer than degenerate: the latter is mathematically inspired, but intimidating. Might be a good doc alias though.

Is WindingOrder fine in bevy_math::primitives? It's not specific to a dimension, so I put it there for now.

Yeah that's a fine place for this to live for now.

@Jondolf
Copy link
Contributor Author

Jondolf commented Nov 19, 2023

Changed it to CounterClockwise, I think it might be slightly nicer and I've also seen it used in at least a couple of codebases.

I also added the Degenerate doc alias and added a mention about degenerate cases to the docs to make the meaning clearer

@mockersf mockersf added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Nov 19, 2023
@mockersf mockersf added this pull request to the merge queue Nov 20, 2023
Merged via the queue into bevyengine:main with commit e1c8d60 Nov 20, 2023
22 checks passed
@Jondolf Jondolf deleted the triangle-winding-order branch November 20, 2023 12:34
rdrpenguin04 pushed a commit to rdrpenguin04/bevy that referenced this pull request Jan 9, 2024
# Objective

This PR adds some helpers for `Triangle2d` to work with its winding
order. This could also be extended to polygons (and `Triangle3d` once
it's added).

## Solution

- Add `WindingOrder` enum with `Clockwise`, `Counterclockwise` and
`Invalid` variants
- `Invalid` is for cases where the winding order can not be reliably
computed, i.e. the points lie on a single line and the area is zero
- Add `Triangle2d::winding_order` method that uses a signed surface area
to determine the winding order
- Add `Triangle2d::reverse` method that reverses the winding order by
swapping the second and third vertices

The API looks like this:

```rust
let mut triangle = Triangle2d::new(
    Vec2::new(0.0, 2.0),
    Vec2::new(-0.5, -1.2),
    Vec2::new(-1.0, -1.0),
);
assert_eq!(triangle.winding_order(), WindingOrder::Clockwise);

// Reverse winding order
triangle.reverse();
assert_eq!(triangle.winding_order(), WindingOrder::Counterclockwise);
```

I also added tests to make sure the methods work correctly. For now,
they live in the same file as the primitives.

## Open questions

- Should it be `Counterclockwise` or `CounterClockwise`? The first one
is more correct but perhaps a bit less readable. Counter-clockwise is
also a valid spelling, but it seems to be a lot less common than
counterclockwise.
- Is `WindingOrder::Invalid` a good name? Parry uses
`TriangleOrientation::Degenerate`, but I'm not a huge fan, at least as a
non-native English speaker. Any better suggestions?
- Is `WindingOrder` fine in `bevy_math::primitives`? It's not specific
to a dimension, so I put it there for now.
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-Feature A new feature, making something new possible S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants