Skip to content

Enable construction of VertexAttributeValues from Vecs of vectors. #4548

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

Enable construction of VertexAttributeValues from Vecs of vectors. #4548

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
115 changes: 115 additions & 0 deletions crates/bevy_render/src/mesh/mesh/conversions.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,21 @@
//! ```

use crate::mesh::VertexAttributeValues;
use bevy_math::{IVec2, IVec3, IVec4, UVec2, UVec3, UVec4, Vec2, Vec3, Vec4};
use bevy_utils::EnumVariantMeta;
use thiserror::Error;

macro_rules! convert_vec_of_vec {
Copy link
Member

@DJMcNab DJMcNab Apr 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this a macro instead of a function?

Copy link
Author

@obsgolem obsgolem Apr 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

($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].
Copy link
Member

@DJMcNab DJMcNab Apr 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

@obsgolem obsgolem Apr 21, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Author

@obsgolem obsgolem Apr 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@DJMcNab DJMcNab Apr 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

@obsgolem obsgolem Apr 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@MinerSebas MinerSebas Apr 21, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

@obsgolem obsgolem Apr 21, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

let mut v = std::mem::ManuallyDrop::new($x);
Vec::from_raw_parts(v.as_mut_ptr() as *mut [$t; $s], v.len(), v.capacity())
Copy link
Member

@DJMcNab DJMcNab Apr 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where does glam give us the guarantee that VecN has the same alignment as [f32; N]?

Copy link
Author

@obsgolem obsgolem Apr 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@DJMcNab DJMcNab Apr 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does this not use https://docs.rs/bytemuck/latest/bytemuck/allocation/fn.cast_vec.html?

Copy link
Author

@obsgolem obsgolem Apr 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mainly because I wasn't aware of it at the time. I will have a look at it tomorrow.

}
};
}

#[derive(Debug, Clone, Error)]
#[error("cannot convert VertexAttributeValues::{variant} to {into}")]
pub struct FromVertexAttributeError {
Expand Down Expand Up @@ -130,6 +142,60 @@ impl From<Vec<[u8; 4]>> for VertexAttributeValues {
}
}

impl From<Vec<Vec2>> for VertexAttributeValues {
fn from(vec: Vec<Vec2>) -> Self {
VertexAttributeValues::Float32x2(convert_vec_of_vec!(vec, f32, 2))
}
}

impl From<Vec<IVec2>> for VertexAttributeValues {
fn from(vec: Vec<IVec2>) -> Self {
VertexAttributeValues::Sint32x2(convert_vec_of_vec!(vec, i32, 2))
}
}

impl From<Vec<UVec2>> for VertexAttributeValues {
fn from(vec: Vec<UVec2>) -> Self {
VertexAttributeValues::Uint32x2(convert_vec_of_vec!(vec, u32, 2))
}
}

impl From<Vec<Vec3>> for VertexAttributeValues {
fn from(vec: Vec<Vec3>) -> Self {
VertexAttributeValues::Float32x3(convert_vec_of_vec!(vec, f32, 3))
}
}

impl From<Vec<IVec3>> for VertexAttributeValues {
fn from(vec: Vec<IVec3>) -> Self {
VertexAttributeValues::Sint32x3(convert_vec_of_vec!(vec, i32, 3))
}
}

impl From<Vec<UVec3>> for VertexAttributeValues {
fn from(vec: Vec<UVec3>) -> Self {
VertexAttributeValues::Uint32x3(convert_vec_of_vec!(vec, u32, 3))
}
}

impl From<Vec<Vec4>> for VertexAttributeValues {
fn from(vec: Vec<Vec4>) -> Self {
VertexAttributeValues::Float32x4(convert_vec_of_vec!(vec, f32, 4))
}
}

impl From<Vec<IVec4>> for VertexAttributeValues {
fn from(vec: Vec<IVec4>) -> Self {
VertexAttributeValues::Sint32x4(convert_vec_of_vec!(vec, i32, 4))
}
}

impl From<Vec<UVec4>> for VertexAttributeValues {
fn from(vec: Vec<UVec4>) -> Self {
VertexAttributeValues::Uint32x4(convert_vec_of_vec!(vec, u32, 4))
}
}

impl TryFrom<VertexAttributeValues> for Vec<[u8; 4]> {
type Error = FromVertexAttributeError;

Expand Down Expand Up @@ -360,6 +426,10 @@ impl TryFrom<VertexAttributeValues> for Vec<f32> {

#[cfg(test)]
mod tests {
use std::f32::consts::PI;

use bevy_math::{IVec2, IVec3, IVec4, UVec2, UVec3, UVec4, Vec2, Vec3, Vec4};

use super::VertexAttributeValues;
#[test]
fn f32() {
Expand Down Expand Up @@ -407,6 +477,11 @@ mod tests {
assert_eq!(buffer, result_into);
assert_eq!(buffer, result_from);
assert!(error.is_err());

let buffer = vec![Vec2::new(0., PI); 2];
let values = VertexAttributeValues::from(buffer);
let result_into: Vec<[f32; 2]> = values.try_into().unwrap();
assert_eq!(result_into[1][1], PI);
}

#[test]
Expand All @@ -419,6 +494,11 @@ mod tests {
assert_eq!(buffer, result_into);
assert_eq!(buffer, result_from);
assert!(error.is_err());

let buffer = vec![IVec2::new(0, -1); 2];
let values = VertexAttributeValues::from(buffer);
let result_into: Vec<[i32; 2]> = values.try_into().unwrap();
assert_eq!(result_into[1][1], -1);
}

#[test]
Expand All @@ -431,6 +511,11 @@ mod tests {
assert_eq!(buffer, result_into);
assert_eq!(buffer, result_from);
assert!(error.is_err());

let buffer = vec![UVec2::new(0, u32::MAX); 2];
let values = VertexAttributeValues::from(buffer);
let result_into: Vec<[u32; 2]> = values.try_into().unwrap();
assert_eq!(result_into[1][1], u32::MAX);
}

#[test]
Expand All @@ -443,6 +528,11 @@ mod tests {
assert_eq!(buffer, result_into);
assert_eq!(buffer, result_from);
assert!(error.is_err());

let buffer = vec![Vec3::new(0., 0., PI); 2];
let values = VertexAttributeValues::from(buffer);
let result_into: Vec<[f32; 3]> = values.try_into().unwrap();
assert_eq!(result_into[1][2], PI);
}

#[test]
Expand All @@ -455,6 +545,11 @@ mod tests {
assert_eq!(buffer, result_into);
assert_eq!(buffer, result_from);
assert!(error.is_err());

let buffer = vec![IVec3::new(0, 0, -1); 2];
let values = VertexAttributeValues::from(buffer);
let result_into: Vec<[i32; 3]> = values.try_into().unwrap();
assert_eq!(result_into[1][2], -1);
}

#[test]
Expand All @@ -467,6 +562,11 @@ mod tests {
assert_eq!(buffer, result_into);
assert_eq!(buffer, result_from);
assert!(error.is_err());

let buffer = vec![UVec3::new(0, 0, u32::MAX); 2];
let values = VertexAttributeValues::from(buffer);
let result_into: Vec<[u32; 3]> = values.try_into().unwrap();
assert_eq!(result_into[1][2], u32::MAX);
}

#[test]
Expand All @@ -479,6 +579,11 @@ mod tests {
assert_eq!(buffer, result_into);
assert_eq!(buffer, result_from);
assert!(error.is_err());

let buffer = vec![Vec4::new(0., 0., 0., PI); 2];
let values = VertexAttributeValues::from(buffer);
let result_into: Vec<[f32; 4]> = values.try_into().unwrap();
assert_eq!(result_into[1][3], PI);
}

#[test]
Expand All @@ -491,6 +596,11 @@ mod tests {
assert_eq!(buffer, result_into);
assert_eq!(buffer, result_from);
assert!(error.is_err());

let buffer = vec![IVec4::new(0, 0, 0, -1); 2];
let values = VertexAttributeValues::from(buffer);
let result_into: Vec<[i32; 4]> = values.try_into().unwrap();
assert_eq!(result_into[1][3], -1);
}

#[test]
Expand All @@ -503,6 +613,11 @@ mod tests {
assert_eq!(buffer, result_into);
assert_eq!(buffer, result_from);
assert!(error.is_err());

let buffer = vec![UVec4::new(0, 0, 0, u32::MAX); 2];
let values = VertexAttributeValues::from(buffer);
let result_into: Vec<[u32; 4]> = values.try_into().unwrap();
assert_eq!(result_into[1][3], u32::MAX);
}

#[test]
Expand Down