Resolve overlap between `bevy_render` primitives and `bevy_math` primitives
- Dominant language
- Rust
- Stars
- 48.2k
- Forks
- 4.8k
- Avg merge
- 3d 16h
- Merged PRs (30d)
- 171
Description
This is an overview of the current `bevy_render` primitive vs. `bevy_math` primitive situation from my viewpoint, and how I personally think we should tackle this. This is somewhat in response to #13882 (see also #13878 and vaguely related #13455) but is something that has been in discussion for a long time.
Feel free to disagree and suggest alternative approaches :)
## The problems
`bevy_render` currently has [rendering primitives](https://docs.rs/bevy/0.13.2/bevy/render/primitives/index.html) like the following:
- `Aabb`
- `Sphere`
- `HalfSpace`
- `Frustum`
All of these can be generally useful types that would be great to have in `bevy_math`, for numerous reasons (interoperability, consistency, more complete APIs, etc.). In fact, `bevy_math` already has `Aabb2d`, `Aabb3d`, and `BoundingSphere`, and #13882 even moves the remaining two types into `bevy_math`. Nice, this means that we can just remove the rendering primitives from `bevy_render` and have them all live happily in `bevy_math`, right?
Well, not quite. These rendering primitives are quite interconnected and have some details that are specific to rendering. For example:
- `Sphere` is used on its own, but it also needs an `intersects_obb` method that takes an `Aabb` (which is actually treated more like an OBB) for cheaply pruning out meshes outside of light spheres.
- `Frustum` (which is a *view frustum*, not just any [frustum](https://en.wikipedia.org/wiki/Frustum)) needs to have methods for intersections against both `Sphere` and `Aabb` (or OBB) for the actual frustum culling.
These methods currently rely on `Aabb` having its center + half-extents representation, which is different from `Aabb3d`, so we *cannot* currently have for example `Frustum::intersects_obb` in `bevy_math` without also having `Aabb` there, unless we're fine with some extra cost or rework the logic. And we obviously don't want to have both `Aabb` and `Aabb3d` in `bevy_math`, at least under such similar names.
In addition, **`Aabb` and `Frustum` need to be components in `bevy_render` anyway.**
- `Aabb` is a component used for mesh/sprite/text bounds and frustum culling.
- `Frustum` is a component used by both lights and cameras for some culling.
They even have documentation specific to their behavior and usage in rendering, which would not make any sense in `bevy_math`.
#13882 takes the approach of just adding an optional `bevy_ecs` feature and a `legacy_bevy_render` module to `bevy_math` to simply chuck these types in there, with the intent of cleaning up later. However, I think this is a bad idea *especially* without a clear plan of *how* and *when* we will actually clean it up. This is not a particularly trivial thing to resolve or "clean up", and even easier things have slipped into releases even though the intent was to resolve them quickly after the initial merge (ex: missing plane subdivisions). I don't think `bevy_math` should ever have these kinds of things even momentarily without pressing reasons.
(Aside: Personally, I'm not sure if a view frustum even makes sense living in `bevy_math`, especially since it'd need some custom intersection methods anyway. It's not what I typically consider to be a geometric primitive even if it is a rendering primitive. But I guess it could still be considered to be a "generally useful gamedev math type", so I wouldn't fight against it.)
## What solution would you like?
Okay, so all of this is tricky and I feel like there's no clear solution, but I'll start by listing what I personally think we should do:
- Keep `Aabb` in `bevy_render`.
- We need the component for mesh/sprite/text bounds and frustum culling anyway.
- In my opinion it should definitely be renamed to be distinct from `Aabb2d`/`Aabb3d` though.
- Later, we should test the implications of changing the internal representation to use `Aabb3d` (and benchmark!)
- *Maybe* remove `Sphere` in favor of just `BoundingSphere`.
- The only method missing from `BoundingSphere` is `intersects_obb`. This is kind of a weird one in terms of its input, and currently depends on the center + half-extents representation of `Aabb`.
- *If* we remove `Sphere` from `bevy_render`, I think our options are:
- Just have a standalone `sphere_intersects_obb` function in `bevy_render`, using `BoundingSphere` and (for now) `Aabb`.
- Same as above, but with an extension trait for `BoundingSphere`.
- Add an OBB type in `bevy_math`. See the "OBBs and removing `Sphere`" section.
- Move `HalfSpace` to `bevy_math`, like #13882 does. I think it's generally useful enough (also usable for collision stuff, for example), and not tied to rendering like the other types kind of are.
- Move or don't move `Frustum` to `bevy_math`, either option is fine to me.
- If we *do* move it, I would rename it to `ViewFrustum`. It is not a general frustum geometry-wise, and we already have e.g. `ConicalFrustum`. If we had `Frustum`, you'd think `ConicalFrustum` is a specialized version of it, but it isn't; they're both distinct frustum types.
- Additionally, if we *do* move it, we need a newtype component in `bevy_render`.
- The same `intersects_obb` issue as with `Sphere`; we don't want the rendering `Aabb` in `bevy_math`.
I do *not* want:
- A dependency on `bevy_ecs` in `bevy_math` even optionally, unless it is for a very pressing reason. The geometric primitives should *not* be components, for that we should just create wrapper types in the respective crates.
- A legacy rendering module or similar in `bevy_math`. From a design standpoint, why would `bevy_math` have a temporary stash for `bevy_render` types? We could just as well have a `legacy_primitives` module in `bevy_render` which is what the types are actually for. Although they're not even legacy types necessarily, since `bevy_render` needs to keep some AABB component type (and frustum component) anyway, and `Sphere` just needs to be refactored away.
### OBBs and removing `Sphere`
For the `intersects_obb` issue mentioned earlier, one option is to add an `Obb3d` bounding volume type to `bevy_math`. It can be useful even outside of rendering.
Currently, the method looks like this:
```rust
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
}
}
```
We could store the required data in an actual type:
```rust
// Note: I'm not sold on this representation. It includes scale unlike other bounding volumes,
// and I believe it is not the best for e.g. OBB-OBB intersection tests.
pub struct Obb3d {
pub affine: Affine3A,
pub half_size: Vec3,
}
```
Then, we can implement the functionality that currently exists on `Aabb` and `Sphere`:
```rust
impl Obb3d {
/// Creates an [`Obb3d`] from an [`Aabb3d`] and [`Affine3A`].
pub fn from_aabb_and_affine(aabb: Aabb3d, mut affine: Affine3A) -> Self {
affine.translation = affine.transform_point3a(aabb.center());
Self {
affine,
half_size: aabb.half_size(),
}
}
// This is currently on `Aabb`, although I don't love the method name
/// Calculate the relative radius of the AABB with respect to a plane
#[inline]
pub fn relative_radius(&self, plane_normal: Vec3A) -> f32 {
// NOTE: dot products on Vec3A use SIMD and even with the overhead of conversion are net faster than Vec3
let half_extents = self.half_size;
Vec3A::new(
plane_normal.dot(self.affine.matrix3.x_axis),
plane_normal.dot(self.affine.matrix3.y_axis),
plane_normal.dot(self.affine.matrix3.z_axis),
)
.abs()
.dot(half_extents)
}
}
impl IntersectsVolume for BoundingSphere {
// This is `Sphere::intersect_obb`, but slightly modified and better commented :)
fn intersects(&self, obb: &Obb3d) -> bool {
let center_offset = obb.affine.translation - self.center;
let center_distance = center_offset.length();
// Compute the distance from the OBB center to its boundary in the direction of the sphere center.
let relative_radius = obb.relative_radius(center_offset / center_distance);
// If the minimum distance from the sphere center to the OBB
// is less than the sphere radius, the shapes intersect.
center_distance < self.radius() + relative_radius
}
}
```
(Note: I have *not* tested the above, but functionally it should hopefully be the same as before)
This would let us cleanly get rid of `Sphere`, remove the dependence on `Aabb` for both `Sphere` and `Frustum`, and overall just give `bevy_math` some more useful functionality (I also have a form of OBB-OBB intersections implemented already). The OBB construction has a *slight* extra cost because of the min + max representation of `Aabb3d`, and the scaling in `Affine3A` might be a bit iffy for some things, but it should probably be alright. Again, benchmarking would probably be important for rendering people.
## Alternatives
Just leave things as is. I think we should do *something* though, like (some of) the things I listed earlier.
Contributor guide
Research direction
Start by reviewing the Aabb, Sphere, HalfSpace, and Frustum implementations and their relationships across bevy_render and bevy_math, then read the linked discussion in #13882. This issue presents several alternative designs rather than a settled change, so completion would require an agreed ownership and representation plan before implementation and benchmarking.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- rust
- Domain
- computer-graphics
- Issue type
- Refactor
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Stale
- Clarity
- Needs clarification
- Newbie friendliness
- 25/100