bevyengine / bevyengine/bevy

Resolve overlap between `bevy_render` primitives and `bevy_math` primitives

Open
#13,945 1 comment 4 reactions 0 assignees View on GitHub
A-Math A-Rendering C-Code-Quality S-Ready-For-Implementation
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.