Make math primitives runtime constructable, meshable #20250
Conversation
tychedelia
added
A-Rendering
tychedelia
added
C-Usability
|
It looks like your PR is a breaking change, but you didn't provide a migration guide. Please review the instructions for writing migration guides, then expand or revise the content in the migration guides directory to reflect your changes. |
|
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! 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. |
1 similar comment
|
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! 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. |
|
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! 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. |
| pub struct ConvexPolygon { | ||
| /// The vertices of the [`ConvexPolygon`]. | ||
| #[cfg_attr(feature = "serialize", serde(with = "super::serde::array"))] | ||
| vertices: [Vec2; N], | ||
| vertices: Vec<Vec2>, |
There was a problem hiding this comment.
This one I'm less sure about, since convex polygons tend to have a small number of vertices, and you can have a lot of them (ex: as a result of convex decomposition). I imagine that ConvexPolygon would primarily be used for collision detection or other geometry stuff where this can be relevant.
I wonder if the ideal representation would be to have a MAX_VERTICES const generic and to store an ArrayVec, that way you can store the vertices inline and have a cap based on how many vertices you expect to have 🤔 For example, for colliders I'd expect a max of 8 vertices per polygon. Box2D does basically the same (source).
Aside: Depending on what we intend ConvexPolygon to be used for, we might also want to store the outward edge normals here. It's required for collision detection, computing support faces, and some other stuff. For example Parry and Box2D store them. That's out of scope for this PR though.
| /// Create a new `Polyline2d` from two endpoints and a number of segments. | ||
| pub fn with_segment_count(start: Vec2, end: Vec2, segments: usize) -> Self { | ||
| if segments == 0 { | ||
| return Self::new(Vec::from([start, end])); | ||
| } | ||
|
|
||
| let step = (end - start) / segments as f32; | ||
| let mut vertices = Vec::with_capacity(segments + 1); | ||
| for i in 0..=segments { | ||
| vertices.push(start + step * i as f32); | ||
| } | ||
| Self { vertices } | ||
| } |
There was a problem hiding this comment.
Hmm, segments == 0 and segments == 1 produce the same result here, right? Both have just the start and end point. Not sure if that's expected.
I wonder if something like subdivisions or resolution would be clearer: 0 returns the polyline as-is, 1 divides the line segment(s) through the middle, and so on. It could even be generalized to not just take two endpoints, but to divide each line segment in a given polyline into parts and to return the resulting higher res polyline.
| impl BoxedPolyline2d { | ||
| /// Create a new `BoxedPolyline2d` from its vertices | ||
| impl Polyline2d { | ||
| /// Create a new `Polyline2d` from its vertices | ||
| pub fn new(vertices: impl IntoIterator<Item = Vec2>) -> Self { |
There was a problem hiding this comment.
Should these just take a Vec, considering from_iter already exists if you want to use an iterator-like?
There was a problem hiding this comment.
Personally I would "never" (say never) use Vec as an argument if I can have impl. Maybe remove from_iter instead?
If we change it, please add a comment to refer to from_iter.
There was a problem hiding this comment.
from_iter here comes from the FromIterator trait, which makes sense to have implemented IMO.
I'm not sure if Rust does some special optimization here, but mainly my motivation is that from_iter does iter.into_iter().collect(), which (I believe) allocates a new vector, unless Rust is smart enough to figure out when it can just use the original vec. For most of my use cases I would instead want to move the given vector directly without doing any unnecessary allocations.
But you can also just not use the constructor at all and do Polyline2d { vertices: todo!() }, so it's not a big problem regardless
alice-i-cecile
added
S-Waiting-on-Author
|
@tychedelia, I'm waiting on your response to feedback on this PR currently :) Once that's done, ping me for a review please. |
Objective
Many math primitives require const generics and thus are not constructable at runtime and also not meshable. While the use of const generics is theoretically more performant, it makes them very difficult to interact with in a generic way, particularly in relationship to mesh construction. For example, a ui that would allow selecting a primitive in order to create a mesh.
Solution
Make them alloc and meshable.