[Merged by Bors] - Allow passing `glam` vector types as vertex attributes #6442

tim-blackbird · 2022-11-01T22:50:46Z

Allow passing Vecs 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.

mockersf · 2022-11-01T22:52:04Z

alternative to #2719 ?

tim-blackbird · 2022-11-01T23:06:03Z

CI is failing because I accidentally removed the following impls:

impl From<Vec<[u16; 4]>> for VertexAttributeValues {
    fn from(vec: Vec<[u16; 4]>) -> Self {
        VertexAttributeValues::Uint16x4(vec)
    }
}

impl From<Vec<[u8; 4]>> for VertexAttributeValues {
    fn from(vec: Vec<[u8; 4]>) -> Self {
        VertexAttributeValues::Unorm8x4(vec)
    }
}

but I'm not sure whether it makes sense to have these as [u16; 4] can be either Uint16x4 or Unorm16x4,
and [u8; 4] can be either Uint8x4 or Unorm8x4.

mockersf · 2022-11-01T23:15:42Z

Failure is in example code. Can the example be improved to not use this From impl / be more explicit?

alice-i-cecile · 2022-11-02T02:15:03Z

crates/bevy_render/src/mesh/mesh/conversions.rs

-                Ok(value)
+macro_rules! impl_from {
+    ($from:ty, $variant:tt) => {
+        impl From<Vec<$from>> for VertexAttributeValues {


Should we be inlining these trait impls? Do we have benchmarks or stress tests that might be able to spot a difference there?

I couldn't find any that update vertex attributes.

Perhaps we should add such a test. I think having stress tests that modify some of the data bindings (index / vertex buffers, textures, uniforms, storage buffers) every frame would be good for catching regressions. Currently we have multiple that update lots of uniforms with dynamic offset bindings (mesh entity transforms) and storage buffers (many_lights, on a system with storage buffer support which is basically everything except WebGL2). I suppose the compute shader game of life updates textures every frame, but it does it from a compute shader so there is no per-frame texture memory transfer from system to video RAM, but that would be good to test. And I don't think we have anything that updates index/vertex buffers.

alice-i-cecile

Much nicer. This is a good, internal only use of macros to reduce error-prone boilerplate.

alice-i-cecile · 2022-11-03T12:44:05Z

bors r+

bors · 2022-11-03T12:52:25Z

Merge conflict.

superdump

I think this should not be merged before the Vec3A question is resolved. Beyond that, it would be good to add tests.

It would also be good to add a mesh vertex attribute mutation stress test but that can be done in a separate PR.

superdump · 2022-11-03T12:48:03Z

crates/bevy_render/src/mesh/mesh/conversions.rs

-                Ok(value)
+macro_rules! impl_from {
+    ($from:ty, $variant:tt) => {
+        impl From<Vec<$from>> for VertexAttributeValues {


Perhaps we should add such a test. I think having stress tests that modify some of the data bindings (index / vertex buffers, textures, uniforms, storage buffers) every frame would be good for catching regressions. Currently we have multiple that update lots of uniforms with dynamic offset bindings (mesh entity transforms) and storage buffers (many_lights, on a system with storage buffer support which is basically everything except WebGL2). I suppose the compute shader game of life updates textures every frame, but it does it from a compute shader so there is no per-frame texture memory transfer from system to video RAM, but that would be good to test. And I don't think we have anything that updates index/vertex buffers.

crates/bevy_render/src/mesh/mesh/conversions.rs

superdump · 2022-11-03T13:04:11Z

bors r-

superdump · 2022-11-03T14:46:51Z

Ok, I'm fine with the Vec3A issue now. But could you add tests for the various glam types, including ones that verify the resulting '.into()'ed vertex attribute values please?

superdump · 2022-11-03T15:30:18Z

crates/bevy_render/src/mesh/mesh/conversions.rs

+
+    #[test]
+    fn vec3a() {
+        let buffer = vec![Vec3A::ZERO; 10];


I can appreciate if my push back is frustrating. My goal is to achieve really useful tests. The problem we were discussing about Vec3A, where my suspicion turned out to be wrong, would not show itself by using Vec3A::ZERO as all values are zero including the unused fourth f32 worth of bytes. Could we maybe used Vec3A::new(1.0, 2.0, 3.0). I wonder if there is any guarantee what that those f32-worth of bytes would be when using alignment. I don't know. My guess would be initialised to bitwise all 0.

That's fair. This did cross my mind c:

I also note that all the others use 0s, but also they only really need to test the type conversions I guess. Vec3A is a special case, imo.

Interesting, the fourth value is the same as the third.

impl Vec3A { pub const fn new(x: f32, y: f32, z: f32) -> Self { unsafe { UnionCast { a: [x, y, z, z] }.v } } }

My first thought is to maybe do the same for the third component as the fourth, in an attempt to avoid illegitimate floating point exceptions or something? I have no idea if that is the reason though. :)

Looks like we don't have to worry about this because it's not actually possible to use bytemuck on a Vec3A by accident!

That's good enough for me! :)

superdump · 2022-11-04T03:45:01Z

bors r+

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 <justthecooldude@gmail.com>

bors · 2022-11-04T03:59:26Z

Pull request successfully merged into main.

Build succeeded:

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 <justthecooldude@gmail.com>

mockersf added A-Rendering Drawing game state to the screen C-Usability A targeted quality-of-life change that makes Bevy easier to use labels Nov 1, 2022

alice-i-cecile added the M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Nov 2, 2022

alice-i-cecile reviewed Nov 2, 2022

View reviewed changes

alice-i-cecile approved these changes Nov 2, 2022

View reviewed changes

tim-blackbird changed the title ~~Allow passing Vecs of glam vector types as vertex attributes~~ Allow passing glam vector types as vertex attributes Nov 2, 2022

james7132 approved these changes Nov 3, 2022

View reviewed changes

james7132 added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Nov 3, 2022

Macro!

a462d69

superdump requested changes Nov 3, 2022

View reviewed changes

tim-blackbird added 5 commits November 3, 2022 14:06

use

b0b807c

Vec3A

6c8ea2f

Fix example

9e56585

fmt

b654df6

spelling

d2a54aa

tim-blackbird force-pushed the glam-into-vertex-attribute branch from 6b30996 to d2a54aa Compare November 3, 2022 13:06

tests

6707d28

alice-i-cecile approved these changes Nov 3, 2022

View reviewed changes

superdump reviewed Nov 3, 2022

View reviewed changes

superdump approved these changes Nov 4, 2022

View reviewed changes

bors bot changed the title ~~Allow passing glam vector types as vertex attributes~~ [Merged by Bors] - Allow passing glam vector types as vertex attributes Nov 4, 2022

bors bot closed this Nov 4, 2022

This was referenced Nov 4, 2022

Add the ability to pass Vec<VecX> directly to mesh.set_attribute + simple example #2719

Closed

Enable construction of VertexAttributeValues from Vecs of vectors. #4548

Closed

tim-blackbird deleted the glam-into-vertex-attribute branch November 8, 2022 21:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Merged by Bors] - Allow passing `glam` vector types as vertex attributes #6442

[Merged by Bors] - Allow passing `glam` vector types as vertex attributes #6442

tim-blackbird commented Nov 1, 2022 •

edited

Loading

mockersf commented Nov 1, 2022

tim-blackbird commented Nov 1, 2022

mockersf commented Nov 1, 2022

alice-i-cecile Nov 2, 2022

tim-blackbird Nov 2, 2022

superdump Nov 3, 2022

alice-i-cecile left a comment

alice-i-cecile commented Nov 3, 2022

bors bot commented Nov 3, 2022

superdump left a comment

superdump Nov 3, 2022

superdump commented Nov 3, 2022

superdump commented Nov 3, 2022

superdump Nov 3, 2022 •

edited

Loading

tim-blackbird Nov 3, 2022

superdump Nov 3, 2022

tim-blackbird Nov 3, 2022

superdump Nov 3, 2022

tim-blackbird Nov 3, 2022

superdump Nov 4, 2022

superdump commented Nov 4, 2022

bors bot commented Nov 4, 2022

[Merged by Bors] - Allow passing glam vector types as vertex attributes #6442

[Merged by Bors] - Allow passing glam vector types as vertex attributes #6442

Conversation

tim-blackbird commented Nov 1, 2022 • edited Loading

Migration Guide

mockersf commented Nov 1, 2022

tim-blackbird commented Nov 1, 2022

mockersf commented Nov 1, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alice-i-cecile left a comment

Choose a reason for hiding this comment

alice-i-cecile commented Nov 3, 2022

bors bot commented Nov 3, 2022

superdump left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

superdump commented Nov 3, 2022

superdump commented Nov 3, 2022

superdump Nov 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

superdump commented Nov 4, 2022

bors bot commented Nov 4, 2022

[Merged by Bors] - Allow passing `glam` vector types as vertex attributes #6442

[Merged by Bors] - Allow passing `glam` vector types as vertex attributes #6442

tim-blackbird commented Nov 1, 2022 •

edited

Loading

superdump Nov 3, 2022 •

edited

Loading