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

Implement bounding volumes for primitive shapes #11336

Merged
merged 17 commits into from
Jan 18, 2024

Conversation

Jondolf
Copy link
Contributor

@Jondolf Jondolf commented Jan 13, 2024

Objective

Closes #10570.

#10946 added bounding volume types and traits, but didn't use them for anything yet. This PR implements Bounded2d and Bounded3d for Bevy's primitive shapes.

Solution

Implement Bounded2d and Bounded3d for primitive shapes. This allows computing AABBs and bounding circles/spheres for them.

For most shapes, there are several ways of implementing bounding volumes. I took inspiration from Parry's bounding volumes, Inigo Quilez, and figured out the rest myself using geometry. I tried to comment all slightly non-trivial or unclear math to make it understandable.

Parry uses support mapping (finding the farthest point in some direction for convex shapes) for some AABBs like cones, cylinders, and line segments. This involves several quat operations and normalizations, so I opted for the simpler and more efficient geometric approaches shown in Quilez's article.

Below you can see some of the bounding volumes working in 2D and 3D. Note that I can't conveniently add these examples yet because they use primitive shape meshing, which is still WIP.

2024-01-10.23-13-57.mp4
2024-01-13.01-26-14.mp4

Changelog

  • Implemented Bounded2d/Bounded3d for primitive shapes
  • Added from_point_cloud method for bounding volumes (used by many bounding implementations)
  • Added point_cloud_2d/3d_center and rotate_vec2 utility functions
  • Added RegularPolygon::vertices method (used in regular polygon AABB construction)
  • Added Triangle::circumcenter method (used in triangle bounding circle construction)
  • Added bounding circle/sphere creation from AABBs and vice versa

Extra

Do we want to implement Bounded2d for some "3D-ish" shapes too? For example, capsules are sort of dimension-agnostic and useful for 2D, so I think that would be good to implement. But a cylinder in 2D is just a rectangle, and a cone is a triangle, so they wouldn't make as much sense to me. A conical frustum would be an isosceles trapezoid, which could be useful, but I'm not sure if computing the 2D AABB of a 3D frustum makes semantic sense.

@Jondolf Jondolf added C-Enhancement A new feature A-Math Fundamental domain-agnostic mathematical operations labels Jan 13, 2024
@alice-i-cecile
Copy link
Member

Do we want to implement Bounded2d for some "3D-ish" shapes too? For example, capsules are sort of dimension-agnostic and useful for 2D, so I think that would be good to implement. But a cylinder in 2D is just a rectangle, and a cone is a triangle, so they wouldn't make as much sense to me. A conical frustum would be an isosceles trapezoid, which could be useful, but I'm not sure if computing the 2D AABB of a 3D frustum makes semantic sense.

I think we should split this out into a follow-up to avoid controversy. Personally, i think you probably want some sort of explicit projection to 2D before using this: the rotation is not always consistent. Alternatively, we could add 2D versions of the missing useful shapes (like capsules).

@Jondolf Jondolf marked this pull request as ready for review January 14, 2024 14:55
@Jondolf
Copy link
Contributor Author

Jondolf commented Jan 14, 2024

Should be ready for review now. I fixed some docs, simplified line and plane stuff, and added basic tests for all of the bounding implementations.

Most of the tests are for the trivial non-rotated case since computing the expected values for the rotated versions can be rather time-consuming, but at least the core functionality should now be tested. I think it'd be enough if we later added bounding volume examples (like in my earlier videos) where you could see the bounding volumes at all kinds of rotations and shapes and sizes.

I also changed the Triangle2d bounding circle to use the circumcircle, similarly to the Cone bounding sphere. For this, I extracted the logic into a Triangle2d::circumcircle method, so we also get a nice helper out of it.

@Jondolf
Copy link
Contributor Author

Jondolf commented Jan 14, 2024

Hmm... I'm not sure if we actually want to use the circumcircle/circumsphere for triangles, cones and conical frusta. The old approach of getting the AABB and computing the bounding circle for that might be better for more cases.

For some cases, the circumcircle does give a tighter fit:

(circumcircle)
Näyttökuva 2024-01-14 171641

(bounding circle of AABB)
Näyttökuva 2024-01-14 151957

But if the triangle has one long side and two shorter legs, the circumcircle puts the center very far away since it needs to pass through all vertices:

(circumcircle)
Näyttökuva 2024-01-14 171536

(bounding circle of AABB)
Näyttökuva 2024-01-14 171431

I feel like the bounding circle of the AABB is better on average even if it doesn't give the tightest fit in all cases. It's also more consistent in that it has the same center as the AABB instead of some completely different center.

Would it be good if I changed the triangle, cone and conical frustum implementations to use the AABB bounding circle/sphere approach, or am I perhaps missing some better alternative? I think the circumcircle helper is still nice to have, but I could split it into a separate PR since it wouldn't be needed here anymore.

@alice-i-cecile
Copy link
Member

Would it be good if I changed the triangle, cone and conical frustum implementations to use the AABB bounding circle/sphere approach, or am I perhaps missing some better alternative? I think the circumcircle helper is still nice to have, but I could split it into a separate PR since it wouldn't be needed here anymore.

I think swapping it back is good: the consistency is valuable. I like the circumcircle helper still: it's a nice little algorithm. Having a slower method to compute an exact minimal bounding volume would also be useful for static cases.

@Jondolf Jondolf mentioned this pull request Jan 15, 2024
46 tasks
Copy link
Contributor

@IQuick143 IQuick143 left a comment

Choose a reason for hiding this comment

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

I think the PR is in a good shape, I've done a thorough read and I've suggested some tweaks that might improve perf or simplify the code. (Some might be just nitpicks though.)

crates/bevy_math/src/bounding/bounded2d/mod.rs Outdated Show resolved Hide resolved
crates/bevy_math/src/bounding/bounded2d/primitive_impls.rs Outdated Show resolved Hide resolved
crates/bevy_math/src/bounding/bounded2d/primitive_impls.rs Outdated Show resolved Hide resolved
crates/bevy_math/src/bounding/bounded3d/mod.rs Outdated Show resolved Hide resolved
/// The bounding sphere is not guaranteed to be the smallest possible.
#[inline(always)]
pub fn from_point_cloud(translation: Vec3, rotation: Quat, points: &[Vec3]) -> BoundingSphere {
let center = point_cloud_3d_center(points);
Copy link
Contributor

Choose a reason for hiding this comment

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

Okay so here I'm not super sure, but maybe instead of taking the geometric center, taking the min-max center (center of the AABB) might produce better results. (Same would apply to the 2D case I guess). It would have certain desirable properties (like being independent of points that are in the middle of the cloud), but I'm not sure it'd be better in real cases.

crates/bevy_math/src/bounding/bounded3d/primitive_impls.rs Outdated Show resolved Hide resolved
crates/bevy_math/src/bounding/bounded3d/primitive_impls.rs Outdated Show resolved Hide resolved
crates/bevy_math/src/bounding/bounded3d/primitive_impls.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@nvdaz nvdaz left a comment

Choose a reason for hiding this comment

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

Looks pretty good. Examples would definitely be nice to visualize this (ofc, when primitive meshing is added)

crates/bevy_math/src/bounding/bounded2d/mod.rs Outdated Show resolved Hide resolved
crates/bevy_math/src/bounding/bounded2d/mod.rs Outdated Show resolved Hide resolved
crates/bevy_math/src/bounding/bounded3d/mod.rs Outdated Show resolved Hide resolved
crates/bevy_math/src/bounding/bounded2d/primitive_impls.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@IQuick143 IQuick143 left a comment

Choose a reason for hiding this comment

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

Alright, I think the PR is good!

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.

Almost ready! The panics docs are important (it's easy to pass in an empty slice), but the others are non-blocking.

@alice-i-cecile alice-i-cecile 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 Jan 18, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Jan 18, 2024
Merged via the queue into bevyengine:main with commit c62ad4b Jan 18, 2024
23 checks passed
@Jondolf Jondolf deleted the bounded-impls branch January 18, 2024 16:15
github-merge-queue bot pushed a commit that referenced this pull request Jan 29, 2024
# Objective

Currently, the `Capsule` primitive is technically dimension-agnostic in
that it implements both `Primitive2d` and `Primitive3d`. This seems good
on paper, but it can often be useful to have separate 2D and 3D versions
of primitives.

For example, one might want a two-dimensional capsule mesh. We can't
really implement both 2D and 3D meshing for the same type using the
upcoming `Meshable` trait (see #11431). We also currently don't
implement `Bounded2d` for `Capsule`, see
#11336 (comment).

Having 2D and 3D separate at a type level is more explicit, and also
more consistent with the existing primitives, as there are no other
types that implement both `Primitive2d` and `Primitive3d` at the same
time.

## Solution

Rename `Capsule` to `Capsule3d` and add `Capsule2d`. `Capsule2d`
implements `Bounded2d`.

For now, I went for `Capsule2d` for the sake of consistency and clarity.
Mathematically the more accurate term would be `Stadium` or `Pill` (see
[Wikipedia](https://en.wikipedia.org/wiki/Stadium_(geometry))), but
those might be less obvious to game devs. For reference, Godot has
[`CapsuleShape2D`](https://docs.godotengine.org/en/stable/classes/class_capsuleshape2d.html).
I can rename it if others think the geometrically correct name is better
though.

---

## Changelog

- Renamed `Capsule` to `Capsule3d`
- Added `Capsule2d` with `Bounded2d` implemented

---------

Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
tjamaan pushed a commit to tjamaan/bevy that referenced this pull request Feb 6, 2024
# Objective

Currently, the `Capsule` primitive is technically dimension-agnostic in
that it implements both `Primitive2d` and `Primitive3d`. This seems good
on paper, but it can often be useful to have separate 2D and 3D versions
of primitives.

For example, one might want a two-dimensional capsule mesh. We can't
really implement both 2D and 3D meshing for the same type using the
upcoming `Meshable` trait (see bevyengine#11431). We also currently don't
implement `Bounded2d` for `Capsule`, see
bevyengine#11336 (comment).

Having 2D and 3D separate at a type level is more explicit, and also
more consistent with the existing primitives, as there are no other
types that implement both `Primitive2d` and `Primitive3d` at the same
time.

## Solution

Rename `Capsule` to `Capsule3d` and add `Capsule2d`. `Capsule2d`
implements `Bounded2d`.

For now, I went for `Capsule2d` for the sake of consistency and clarity.
Mathematically the more accurate term would be `Stadium` or `Pill` (see
[Wikipedia](https://en.wikipedia.org/wiki/Stadium_(geometry))), but
those might be less obvious to game devs. For reference, Godot has
[`CapsuleShape2D`](https://docs.godotengine.org/en/stable/classes/class_capsuleshape2d.html).
I can rename it if others think the geometrically correct name is better
though.

---

## Changelog

- Renamed `Capsule` to `Capsule3d`
- Added `Capsule2d` with `Bounded2d` implemented

---------

Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
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-Enhancement A new feature 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.

Bounding volumes for primitive shapes
4 participants