-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Define a basic set of Primitives #10466
Conversation
Cool! Part of the RFC was about bounding volumes. These are often used for culling. Every frame has at least one frustum. I wouldn’t say they’re rare. And they are a bounding volume. I’m thinking if debug gizmos are implemented for all of these, it would be good to also have frustum included. Aside from that if any of these are to be used for bounding volumes and would replace any existing bounding volumes or intersection methods used for culling or other rendering functionality, naturally ensuring no performance regressions is super important. :) |
Not really saying Frustums are rare by theirselves, but rather uncommon to be used as a primitive shape, tho I guess the same could be said about Ray2d/3d. I'd probably also define the primitive shape for a Frustum differently than how I would store it for bounding checks. Bounding volumes are probably the most logical next step. Culling, rendering (be it rasterized of raytracing with sdfs or meshes) and collisions could all use them. Some solid tests and benchmarks would definitely be important there. I think we'd at the very least want AABBs and a fairly complete set of intersection tests, aabb overlaps, frustum checks, ray casts and padded ray casts (used for shape casts). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like a good first version. A couple of things to discuss I think, but no fundamental objection. Thanks!
|
||
/// A normalized vector pointing in a direction in 2D space | ||
#[derive(Clone, Copy, Debug)] | ||
pub struct Direction2d(Vec2); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we're using Vec2
and fn
etc. in Rust, can we use Dir2d
here and save some typing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apart from Quad
(quadrilateral), it'd be the only abbreviated primitive, but it does resemble a struct like Vec2
more than an actual primitive shape... if it will be used a lot in public APIs, then perhaps Dir2d
would be good, but otherwise I think I prefer Direction2d
since it's generally better not to abbreviate names in my eyes. Other opinions on this would be useful though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's generally better not to abbreviate
Sure but have you been looking at Rust, didn't that ship float already? 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For very common types that appear everywhere like Vec
, Vec2
, Quat
and Res
, it makes sense to abbreviate, but for most other types I don't consider it good practise. But Dir2d
could be considered to be a utility type similar to Vec2
, so it could work here, not sure. (although we have the extra d
there... Dir2
would be more consistent with Glam math types, but not with Bevy's other types)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it's difficult to tell in advance. I don't mind either way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer to be consistent and explicit here. I don't think it's common enough to warrant shortening.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’d vote Direction2d for what it’s worth.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we use Normalized<T>(T)
, it could be used for T = Vec2
, Vec3
, Quat
, etc. Aliases could be defined as well.
I think it's okay to abbreviate often-used types, such as Vec2
/Vec3
(integers and floats only get a single letter), but using an expanded name for less common types avoids confusion and time spent searching documentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah we can go for Direction2d
since it's very explicit and consistent with other names. I kinda like the Normalized
suggestion too, but we only need it for Vec2
/Vec3
for now, so I think it's fine if we go for the non-generic approach until there's a real need for it. It would probably be aliased under Direction2d
/Direction3d
anyway and the API would be basically the same, so changing it later if necessary should be pretty straightforward.
Fixed (I hope) most of the comments, and added a few helpers but I'm not 100% sold on some of their names. And I think we could use some extra opinions on the comments about:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think my question about using Dir2d
was not addressed? A few other minor stuffs too.
Co-authored-by: Joona Aalto <[email protected]>
Alright, merging this in! We can iterate on this more (and add all of the helper methods) in follow-up PR. |
I'm a bit late to the party but also quiet interested in this. While there was already some discussion here and this is closed for a while I still have some questions to the authors and reviewers. Mainly I think that @jnhyatt made a good point and while some of the justifications made sense I still don't get some of the details. As I understand it, we implicitly assume that the line types pass through the origin to save some memory which plays nice with the ECS. My main questions are:
Maybe I just have tomatoes on my eyes though 🙈 |
In cases where positions are required, it should just be stored/passed separately. For the line intersection example, it could be something like this: fn line_intersection(line1: Line2d, line2: Line2d, pos1: Vec2, pos2: Vec2) {
// ...
} The Parry collision detection library does collision detection similarly, where the primitive shapes and isometries (or relative isometries) are given separately. Eventually, we'll probably have some kind of intersections and collision detection built in via traits, and I'd expect it work similarly by taking an isometry. Also note that if we wanted to, we still could store positions in the shape structs if we just made a wrapper, e.g. a collider could be something like this: Line2dCollider {
line: Line2d,
position: Vec2,
} But for a lot of things where the primitive shapes should be used (e.g. meshes and colliders) there wil probably be a I agree that it'd be more convenient to do some math things on the primitives if they stored positions, but I think e.g. computing line intersections is much more niche in comparison to the other use cases of primitives, so in my opinion it's not worth the added complexity and memory usage, especially when the positions can still be passed separately. |
Thanks for the clarification. Just curious: What are these general use cases you talk about? Also wouldn't it be more in line with the design philosophy to store |
Primitive shapes should be used for use cases like the ones listed here: #10572 Some very common ones include mesh creation (#10569) and colliders (not implemented yet). Both of these are in the context of entities, and those entities have Gizmo methods like Another reason to store position separately is that you might want to make multiple copies of the same underlying primitive, but at different positions. This could be for meshes, colliders, gizmos, basically anything. If the position was stored in the primitive itself, it could be more annoying to copy it at different positions, although I guess there could be some One exception is bounding volumes (#10946). They are inherently used in contexts where they have specific global extents, as they are used for several acceleration structures, so it makes sense to encode the position into the struct. However, they're not actually in the
Overall, the high-level goal is summarized quite well by the RFC:
The primitives are lightweight types that can be used everywhere in the Bevy ecosystem that uses shapes. Domain-specific properties can be handled via traits and/or wrappers, but the "low-level" types should not assume that e.g. storing positions in the primitives is needed, as it is not needed everywhere. The shapes need to strike a balance between memory efficiency (minimal size in bytes), computational efficiency (common operations should be cheap), and ergonomics (the API should be nice to use). The main focus is perhaps on the first two, but the third can be done quite well using helpers such as convenient constructors, getters, and wrappers. |
Ok, I'm fine and fully onboard with going the very minimalistic route of defining the structs. I just want to be consistent here: Why aren't we going for the maximum laziness in all situations? If we apply the same logic for storing the positions to storing the half lengths: Couldn't we just cache the length field separately outside of the Let's say we're just interested in one of you're mentioned main use cases: creating the meshes of a bunch of lines for rendering. Then if we keep the half length we would:
Maybe the current design is cool for your specific domain of use cases, but coming from a CAD background I currently don't see any benifits of using these types over using my own math lib. It's diverging too much from how we think about geometry. And I really wish it wasn't that way. So please could we just reconsider the things mentioned above? That way we can really base our CAD math ontop of the primitives like you envisioned it. |
If we stored the length separately, what would Each primitive is supposed to contain the minimum amount of data that fully represents the core shape (although interpretations of "fully" may vary). For line segments, the critical piece of information is that it describes a way to get from point The position differs from length in that it is not required to describe the core shape itself. The position is just a transformation that moves the shape relative to the origin. No matter how you transform a shape (outside non-uniform scaling), it will always be geometrically similar and therefore represent the shape accurately. This same logic does not work for lengths, as they are required to describe the actual shape. Caching length separately also means that line segments would need to be special-cased everywhere. Positions are handled the same way for all primitives, so you can use e.g. traits and share quite a lot of logic and APIs, but if the lengths of line segments (or other shapes) were stored separately, each system or method would need to handle them in a special way to get access to the length. Caching data that describes the core shapes fragments the API and logic, and frankly makes the idea of the primitives useless.
I'm not entirely sure what your specific needs are, just storing positions in the primitives? Either way, I don't think these primitives are supposed to be tailored to geometry and math specifically, as that is not what they are typically used for in games or in Bevy internally. They are general-purpose in the sense that they aim to support as many use cases as possible, but this does come with the trade-off that they might not be the perfect solution for some domains as requirements differ. I'm open to reconsidering the approach if there are concrete use cases that aren't reasonably possible with the current approach, but it also shouldn't be at the detriment of the "main use cases" mentioned earlier. I think creating an issue could be useful if you can nicely outline the current problems you have with the approach, and perhaps a proposal of how it should be done and what kind of use cases it would enable. Others are also free to chime in, I'm just sharing my personal thoughts and may have missed things :P |
Also, returning to storing positions in primitives for a moment, what would that actually look like? Consider the following example of creating a mesh: commands.spawn(PbrBundle {
mesh: meshes.add(
Cuboid::new(1.0, 1.0, 1.0).mesh().transform_by(Transform::from_xyz(1.0, 2.0, 0.0)).into(),
),
transform: Transform::from_xyz(5.0, 0.0, 0.0),
}); What is the position of the mesh here? We could specify the primitive transform as a local transform relative to the entity's own Alternatively, we could treat it as a global transform, but that has it's own set of very bad problems. It would require having systems that constantly update the positions of the primitives, which would be expensive and prone to scheduling issues. It would be very implicit, as users expect the normal I feel like having two models for positions, |
I was talking about storing it separately when using the approach of just using the "end point representation". Sorry for the wording and for making it easy to misinterpret.
Just the endpoint is also a full representation of the line segement given that we implicitly take the origin as the start point. Let's focus a bit. My argument was:
Yeah that's kind of fair and would incur some extra logic although I doubt it would be special cased everywhere. Imo: through careful design we could probably hide away the caching mechanism, making the systems calculate the length JIT or something. But that's just an idea and would probably mean other challanges in case of a real implementation
We're doing a lot of projections and intersections with lines and I fear that the current design just creates friction if we always have to transform forth to the actual position of the primitive and back to the origin representation of the primitive.
I didn't say that or at least I didn't mean to say it if it sounded like I was
Honestly I'm at a point where this discussion is just too exhausting and I feel like this is bike shedding. We can leave it as it is and I'll just trust you. |
Ah, gotcha. I didn't realize it was in the context of the end point approach, makes sense now!
This is true, but it has the issue of more expensive computations (which you do address right after). That being said, I think we might be overengineering this a bit and doing premature optimization. A few extra bytes or computations should rarely matter in practise, so unless it shows up in profiling, it's probably a non-issue.
That's fair. If the issue is specifically in the line segment representation, and it'd be solved by simply using endpoints The main issues would be memory usage and computational efficiency as mentioned earlier, but in practise it probably wouldn't be a big deal. I think those are secondary if the approach enables more advanced use cases and better ergonomics, and by your description, it seems like it does. However, the thing I don't want is for all primitives to have an actual position or transform stored in them (outside of "implicit position" defined by e.g. the endpoints) due to the reasons I've mentioned earlier and also for the reasons the RFC mentions.
Sorry, my bad. I read this: "I currently don't see any benifits of using these types over using my own math lib. It's diverging too much from how we think about geometry" and interpreted it as you wanting things to be generally more math/geometry focused, but I believe you meant that specifically it doesn't quite fit your requirements for CAD.
Yeah, agreed that this might be bike shedding. I feel like I've been a bit too critical/defensive here, so my apologies if it came across like that. Like you, I just want the primitives to be as good as possible, which is why I've been defending my viewpoints quite thoroughly. But as I mentioned, I could get behind the line segment representation of two endpoints in case that'd be enough for your use case. Or we can just leave it be, or return to it later. |
I think we both have some points. For now I'm fine with leaving it as it is. Let's see where things are going. We can always discuss things when they're popping up 👍 Sorry for being pesky with all of this fuss. I just wanted to create some awareness for the CAD people as there are already some companies using bevy for real world use cases. Thank you for being accommodating to me after all. Let's work together on this to create the best for everyone 🤝 |
# Objective - Implement a subset of https://github.com/bevyengine/rfcs/blob/main/rfcs/12-primitive-shapes.md#feature-name-primitive-shapes ## Solution - Define a very basic set of primitives in bevy_math - Assume a 0,0,0 origin for most shapes - Use radius and half extents to avoid unnecessary computational overhead wherever they get used - Provide both Boxed and const generics variants for shapes with variable sizes - Boxed is useful if a 3rd party crate wants to use something like enum-dispatch for all supported primitives - Const generics is useful when just working on a single primitive, as it causes no allocs #### Some discrepancies from the RFC: - Box was changed to Cuboid, because Box is already used for an alloc type - Skipped Cone because it's unclear where the origin should be for different uses - Skipped Wedge because it's too niche for an initial PR (we also don't implement Torus, Pyramid or a Death Star (there's an SDF for that!)) - Skipped Frustum because while it would be a useful math type, it's not really a common primitive - Skipped Triangle3d and Quad3d because those are just rotated 2D shapes ## Future steps - Add more primitives - Add helper methods to make primitives easier to construct (especially when half extents are involved) - Add methods to calculate AABBs for primitives (useful for physics, BVH construction, for the mesh AABBs, etc) - Add wrappers for common and cheap operations, like extruding 2D shapes and translating them - Use the primitives to generate meshes - Provide signed distance functions and gradients for primitives (maybe) --- ## Changelog - Added a collection of primitives to the bevy_math crate --------- Co-authored-by: Joona Aalto <[email protected]>
# Objective `Direction2d::from_normalized` & `Direction3d::from_normalized` don't emphasize that importance of the vector being normalized enough. ## Solution Rename `from_normalized` to `new_unchecked` and add more documentation. --- `Direction2d` and `Direction3d` were added somewhat recently in #10466 (after 0.12), so I don't think documenting the changelog and migration guide is necessary (Since there is no major previous version to migrate from). But here it is anyway in case it's needed: ## Changelog - Renamed `Direction2d::from_normalized` and `Direction3d::from_normalized` to `new_unchecked`. ## Migration Guide - Renamed `Direction2d::from_normalized` and `Direction3d::from_normalized` to `new_unchecked`. --------- Co-authored-by: Tristan Guichaoua <[email protected]> Co-authored-by: Joona Aalto <[email protected]>
Objective
Solution
Some discrepancies from the RFC:
Future steps
Changelog