lpghatguy / crevice Goto Github PK

I'm looking for a library for handling shader layouts in rust. From the example shown in README, The derive(AsStd140) seems to generate a new std140 layout struct and provide a method to convert between the origin struct. I wonder if there is possible to directly insert the padding into the origin struct, to avoid possible memory copy or conversion runtime cost(I don't sure even if the conversion is trivial, the compiler is able to eliminate the cost) and code bloat.

The other question is if it's possible to directly allow users custom math library if the library implements an unsafe trait that constraint and maps the math primitive to a shader primitive struct. This will also remove the math lib conversion costs.

Finally thanks for creating such a library, the way to generate the padding function using the const function is pretty inspiring.

I think the following layout

#[derive(AsStd140, AsStd430)]
struct MainUniform {
    e: mint::ColumnMatrix3<f32>,
    f: f32,
}

needs to produce the same result in std140 and std430

currently:

Std140MainUniform (size 52, alignment 4)
| Offset | Name     | Size |
| ------ | -------- | ---- |
| 0      | _e_align | 0    |
| 0      | e        | 48   |
| 48     | _f_align | 0    |
| 48     | f        | 4    |

[src/main.rs:42] std::mem::size_of::<Std140MainUniform>() = 52

Std430MainUniform (size 40, alignment 4)
| Offset | Name     | Size |
| ------ | -------- | ---- |
| 0      | _e_align | 0    |
| 0      | e        | 36   |
| 36     | _f_align | 0    |
| 36     | f        | 4    |

[src/main.rs:50] std::mem::size_of::<Std430MainUniform>() = 40

I think this is the case because e is an array of arrays where each array has an alignment of 16 (rule 3 https://www.khronos.org/registry/OpenGL/specs/gl/glspec45.core.pdf#page=159)

Be ready to split some hairs :).

Take the following struct

struct Bleh {
    mat4 a;
    vec3 b;
}

Crevice interprets this struct as being of size 76, via calling std140_size_static.

This goes against what both the wgsl and GL spec specify, asking for a size of 80 (roundUp(16, 64 + 12)). It also interprets this struct as size 80 instead of the proper size 96.

struct Other {
    Bleh a;
    f32 b;
}

wgsl:

The size of a structure is equal to the offset plus the size of its last member, rounded to the next multiple of the structure’s alignment:

SizeOf(S) = roundUp(AlignOf(S), OffsetOf(S, L) + SizeOf(S, L))
Where L is the last member of the structure 

GL:

The structure may have padding at the end;the base offset of the member following the sub-structure is rounded up to the next multiple of the base alignment of the structure.

This GL spec rules only applies to the context of it being a member. Wgsl specifies this for both being a member and a top level struct. The GL spec does not say the size of the struct when it is a top level struct. Because of this omission, it should be valid to always interpret the size of this struct as 80.

This should make everyone agree, then we can live a happy life away from all this hair splitting and spec reading.

Having a generic, transparent wrapper that aligns to 256 bytes is easy and also very useful for many graphics APIs.

I have an implemention in my game project that I'll port over tomorrow.

When I tried to use your crate with cgmath, rustc somehow complained that there is no implementation of From<cgmath::Matrix4<f32>> for mint::ColumnMatrix4. It took me a while to figure out I should have enabled the feature "mint" of cgmath. I found out that issue by looking at your Cargo.toml dependencies :( because somehow this "mint" feature is not mentioned in the docs of cgmath either, which is weird.

It will be great if you can update your doc a bit to mention this "mint" feature in your examples. And I have also made an another issue to cgmath to address this problem. Thanks!

Could you please provide instructions on how to get those definitions working? I see impl macro invocations hiding behind the "glam" flag but I fail to get them in scope nevertheless.

the following code

#[derive(AsStd140)]
struct MainUniform {
    b: Anon,
    c: f32,
}

#[derive(AsStd140)]
struct Anon {
    u: f32,
    v: f32,
}

yields:

| Offset | Name     | Size |
| ------ | -------- | ---- |
| 0      | _b_align | 0    |
| 0      | b        | 8    |
| 8      | _c_align | 0    |
| 8      | c        | 4    |

[src/main.rs:42] std::mem::size_of::<Std140MainUniform>() = 12

I think this is incorrect as struct have 16byte alignment and thus 8byte padding would need to be inserted as _c_align.

https://www.khronos.org/registry/OpenGL/specs/gl/glspec45.core.pdf#page=159

If the member is a structure, the base alignment of the structure is N, where
N is the largest base alignment value of any of its members, and rounded
up to the base alignment of a vec4. The individual members of this substructure are then assigned offsets by applying this set of rules recursively,
where the base offset of the first member of the sub-structure is equal to the
aligned offset of the structure. The structure may have padding at the end;
the base offset of the member following the sub-structure is rounded up to
the next multiple of the base alignment of the structure.

I often develop with #![warn(missing_docs)], and the generated code from crevice's proc macros run afowl of this. Adding #[allow(warnings)] to the generated code should fix this for all custom lints (of course, when developing crevice itself and maybe by default this should not be included, so placing it behind a feature flag seems sensible to me).

A while back, glam 0.23.0 was released. While it didn't bring any major changes, it seems that it's still treated as separate by the compiler, and any code using glam 0.23 will fail to compile when used with crevice.

It could be interesting to use e.g. cargo asm https://github.com/gnzlbg/cargo-asm to see how much work is roughly involved when using these conversions/ how the compiler can optimise them.

I used it here as an example https://github.com/benmkw/comptime_trie (which may be a bit hacky but :D)

This appears to have been fixed for WriteStd140, but WriteStd430 seems to have been missed.

In order to support DSTs like slices, I think it would be beneficial to introduce a new trait, WriteStd140. It describes a type that can be written to a buffer using std140 layout rules.

This trait:

• Would be blanket impl'd for all types that implement AsStd140
• Would be impl'd for slices and maybe std types that have allocated memory like Vec

Because this trait is more general, lots of functions can be changed from bounding on AsStd140 to bounding on WriteStd140 to allow more types. In particular, std140::Writer's methods can change a bit.

This all results in a nice hierarchy of traits:

• Std140 is for types that are already std140-compatible. All Std140 types implement AsStd140, which is the same as Clone.
• AsStd140 is for types that can turn into Std140 types directly. Their layout is statically known. All AsStd140 types implement WriteStd140.
• WriteStd140 is for any type, including those with dynamic layout or size.

Without this hiding, these structs show up in generated documentation for crates using crevice, which, based on my reading of the API, is not intended since these structs are not used directly.

I have the following code :

#[derive(AsStd430, Clone)]
pub struct Globals {
    pub resolution: mint::Vector2<u32>,
    pub mouse: mint::Vector2<u32>,
    pub mouse_wheel: f32,
    pub ratio: f32,
    pub time: f32,
    pub frame: u32,
}

Which fails to build if I include crevice like that :

[dependencies.crevice]
version = "0.7"

with the error :

error[E0046]: not all trait items implemented, missing: `from_std430`
  --> src\lib.rs:55:10
   |
55 | #[derive(AsStd430, Clone)]
   |          ^^^^^^^^ missing `from_std430` in implementation
   |
   = note: this error originates in a derive macro (in Nightly builds, run with -Z macro-backtrace for more info)
   = help: implement the missing item: `fn from_std430(_: <Self as AsStd430>::Std430Type) -> Self { todo!() }`

But it works correctly if I specify the latest commit from this repo (78165c1) :

[dependencies.crevice]
version = "0.7"
git = "https://github.com/LPGhatguy/crevice"
rev = "78165c1bdb22c699b2523cdfa4bd13dd60ced79f"

Running cargo-expand shows that from_std430 is effectively missing in the former.

impl ::crevice::std430::AsStd430 for Globals {
    type Std430Type = Std430Globals;
    fn as_std430(&self) -> Self::Std430Type {
        Self::Std430Type {
            resolution: self.resolution.as_std430(),
            mouse: self.mouse.as_std430(),
            mouse_wheel: self.mouse_wheel.as_std430(),
            ratio: self.ratio.as_std430(),
            time: self.time.as_std430(),
            frame: self.frame.as_std430(),
            ..::crevice::internal::bytemuck::Zeroable::zeroed()
        }
    }
    // With the crate built from the repo we get this impl as well
    fn from_std430(value: Self::Std430Type) -> Self {
        Self {
            resolution: <mint::Vector2<u32> as ::crevice::std430::AsStd430>::from_std430(
                value.resolution,
            ),
            mouse: <mint::Vector2<u32> as ::crevice::std430::AsStd430>::from_std430(value.mouse),
            mouse_wheel: <f32 as ::crevice::std430::AsStd430>::from_std430(value.mouse_wheel),
            ratio: <f32 as ::crevice::std430::AsStd430>::from_std430(value.ratio),
            time: <f32 as ::crevice::std430::AsStd430>::from_std430(value.time),
            frame: <u32 as ::crevice::std430::AsStd430>::from_std430(value.frame),
        }
    }
}

This is really weird assuming the release on crates.io is built from this specific commit. This makes me think the release on crates.io is not built from the code we see on this repo.

Please note that I tried cleaning the build cache and the cargo crate cache.

These can probably follow the GLSL naming conventions: DVec2, DVec3, DVec4.

Hi! I noticed no-std support has been merged for a while but it's not on crates.io :( Could we do another release to get that in? :)

Since const generics MVP hit the stable, it should be possible to implement AsStd* for sized arrays.

The corresponding Std* type would need to be a sized array of a generated struct type with a field for underlying element's Std* representation, and a field for stride-correcting padding.

I believe the implementation should be rather straightforward, I'll gladly take on it if it is deemed worthwhile (well, I'd certainly like to use it personally).

Currently, the traits are disconnected, which means the only useful way to implement them is a derive macro.
I propose a following minor breaking interface change.

pub unsafe trait Layout<const MIN_ALIGNMENT: usize> {...} // Std140 stuff
pub trait AsLayout<const MIN_ALIGNMENT: usize> {...} // AsStd140 stuff

// Alias trait
pub unsafe trait Std140: Layout<16> {}
unsafe impl<T> Std140 for T where T: Layout<16> {}

// Preserve interface
pub trait AsStd140 {...}
impl<T> AsLayout<16> for T where T: AsStd140 { ...reexport stuff }

This would unfortunately break direct users of Std140 trait (they'll need to implement Layout<16> instead), but since the library is not v1.0 yet and this is not the intended usecase, that's probably fine.

This would significantly simplify the derive macro (we no longer need to track which of the unsafe traits we need to implement),
and also allows us to unify almost all of the primitive types (except matrices, which aren't truly "primitive" from GLSL's point of view).

The rules for implementing bytemuck::Pod manually say that the type must be repr(C) or repr(transparent).

Currently, crevice implements Pod on each of the gVecN and gMatN structs, which are implicitly repr(Rust).

Maybe crevice should consider using the Pod derive macro? It adds additional compile-time checks, such as ensuring that the size of the struct is equal to the sum of the sizes of the struct's members:

#[repr(C)]
struct Example {
    foo: f32,
    bar: u32,
}
const _: fn() = || {
    struct TypeWithoutPadding([u8; ::core::mem::size_of::<f32>() + ::core::mem::size_of::<u32>()]);
    let _ = ::core::mem::transmute::<Example, TypeWithoutPadding>;
};

It might be desirable to use Crevice in rust-gpu. I believe that would require Crevice to support no_std, which should be straightforward!

When porting from custom type layout code over to Crevice, I tried to use Sizer to calculate buffer offsets for a dynamic uniform. This did not work: it's a fencepost error!

A new type or maybe a new method on Sizer could help compute buffer offsets instead of buffer sizes.

It should be straightforward to extend the crate with support for std430 layout as well.

Sometimes it's useful to read data from the GPU. I think a lot of the prerequisite foundations are in place for this.

GLSL's bools are 32 bits. Rust's are 8 bits. Making a vec of 8 bit bools doesn't make them line up.

Additionally, I don't think bool is fully inhabited, so making it Pod/Zeroable isn't quite right.

I'm currently using glam 0.20 in my project. The version of this crate published to crates.io uses glam 0.19. As the glam version was updated in f1c6a6c, it'd be great if you could publish a new version sometime :)

I don’t have the answer at hand but one could check if the sizes of the members (recursively for structs) are in descending or ascending order or else emit an error.

If I remember correctly if padding is not reused then ascending and descending order are both optimal.

This would make sure one does not waste space.
Maybe there are use cases where one wants that so it probably needs to be optional.

Currently, users need to use mint types in their derives:

#[derive(AsStd140)]
struct Foo {
    value: mint::ColumnMatrix3<f32>, // eek! so verbose...
}

We should make it so types from math libraries can be used directly:

#[derive(AsStd140)]
struct Foo {
    value: Mat4, // wow! so sleek!
}

We've got a couple possible solutions:

1. Features for every math library.
   • This would be no fun to implement and maintain.
2. Extend mint, maybe with a trait like the one in kvark/mint#59
   • I'm not sure exactly what form this needs to be in for Crevice to leverage it.

This creates a bit of a blow-up to the number of types in the crate. I'm unsure how we'll organize and verify them for correctness.

Hey, just a heads up.
I was planning to use crevice for a rust-gpu project but I noticed compiling without the std feature currently results in compilation errors.

The usage of String here seems to cause it:

Crevice should implement traits for arrays of types that implement AsStd140.