Skip to content

Support transforming bounding volumes #11681

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

Merged
merged 10 commits into from
Mar 5, 2024

Conversation

Jondolf
Copy link
Contributor

@Jondolf Jondolf commented Feb 3, 2024

Objective

Make it straightforward to translate and rotate bounding volumes.

Solution

Add translate_by/translated_by, rotate_by/rotated_by, transform_by/transformed_by methods to the BoundingVolume trait. This follows the naming used for mesh transformations (see #11454 and #11675).


Changelog

  • Added translate_by/translated_by, rotate_by/rotated_by, transform_by/transformed_by methods to the BoundingVolume trait and implemented them for the bounding volumes
  • Renamed Position associated type to Translation

@Jondolf Jondolf added C-Feature A new feature, making something new possible A-Math Fundamental domain-agnostic mathematical operations labels Feb 3, 2024
@Jondolf
Copy link
Contributor Author

Jondolf commented Feb 3, 2024

I changed it so that the rotation methods also rotate the centers of bounding volumes around the origin. This is important when you have e.g. a local AABB of a collider that is offset, and need to transform the AABB to match the collider's global pose. This is also what Parry does.

@mockersf
Copy link
Member

mockersf commented Feb 3, 2024

adding rotation to something called "axis aligned" feel strange to me

@Jondolf
Copy link
Contributor Author

Jondolf commented Feb 3, 2024

For prior art, this is Parry's equivalent: https://docs.rs/parry3d/latest/parry3d/bounding_volume/struct.Aabb.html#method.transform_by

There are cases where AABBs are stored in local space and then transformed into world space, which requires computing the rotated version of the AABB. It's not computing the actual OBB (Oriented Bounding Box) but the AABB of the rotated shape. I agree rotation might not be the best term, but I'm not sure what a better name would be.

@alice-i-cecile
Copy link
Member

This feels more like a revolve operation than a rotate: you're sweeping the object rotationally. Maybe there's a better name for all of the operations in there?

We could call this transform_sweep or something.

@Jondolf
Copy link
Contributor Author

Jondolf commented Feb 3, 2024

It's not necessarily a sweep or revolve in the sense that if you rotated a uniform AABB 90 degrees, it would be the same as the original AABB, not some AABB that contains all of the "swept" space. It's essentially just a merged version of the original AABB and the "rotated" one.

@Jondolf
Copy link
Contributor Author

Jondolf commented Feb 3, 2024

If it was transform_sweep, it would also imply that the starting position (before translation) would be contained within the resulting AABB. This is what swept_aabb in Parry returns. Here, however, the AABB only contains the volume of the "rotated" AABB at the end position, not the full swept AABB.

@alice-i-cecile
Copy link
Member

Okay, noted. Hmm, not quite correct either then. I'm fine to just follow parry's naming for now.

@Jondolf
Copy link
Contributor Author

Jondolf commented Feb 27, 2024

Some more prior art in terms of naming and why we even need bounding volume transformations like these. Quotes from Christer Ericson's famous book Real-Time Collision Detection (chapters 4.1 and 4.2):

Desirable properties for bounding volumes include: [list cut down for brevity] Easy to rotate and transform

Bounding volumes are typically computed in a preprocessing step rather than at
runtime. Even so, it is important that their construction does not negatively affect
resource build times. Some bounding volumes, however, must be realigned at runtime
when their contained objects move. For these, if the bounding volume is expensive
to compute realigning the bounding volume is preferable (cheaper) to recomputing
it from scratch.

Computing an encompassing bounding box for a rotated AABB using the min-max representation can therefore be implemented as follows:

We could also remove the separate rotation and translation methods and only keep transform_by, but I think they're still nice to have.

Even more prior art:

@NthTensor
Copy link
Contributor

The bounding boxes remain axis-aligned after rotation/transformation, right? Docs need to state that much more clearly. Otherwise seems useful.

Copy link
Contributor

@NthTensor NthTensor left a comment

Choose a reason for hiding this comment

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

This change is well motivated and matches the implementation in parry. I agree the naming is confusing but it conforms with standard practice and the docs clarify things further.

Looks good for merge, but I do have a non-blocking question about intended use. rotate_by appears to be non-decreasing in volume. Do repeated rotations by pi/4 cause the box extents to grow monotonically? How do you avoid arbitrarily large bounding boxes after repeated transformations?

@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 Mar 4, 2024
@Jondolf
Copy link
Contributor Author

Jondolf commented Mar 4, 2024

Do repeated rotations by pi/4 cause the box extents to grow monotonically?

Yes.

How do you avoid arbitrarily large bounding boxes after repeated transformations?

The idea here is that we store the AABB in local space and only rotate that every time.

From Real-Time Collision Detection, chapter 4.2.6:

Last of the four realignment methods, the most common approach is to simply wrap
the rotated AABB itself in a new AABB. This produces an approximate rather than
a tight AABB. As the resulting AABB is larger than the one that was started with, it
is important that the approximate AABB is computed from a rotation of the original
local-space AABB. If not, repeated recomputing from the rotated AABB of the previous
time step would make the AABB grow indefinitely.

@NthTensor
Copy link
Contributor

NthTensor commented Mar 4, 2024

That makes sense. I would love it if you could work that into the docs somewhere (unless it already is and I missed it).

Edit for clarity: Specifically the bit about it only producing an "approximate" bounding box, and it usually not being good practice to rotate the same box multiple times.

@Jondolf
Copy link
Contributor Author

Jondolf commented Mar 5, 2024

Added a mention of how rotation can result in the AABB not being as tightly fitting, and how you should avoid successive rotations to the same AABB. This only really applies to axis-aligned volumes, so I didn't add this on the BoundingVolume trait itself but rather the AABB methods.

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Mar 5, 2024
@NthTensor
Copy link
Contributor

Looks great! Could we get that note added to the 3d bounds as well? This is looking really really solid.

Merged via the queue into bevyengine:main with commit 921ba54 Mar 5, 2024
@Jondolf
Copy link
Contributor Author

Jondolf commented Mar 5, 2024

Dunno how I missed that... That's what I get for writing docs at midnight 😅

Made a quick PR to add the note for Aabb3d: #12315

@Jondolf Jondolf deleted the bounding-transformations branch March 5, 2024 09:58
github-merge-queue bot pushed a commit that referenced this pull request Mar 5, 2024
# Objective

Fixes #12310.

#11681 added transformations for bounding volumes, but I accidentally
only added a note in the docs about repeated rotations for `Aabb2d` and
not `Aabb3d`.

## Solution

Copy the docs over to `Aabb3d`.
@Jondolf Jondolf mentioned this pull request Mar 7, 2024
spectria-limina pushed a commit to spectria-limina/bevy that referenced this pull request Mar 9, 2024
# Objective

Make it straightforward to translate and rotate bounding volumes.

## Solution

Add `translate_by`/`translated_by`, `rotate_by`/`rotated_by`,
`transform_by`/`transformed_by` methods to the `BoundingVolume` trait.
This follows the naming used for mesh transformations (see bevyengine#11454 and
bevyengine#11675).

---

## Changelog

- Added `translate_by`/`translated_by`, `rotate_by`/`rotated_by`,
`transform_by`/`transformed_by` methods to the `BoundingVolume` trait
and implemented them for the bounding volumes
- Renamed `Position` associated type to `Translation`

---------

Co-authored-by: Mateusz Wachowiak <[email protected]>
spectria-limina pushed a commit to spectria-limina/bevy that referenced this pull request Mar 9, 2024
# Objective

Fixes bevyengine#12310.

bevyengine#11681 added transformations for bounding volumes, but I accidentally
only added a note in the docs about repeated rotations for `Aabb2d` and
not `Aabb3d`.

## Solution

Copy the docs over to `Aabb3d`.
mtsr pushed a commit to mtsr/bevy that referenced this pull request Mar 15, 2024
# Objective

Make it straightforward to translate and rotate bounding volumes.

## Solution

Add `translate_by`/`translated_by`, `rotate_by`/`rotated_by`,
`transform_by`/`transformed_by` methods to the `BoundingVolume` trait.
This follows the naming used for mesh transformations (see bevyengine#11454 and
bevyengine#11675).

---

## Changelog

- Added `translate_by`/`translated_by`, `rotate_by`/`rotated_by`,
`transform_by`/`transformed_by` methods to the `BoundingVolume` trait
and implemented them for the bounding volumes
- Renamed `Position` associated type to `Translation`

---------

Co-authored-by: Mateusz Wachowiak <[email protected]>
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.

5 participants