Skip to content
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.

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

Std430 #1

Closed
LPGhatguy opened this issue Sep 22, 2020 · 10 comments · Fixed by #12
Closed

Std430 #1

LPGhatguy opened this issue Sep 22, 2020 · 10 comments · Fixed by #12
Labels
enhancement New feature or request

Comments

@LPGhatguy
Copy link
Owner

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

@LPGhatguy LPGhatguy added the enhancement New feature or request label Sep 22, 2020
@benmkw
Copy link

benmkw commented Oct 17, 2020

As far as I'm aware the only difference is in struct and array handling where for std430 the alignment of the struct is just the maximum alignment of the members and for array its just the alignment of the member type. For std140 its additionally at least 16, so thats just one if that would be different. I could not yet find the code though https://github.com/LPGhatguy/crevice/search?q=max .

@LPGhatguy
Copy link
Owner Author

std140 struct alignment is currently forced in the AsStd140 derive macro. For deriving AsStd430 we'll probably need to generate a const expression here that looks at the member types involved.

I think we will need separate primitive types for other layouts like std430. Currently, the minimum alignment forcing for matrices, for example, is just a regular associated constant. For example, here is Mat3, whose alignment should (I think) be 12 in std430.

@benmkw
Copy link

benmkw commented Oct 17, 2020

I think you need to look at the member types for std140 as well (for structs and also for arrays, only that for structs you take the max of the member types and for arrays there only is one member type), if one of them has an alignment or more than 16 bytes the whole thing (struct or array) needs to have larger alignment as well. How does glsl-layout handle that?

So this https://github.com/sotrh/learn-wgpu/pull/107/files is my current understanding of the situation/ rules extracted from university slides.

@LPGhatguy
Copy link
Owner Author

Ahah! You're right, based on the OpenGL 4.5 spec, section 7.6.2.2 on std140 layout:

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.

I highly recommend this doc, it has very concise and clear rules for the layout that I apparently need to fix up a little in this crate: https://www.khronos.org/registry/OpenGL/specs/gl/glspec45.core.pdf#page=159

@benmkw
Copy link

benmkw commented Oct 17, 2020

yes exactly, thanks for the link :)

Do you know if glsl-layout does this correctly?

@LPGhatguy
Copy link
Owner Author

I'm not sure. I think glsl-layout does the right thing here, since it relies on Rust's #[repr(align)] attribute, which specifies minimum alignment, not alignment. Looking at the derive macro, however, the generated associated Align type is always Align16. I don't think that ends up mattering, but here is that code: https://github.com/rustgd/glsl-layout/blob/119f5ec524dca42403db2aa07569baa349793e9b/glsl-layout-derive/src/lib.rs#L46-L74

@LPGhatguy
Copy link
Owner Author

Ahahah, I see why this never came up before. None of the primitive types exposed in this crate have a base alignment above 16! If I'm reading the spec correctly, implementing the double precision variants of types should give us some types with a base alignment of 32 to test with.

LPGhatguy added a commit that referenced this issue Oct 18, 2020
…ment

Closes #4.

This PR does a couple things that are intertwined. I'll try to explain each thing and why it's tangled with the others!

First, I added support for structs with alignment over 16 bytes. Thanks to @benmkw in [this comment](#1 (comment)) for pointing out this need!

In order to write sufficient tests for that increased alignment, I needed base types with alignment greater than 16. Instead of adding fake types, I added the double types that we already wanted anyways. I'm not 100% sure about the layout of the double-based matrix types and would love some extra review.

In order to add the new vector and matrix types, I decided it was probably time to switch the vector and matrix declarations to use a macro. I figure this will be useful as part of #3 as well in the future, as well as if/when we introduce the int and bool-based vectors and matrices.
@benmkw
Copy link

benmkw commented Oct 19, 2020

looking at the code it seems to me like everything in src is basically independent of std140/430 so that could maybe be renamed a bit and then it would only be the macro part that would expose another derive AsStd430?

So this would basically be the diff

--- a/crevice-derive/src/lib.rs
+++ b/crevice-derive/src/lib.rs
@@ -76,7 +76,11 @@ pub fn derive_as_std140(input: CompilerTokenStream) -> CompilerTokenStream {
         initializer.push(quote!(#field_name: self.#field_name.as_std140()));
     }
 
-    let struct_alignment = emit_struct_alignment(16, fields);
+    let struct_align = if 130 {
+        emit_struct_alignment(16, fields);
+    } else {
+        emit_struct_alignment(0, fields);
+    };
 
     let type_layout_derive = if cfg!(feature = "test_type_layout") {
         quote!(#[derive(::type_layout::TypeLayout)])
diff --git a/src/std140/primitives.rs b/src/std140/primitives.rs
index b2f6665..0aa2ead 100644

@LPGhatguy
Copy link
Owner Author

It ends up being more complicated than that because of all of the trait and module name references! I put up a WIP PR at #12, also auto-linked above.

@benmkw
Copy link

benmkw commented Oct 19, 2020

Ok cool, feel free to ping me if you think I could help with some feedback/ ideas, otherwise I'll take a look anyways as time permits and if I think I could be helpful :)

leod pushed a commit to leod/crevice that referenced this issue Jun 3, 2023
…ome-alignment

Closes LPGhatguy#4.

This PR does a couple things that are intertwined. I'll try to explain each thing and why it's tangled with the others!

First, I added support for structs with alignment over 16 bytes. Thanks to @benmkw in [this comment](LPGhatguy#1 (comment)) for pointing out this need!

In order to write sufficient tests for that increased alignment, I needed base types with alignment greater than 16. Instead of adding fake types, I added the double types that we already wanted anyways. I'm not 100% sure about the layout of the double-based matrix types and would love some extra review.

In order to add the new vector and matrix types, I decided it was probably time to switch the vector and matrix declarations to use a macro. I figure this will be useful as part of LPGhatguy#3 as well in the future, as well as if/when we introduce the int and bool-based vectors and matrices.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants