Add geometric primitives

[aevyrie]

Note: See RFC bevyengine/rfcs#12

Adds geometric primitives to Bevy. These are lightweight mathematical representations of primitives commonly used for bounding volumes and physics.

[aevyrie]

I've opted to fully define these primitives in world space, instead of using transforms, intentionally.

1. These shapes are only useful if fully defined in space, I can't think of a case where you would, for example, reference the radius of a sphere without also referencing the origin
2. Using transforms adds complexity for any operation - you now need to query the GlobalTransform as well as the primitive.
3. If needed, they could still be used with Transforms! However, I would consider this a secondary use case, and these primitives need to be optimized for use in core functionality - frustum culling, bounding volumes, collision, etc.

I'd like to open this up for discussion. any disagreement on the above points?

[MinerSebas]

These additions should probebly also be re-exported by the bevy_internal crate.
Also, the crate needs to be added to the publish.sh inside the tools folder.

[alice-i-cecile]

As an end user, it would be nice to have some alternate construction impls for each of these. E.g. Box by 3 points, plane by 3 points and so on.

These structs should derive at least Clone and Debug, and probably Hash, PartialEq, and Eq. Also probably Reflect?

[alice-i-cecile]

The ability to construct Transform versions of these structs seems very useful in downstream users: it feels like this should be a shared trait method?

[alice-i-cecile]

As a general note, these are very much needed in Bevy itself to a) avoid ecosystem fragmentation b) use in critical internal tools.

[aevyrie]

Great points. Yeah, I'd like to add some constructors and figure out what shared functionality should go into the primitive trait. I could see that trait being useful in the future for checking primitive intersections using the trait.

[aevyrie]

The ability to construct Transform versions of these structs seems very useful in downstream users: it feels like this should be a shared trait method?

This would be done by having a Transform component on the same entity, and any special logic would be handled in the user's systems. These primitives would be blind to this and could be used however you like.

[alice-i-cecile]

Should Line, Point, Arc and Triangle also be implemented as this same sort of primitive? They're not fully 3D, but very common.

[aevyrie]

Should Line, Point, Arc and Triangle also be implemented as this same sort of primitive? They're not fully 3D, but very common.

Yes, I think those "2D" primitives defined in 3d space would make sense to add, as well as capsule, cylinder, and anything else I've forgotten.

I will be updating this PR sporadically, but (anyone!) please leave comments with ideas or feedback as I'm working. I want to keep the scope limited to primitive definition only. We can add helper functions in future PRs.

[alice-i-cecile]

For clarity, I would impl Primitive3d for each of the structs you've created already, even before it has any behavior. It can still be used to make trait objects, and signals the intent to extend the trait.

[aevyrie]

For clarity, I would impl Primitive3d for each of the structs you've created already, even before it has any behavior. It can still be used to make trait objects, and signals the intent to extend the trait.

Yup, I just haven't pushed it yet. :)

[aevyrie]

I've opted to fully define these primitives in world space, instead of using transforms, intentionally.

I've been going back and forth on this for a while, but I'm now feeling fairly confident this is the right choice. I implemented a Primitive3d::outside_plane() function to get a feel for the impact of this choice. It's unsurprisingly much simpler to use if the primitives are fully defined internally, and maybe somewhat surprisingly, more efficient.

Cache Efficiency

• Some primitives such as AAB and Sphere don't need a rotation to be fully defined.
• By using a GlobalTransform, not only is this an unused Quat that fills the cache line, it would also cause redundant change detection on rotations.
• This is especially important for AABs and Spheres, because they are fundamental to collision detection and BV(H), and as such need to be as efficient as possible.
• I still haven't found a case where you would use a Primitive3d without needing this positional information that fully defines the primitive in space. If this holds true, it means that storing the positional data inside the primitive is not a waste of cache, which is normally why you would want to separate the transform into a separate component.

CPU Efficiency

• Storing the primitive's positional information internally serves as a form of memoization.
• Because you need the primitive to be fully defined in world space to run useful operations, this means that with a GlobalTransform you would need to apply the transform to the primitive every time you need to use it.
• By applying this transformation only once (e.g. during transform propagation), we only need to do this computation a single time.

Ergonomics

• As I've already mentioned a few times, primitives need to be fully defined in world space to do anything useful with them.
• By making the primitive components fully defined and standalone, computing operations is as simple as: primitive1.some_function(primitive_2), instead of also having query and pass in 2 GlobalTransforms in the correct order.

Use with Transforms

• For use cases such as oriented bounding boxes, a primitive should be defined relative to its parent.
• In this case, the primitive would still be fully defined internally, but we would need to include primitive updates analogous to the transform propagation system.
• For example, a bounding sphere entity would be a child of a mesh, with a Sphere primitive and a Transform. On updates to the parent's GlobalTransform, the bounding sphere's Transform component would be used to update the Sphere's position and radius by applying the scale and translation to a unit sphere. This could be applied to all primitives, with the update system being optimized for each primitive.

[alice-i-cecile]

This was a fantastic analysis and I agree with your conclusions. A few thoughts:

I still haven't found a case where you would use a Primitive3d without needing this positional information that fully defines the primitive in space. If this holds true, it means that storing the positional data inside the primitive is not a waste of cache, which is normally why you would want to separate the transform into a separate component.

I spent some time thinking about this, and I couldn't come up with a compelling example, especially a performance-critical one.

In this case, the primitive would still be fully defined internally, but we would need to include primitive updates analogous to the transform propagation system.

This is actually an interesting use case for trait queries (cc @Davier whose exploring these) : you could have a single system that queried for a dyn Primitive component or the like, then used the appropriate method on it. This would be clearer and avoid duplication of systems for each new primitive type (which is particularly useful for user code. I'm not sure if they would come with a performance overhead though.

[bitshifter]

project_point3 performs a perspective divide which is only necessary if the transform is not a perspective projection. If the transform was a perspective projection the the vertices would no longer be a box shape, so perhaps it is OK to assume that it is not. For an affine transform transform_point3 is more efficient as it won't do an unnecessary divide.

[bitshifter]

It seems quite expensive calculating these every time this is called. It may be preferable to store the transformed vertices instead of storing it as an aabb and transform?

[aevyrie]

I'm really not sure. I've been deliberating how to make the structs as small as possible, while also retaining information needed to update the primitives. Some algorithms call for AABB box extents, while others need all vertices. Storing only box extents is the most memory efficient, while storing all vertices (might) be more CPU efficient. I don't want to store both in the struct, because that would be even worse for memory use.

The ECS idiomatic way to handle this would be to create a component for each style of struct, and only query the one I need. My hesitation with this approach is this could quickly lead to an explosion in number of types, and I'd like to keep primitives as simple as possible. Maybe namespacing them would be enough, e.g.: aabb::extents and aabb::vertices.

I'm really uncertain. I'll create an RFC once that process has been finalized, and that will hopefully allow a nuanced discussion on the topic.

[aevyrie]

Hey @bitshifter, I really appreciate this review. I'll be making an RFC where we take a closer look at the design of primitives, including looking into any alternative libraries. Would you mind if I ping you when I get that up?

[bitshifter]

Yep, I'm happy to chime in @aevyrie .

[alice-i-cecile]

Is this the standard name for this function / property? Seems a bit confusing for non-planar shapes?

[aevyrie]

I see how this might be confusing. It's checking if a 3d objects is fully on the outer side of the plane - i.e. fully contained by the halfspace in the direction of the plane's normal. I'll poke around and see if I can find a standard name.

[alice-i-cecile]

Can we even define this for lower dimensional shapes embedded in 3D?

[aevyrie]

Yes, all 3d primitives can be bounded by a finite volume. Lower dimensional shapes may have a bounding volume of zero, but it still makes sense to check if they are fully in the outer halfspace of a plane.

[aevyrie]

Even a plane, with an infinite area, can be fully "outside" another plane if both planes are parallel.

[alice-i-cecile]

It might be nice to construct this whole thing in a for loop for clarity? The actual logic is hard to follow.

[aevyrie]

I don't see a way to do this in a loop yet, without making the logic harder to follow. Everything is "unrolled" and named so it's easier to follow which points are being used to take cross products and compute normals. I could probably make the code shorter and less repetitive with a well-constructed loop, but I worry it will be harder to follow and debug.

[alice-i-cecile]

I noticed I had some old pending comments stored up; sorry if anything is out of date!

[aevyrie]

FYI: There are a lot of changes I want to make to this PR, but I'm holding off until we have the RFC process up (next week?) so I can flesh out the details of what I'm thinking.

Edit: the RFC process PR: #1662

[ghost]

https://github.com/bevyengine/bevy/search?q=thiserror

I see lots of use of thiserror in other bevy crates FYI

[aevyrie]

Thanks for the heads up. For some reason I didn't think this was the case.

[aevyrie]

Yep, I'm happy to chime in @aevyrie .

Hey @bitshifter, the RFC in question is up and ready for review. I'd appreciate your feedback!
bevyengine/rfcs#12

[aevyrie]

Closing, too stale to be useful.