Add associated constant for WGSL struct definition #44

victorvde · 2023-06-18T16:42:57Z

Closes #7

Notes:

Uses String everywhere. It might be possible to use const_str to optimize it to const &str but I don't think that's really necessary.
Rust Atomic<u32> is translated to WGSL atomic<u32>, not sure if that really makes sense but it's intuitive at least.
This doesn't work great with the include_wgsl! macro.

teoxoy

Thanks for the PR! And sorry for the delayed review.

Uses String everywhere. It might be possible to use const_str to optimize it to const &str but I don't think that's really necessary.

I think it would be ideal if the methods would return &'static str since we have all the required information at compile time.

Rust Atomic<u32> is translated to WGSL atomic<u32>, not sure if that really makes sense but it's intuitive at least.

What I had in mind for atomics is a new #[shader_atomic] attribute that can be used on any u32/i32 since I don't think you'd want cpu atomics to get translated to gpu atomics.

Regarding the new trait, I think we should either add a separate derive for it or have a shader_declaration method on ShaderType where it would return an Option.

Let me know what you think!

src/types/array.rs

derive/impl/src/lib.rs

victorvde · 2023-07-27T01:31:05Z

Thanks for the review, I addressed most of the comments.

--

I think it would be ideal if the methods would return &'static str since we have all the required information at compile time.

This was trickier than expected as traits and const don't work too well together. As such there is now an arbitrary limit of 8912 characters (~300+ fields).

Regarding the new trait, I think we should either add a separate derive for it or have a shader_declaration method on ShaderType where it would return an Option.

Could you explain the rationale?
The first option duplicates code and makes the feature harder to use. The derive already implements multiple traits anyway.
The second option makes a compile-time error into a run-time error which I really don't like.
One thing I considered was putting everything behind a Cargo feature flag, but I'm not sure if that would address any concerns.

teoxoy · 2023-08-19T13:07:15Z

I think it would be ideal if the methods would return &'static str since we have all the required information at compile time.

This was trickier than expected as traits and const don't work too well together. As such there is now an arbitrary limit of 8912 characters (~300+ fields).

Oh wow, I also ran into quite a few rough edges with rust's const stuff, especially when throwing traits in the mix and this seems to be another.

I like the solution you came up with! I don't think anyone will use structs that big and an easy solution would be to break them down into multiple smaller structs; at least until rust supports more const features.

Regarding the new trait, I think we should either add a separate derive for it or have a shader_declaration method on ShaderType where it would return an Option.

Could you explain the rationale? The first option duplicates code and makes the feature harder to use. The derive already implements multiple traits anyway. The second option makes a compile-time error into a run-time error which I really don't like. One thing I considered was putting everything behind a Cargo feature flag, but I'm not sure if that would address any concerns.

My main concern was compile time and binary size but I guess if you don't use the declaration it won't be part of the binary.

Would you be able to test the compile time impact of the new functionality?
If it's not that bad, we shouldn't need a feature flag for it.

teoxoy

The syn version bump and clippy lint are fixed on main, could you rebase?

teoxoy · 2023-08-19T13:11:22Z

Cargo.toml

@@ -2,7 +2,7 @@
 name = "encase"
 version = "0.6.1"
 edition = "2021"
-rust-version = "1.63"
+rust-version = "1.64"


Do we need to bump the MSRV?

teoxoy · 2023-08-19T13:16:12Z

README.md

@@ -18,7 +18,7 @@ Having to manually lay out data into GPU buffers can become very tedious and err

 The core trait is [`ShaderType`] which mainly contains metadata about the given type.

-The [`WriteInto`], [`ReadFrom`] and [`CreateFrom`] traits represent the ability of a type to be written into the buffer, read from the buffer and created from the buffer respectively.
+The [`WriteInto`], [`ReadFrom`] and [`CreateFrom`] traits represent the ability of a type to be written into the buffer, read from the buffer and created from the buffer respectively. The [`ShaderStructDeclaration`] trait allows to generate the WGSL struct definition.


Suggested change

The [`WriteInto`], [`ReadFrom`] and [`CreateFrom`] traits represent the ability of a type to be written into the buffer, read from the buffer and created from the buffer respectively. The [`ShaderStructDeclaration`] trait allows to generate the WGSL struct definition.

The [`WriteInto`], [`ReadFrom`] and [`CreateFrom`] traits represent the ability of a type to be written into the buffer, read from the buffer and created from the buffer respectively. The [`ShaderStructDeclaration`] trait contains the WGSL struct definition.

teoxoy · 2023-08-19T13:28:19Z

derive/impl/src/lib.rs

@@ -14,7 +14,7 @@ pub use syn;
 #[macro_export]
 macro_rules! implement {
    ($path:expr) => {
-        #[proc_macro_derive(ShaderType, attributes(align, size))]
+        #[proc_macro_derive(ShaderType, attributes(align, size, shader_atomic))]


Thanks for implementing the new attribute!

Could you mention it in the docs here?

encase/src/lib.rs

Line 22 in 308bb72

/// Used to implement `ShaderType` for structs

teoxoy · 2023-08-19T14:18:03Z

derive/impl/src/lib.rs

+                } else if attr.meta.path().is_ident("shader_atomic") {
+                    match attr.meta.require_path_only() {
+                        Ok(_) => {
+                            data.shader_atomic = true;


I think we need to make sure that this is only set for integer scalars. The validation could go in generate_field_trait_constraints but I think we need some new metadata for integer scalars to distinguish them from other types.

victorvde added 7 commits June 18, 2023 16:27

initial wgsl_type / wgsl_struct

10928a4

add wgsl layout

2822bfe

change wgpu tests to use generated wgsl

4c42f01

cargo fmt

cb23601

add docs for wgsl_type and wgsl_struct

a16032b

clean up

22d97a6

fix MSRV CI test

b0b65dd

teoxoy reviewed Jul 11, 2023

View reviewed changes

src/types/array.rs Outdated Show resolved Hide resolved

derive/impl/src/lib.rs Outdated Show resolved Hide resolved

derive/impl/src/lib.rs Outdated Show resolved Hide resolved

derive/impl/src/lib.rs Outdated Show resolved Hide resolved

victorvde added 2 commits July 26, 2023 22:08

make wgsl_type const

64fd334

make wgsl_struct const

2dfd7f8

victorvde force-pushed the main branch from 25851af to d50d7d7 Compare July 26, 2023 23:40

shader_atomic attribute

88a3900

victorvde force-pushed the main branch from d50d7d7 to 88a3900 Compare July 26, 2023 23:44

victorvde added 4 commits July 27, 2023 02:55

rename wgsl -> shader

9ce2ee4

add a where clause for ShaderStructDeckaration impl

9216df6

add space in array shader type

192dbf2

random clippy stuff

915bafc

victorvde force-pushed the main branch from 06a624b to 915bafc Compare July 27, 2023 01:03

victorvde changed the title ~~Add function to generate WGSL struct definition~~ Add associated constant for WGSL struct definition Jul 29, 2023

teoxoy reviewed Aug 20, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add associated constant for WGSL struct definition #44

Add associated constant for WGSL struct definition #44

victorvde commented Jun 18, 2023

teoxoy left a comment

victorvde commented Jul 27, 2023

teoxoy commented Aug 19, 2023

teoxoy left a comment

teoxoy Aug 19, 2023

teoxoy Aug 19, 2023

teoxoy Aug 19, 2023

teoxoy Aug 20, 2023

teoxoy Aug 19, 2023

	The [`WriteInto`], [`ReadFrom`] and [`CreateFrom`] traits represent the ability of a type to be written into the buffer, read from the buffer and created from the buffer respectively. The [`ShaderStructDeclaration`] trait allows to generate the WGSL struct definition.
	The [`WriteInto`], [`ReadFrom`] and [`CreateFrom`] traits represent the ability of a type to be written into the buffer, read from the buffer and created from the buffer respectively. The [`ShaderStructDeclaration`] trait contains the WGSL struct definition.

Add associated constant for WGSL struct definition #44

Are you sure you want to change the base?

Add associated constant for WGSL struct definition #44

Conversation

victorvde commented Jun 18, 2023

teoxoy left a comment

Choose a reason for hiding this comment

victorvde commented Jul 27, 2023

teoxoy commented Aug 19, 2023

teoxoy left a comment

Choose a reason for hiding this comment

teoxoy Aug 19, 2023

Choose a reason for hiding this comment

teoxoy Aug 19, 2023

Choose a reason for hiding this comment

teoxoy Aug 19, 2023

Choose a reason for hiding this comment

teoxoy Aug 20, 2023

Choose a reason for hiding this comment

teoxoy Aug 19, 2023

Choose a reason for hiding this comment