Adds and (mostly) Implements an Obb2d BoundingVolume - 2D Oriented Bounding Boxes

[kfc35]

Objective

• See if there is appetite for Oriented Bounding Boxes in bevy_math. In Bounding volumes for primitive shapes #10570 , there was a mention of adding an OBB bounding volume as an option for primitives, so I get the impression that OBB’s could be useful.
• Resolve overlap between bevy_render primitives and bevy_math primitives #13945 mentions creating an Obb3d if we pursue removing Sphere from what is now bevy_camera. It mentions that an Obb3d can also be useful outside of rendering. To warm up to doing this, I explored creating and implementing an Obb2d type first. The Obb3d struct would basically be what is in this PR but for three dimensions.

Solution

• Creates and implements an Obb2d that also implements BoundingVolume and can detect collisions with the other two 2d bounding volume types (Aabb2d and BoundingCircle).
• This does NOT include Obb2d in the Bounded2d trait. I figured this can be done as a follow up if we feel good about having what I have written for Obb2d.
• merge is basically stubbed with a created Aabb2d from the two Obb2d’s converted back to an Obb2d. It is not really implemented in true OBB fashion because that seems more complicated to implement, given that the function specifies it should be the “smallest” bounding volume. If possible, I’d like to explore if we can merge this without it truly implemented and have an issue to follow up to really implement it (another math person can help out with implementing that too!).

Testing

• Added unit tests for the important things

[github-actions]

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-22748

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.

[Smerom]

❤️‍🔥

[IQuick143]

I will note that there's no one single smallest bounding box by inclusion, unlike with AABBs.
The reason is that with AABBs if you have two candidates X: AABB and Y: AABB, such that all your things are contained in both X and Y, then their intersection X n Y will also contain everything, and crucially X n Y: AABB. For OOBBs the last property does not hold unless it's a special case.

You could define the smallest OOBB by volume, but that will be really difficult to compute, and possibly not even produce better results in various algorithms.

[kfc35]

Thanks for the wisdom 🙏 😞

I guess then that we can just have a best effort here that is not too complicated and can be executed pretty quickly

As an aside: the reason why I’m focusing on smallest here is because that’s what the BoundingVolume trait says merge should do: /// Computes the smallest bounding volume that contains both self and other. We’d probably have to update that comment if we pursue having an OBB included in the codebase

[IQuick143]

Should there be some sort of clamping to avoid negatives?

[atlv24]

The Obb3d struct would basically be what is in this PR but for three dimensions.

This doesn't really look like any rendering bounding type - whats the plan exactly? Once we have an Obb3d version, assuming it looks like (iso: Isometry3d, half_extents: Vec3), that can't replace neither Aabb nor Sphere. I'm just wary of adding yet another bounding box type without a clear path to reducing it later i guess

[kfc35]

My main motivation to create an Obb3d is to replace Sphere from bevy_camera with BoundingSphere. There are also some functions in bevy_camera that are called intersects_obb that can be changed to accept an Obb3d instead where relevant (i.e. when we can convert Affine3A into a non-identity Isometry3d, with any scaling applied to the half_size). Since Aabb is also treated as an OBB in some cases, having the Obb3d struct can help untangle some of Aabb’s usages so we can figure out what to do next with Aabb.

The Current Situation

The current Sphere impl in bevy_camera’s primitives.rs currently looks like this:

#[derive(Clone, Debug, Default)]
pub struct Sphere {
    pub center: Vec3A,
    pub radius: f32,
}

impl Sphere {
    #[inline]
    pub fn intersects_obb(&self, aabb: &Aabb, world_from_local: &Affine3A) -> bool {
        let aabb_center_world = world_from_local.transform_point3a(aabb.center);
        let v = aabb_center_world - self.center;
        let d = v.length();
        let relative_radius = aabb.relative_radius(&(v / d), &world_from_local.matrix3);
        d < self.radius + relative_radius
    }
}

Note that Aabb here is the bevy_camera::primitives::Aabb with a “center” and “half extents” definition. The Aabb here is treated as an Obb because of the additional transformation argument world_from_local.

The BoundingSphere in bevy_math is basically defined as the same struct (the Sphere referenced in its definition is the one in bevy_math):

// in bounded3d mod.rs
pub struct BoundingSphere {
    /// The center of the bounding sphere
    pub center: Vec3A,
    /// The sphere
    pub sphere: Sphere,
}

// in primitives dim3.rs
pub struct Sphere {
    /// The radius of the sphere
    pub radius: f32,
}

The Plan

An Obb3d would be implemented, which would include IntersectsVolume<BoundingSphere> for Obb3d and vice versa. This would cover implementing the logic behind intersects_obb in the standard bevy_math way.

After an Obb3d is implemented, there will be a conversion method that lives in bevy_camera that takes in an
aabb: &Aabb, world_from_local: &Affine3A and outputs an Obb3d. The resulting isometry would have its:

• rotation as world_from_local’s rotation component (I… think, I need to think about that more)
• translation as world_from_local.transform_point3a(aabb.center), as implied in the body of the function intersects_obb

For the obb’s half_size, the scale that is in world_from_local would be applied to aabb.half_extents.
(I assume that Affine3A can be neatly decomposed into rotation translation and scale via to_scale_rotation_translation since it comes from a GlobalTransform)

Then, I’d replace any usage of a bevy_camera Sphere (right now it appears to only be used in check_point_light_mesh_visibility in bevy_light for spot lights and point lights) with a BoundingSphere and call its intersects method with an Obb3d (from implementing IntersectsVolume<Obb3d>). Then Sphere in bevy_camera can be removed 💥

Of course there should be testing done to ensure the change over to using the new bevy_math bounding volume does not disrupt anything.

[atlv24]

After an Obb3d is implemented, there will be a conversion method that lives in bevy_camera that takes in an
aabb: &Aabb, world_from_local: &Affine3A and outputs an Obb3d.

This is going to be a performance bottleneck, and probably a regression, i'd wager. Converting an Affine3A into an isometry is nontrivial and this is the hot path of frustum culling.

[kfc35]

I wasn’t aware that extracting an Isometry3d from Affine3A would costly but I can see that now from you and from reading a code comment on GlobalTransform 😞

I knew the situation would have to be handled delicately because of the hot path, but it’s a bummer that using Isometry isn’t an option.

I’m fine with closing this pull since the plan does not satisfy my main motivations anymore (unless we find value in OBB’s generally… but I doubt we want to contribute to bloat in bevy_math). I personally would not follow #13945 ’s suggestion of creating an Obb3d with an Affine3A in it. I’ll keep this open for a week if anyone has anything else to say but my intention is to close this then, after which I’ll note in that issue the conversation here regarding trying to resolve the overlap with an Obb3d that has an Isometry3d inside.

🙏 Thanks for your review and input

[atlv24]

I think the crux of the problem with unifying bevy_render math with bevy_math is theres a fundamental impedance mismatch: bevy_render takes approximation and assumptions to serve the purpose of "render as fast as possible" meanwhile bevy_math seemingly focuses on mathematical correctness and accuracy, for better or worse. This is hard to reconcile, and arguably begs the question of what we are even trying to achieve by reconciling.

[IQuick143]

I will also note that converting an affine into an isometry will be in general impossible. Though in the cases where the affine transformation isn't an isometry, the intersects_obb method likely produces wrong results anyway.