Enable construction of VertexAttributeValues from Vecs of vectors. #4548
Conversation
This feature is useful when working with procedural mesh generation code which generates its vertices in the form of `Vec<Vec3>`. The rest of the vector types are included for completeness.
alice-i-cecile
added
A-Rendering
| ($x:expr, $t:ty, $s:literal) => { | ||
| unsafe { | ||
| // SAFETY: x is only used in this file. It is called only with x being a value of type `Vec<Vec3>` or something similar. | ||
| // Vec3 is a repr(C) struct containing 3 f32s, hence it is transmutable to a [f32; 3]. |
There was a problem hiding this comment.
Where in the glam docs are we given this guarantee? Doing a search on the page for Vec3 and across the entire glam docs for Layout gives no result.
There was a problem hiding this comment.
In relevant part:
#[cfg_attr(target_arch = "spirv", repr(simd))]
#[cfg_attr(not(target_arch = "spirv"), repr(C))]
pub struct XYZ<T> {
pub x: T,
pub y: T,
pub z: T,
}
#[repr(transparent)]
pub struct Vec3(pub(crate) XYZF32);The second is repr(transparent) and the former is repr(C) so overall Vec3 is repr(C).
There was a problem hiding this comment.
Also note that while I don't have the reference right now, I found some docs stating that repr(simd) is transmutable to an array. Will get the reference tomorrow.
There was a problem hiding this comment.
That doesn't answer the question - I asked where glam gives us this guarantee. You have responded showing how the current implementation has this property
Those are not the same thing.
There was a problem hiding this comment.
A choice of repr is inherently part of the API for exactly this reason. By choosing repr(C) you are making the promise that your code can interop with C, so removing it is an API break. Another way to see this is to note that the repr appears in rustdoc.
There was a problem hiding this comment.
From the nomicon (https://doc.rust-lang.org/nomicon/other-reprs.html#reprtransparent):
This repr is only considered part of the public ABI of a type if either the single field is pub, or if its layout is documented in prose. Otherwise, the layout should not be relied upon by other crates.
-> Vec3 uses only pub(crate)
-> the layout should not be relied upon by other crates.
There was a problem hiding this comment.
Alright, thanks. If I got a PR on glam merged to add docs for this would that be sufficient to unblock this? I can't imagine them not wanting to have this guarantee.
| // SAFETY: x is only used in this file. It is called only with x being a value of type `Vec<Vec3>` or something similar. | ||
| // Vec3 is a repr(C) struct containing 3 f32s, hence it is transmutable to a [f32; 3]. | ||
| let mut v = std::mem::ManuallyDrop::new($x); | ||
| Vec::from_raw_parts(v.as_mut_ptr() as *mut [$t; $s], v.len(), v.capacity()) |
There was a problem hiding this comment.
Where does glam give us the guarantee that VecN has the same alignment as [f32; N]?
There was a problem hiding this comment.
Vec3 is a XYZ<f32> which is defined as a repr(C) struct with three f32 fields. repr(C) alignment is the same as the largest field alignment. Similarly, [T; N] has the same alignment as T, see https://doc.rust-lang.org/reference/type-layout.html.
| // SAFETY: x is only used in this file. It is called only with x being a value of type `Vec<Vec3>` or something similar. | ||
| // Vec3 is a repr(C) struct containing 3 f32s, hence it is transmutable to a [f32; 3]. | ||
| let mut v = std::mem::ManuallyDrop::new($x); | ||
| Vec::from_raw_parts(v.as_mut_ptr() as *mut [$t; $s], v.len(), v.capacity()) |
There was a problem hiding this comment.
Why does this not use https://docs.rs/bytemuck/latest/bytemuck/allocation/fn.cast_vec.html?
There was a problem hiding this comment.
Mainly because I wasn't aware of it at the time. I will have a look at it tomorrow.
| use bevy_utils::EnumVariantMeta; | ||
| use thiserror::Error; | ||
|
|
||
| macro_rules! convert_vec_of_vec { |
There was a problem hiding this comment.
Why is this a macro instead of a function?
There was a problem hiding this comment.
Because the base types in glam are XY, XYZ, and XYZW instead of arrays, hence I can't use const generics to genericize over the size of the type. I could use a function, but then I would need three of them.
Allow passing `Vec`s of glam vector types as vertex attributes. Alternative to #4548 and #2719 Also used some macros to cut down on all the repetition. # Migration Guide Implementations of `From<Vec<[u16; 4]>>` and `From<Vec<[u8; 4]>>` for `VertexAttributeValues` have been removed. I you're passing either `Vec<[u16; 4]>` or `Vec<[u8; 4]>` into `Mesh::insert_attribute` it will now require wrapping it with right the `VertexAttributeValues` enum variant. Co-authored-by: devil-ira <[email protected]>
|
This was superseded by #6442. |
Allow passing `Vec`s of glam vector types as vertex attributes. Alternative to bevyengine#4548 and bevyengine#2719 Also used some macros to cut down on all the repetition. # Migration Guide Implementations of `From<Vec<[u16; 4]>>` and `From<Vec<[u8; 4]>>` for `VertexAttributeValues` have been removed. I you're passing either `Vec<[u16; 4]>` or `Vec<[u8; 4]>` into `Mesh::insert_attribute` it will now require wrapping it with right the `VertexAttributeValues` enum variant. Co-authored-by: devil-ira <[email protected]>
Objective
I am writing procedural mesh generation which outputs its vertices in the form of a
Vec<Vec3>. It would be convenient to be able to feed this directly to a mesh. See discord for me proposing the idea.Solution
Implemented
From<Vec<Vec3>> for VertexAttributeValues. The rest of the vector types are included for convenience.Changelog
Enabled the construction of VertexAttributeValues from
Vec<(I|U|F)Vec(2|3|4)>through the use offromorinto.