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

Use vello_shaders crate to load and preprocess WGSL #563

Merged
merged 3 commits into from
May 3, 2024

Conversation

armansito
Copy link
Collaborator

This replaces the shader preprocessing code in the vello crate with invocations to the vello_shaders crate, which supports both reading from a directory at runtime (for hot_reload) and bundlng pre-compiled shader code into the binary (which is suitable for wasm).

This also moves the CPU shader code over to the vello_shaders crate behind the "cpu" feature.

Resolves #467

@waywardmonkeys
Copy link
Collaborator

Since the copyright check is failing, you'll need to modify the 2 checks to account for the new location of the CPU shaders.

That's in .github/copyright.sh

Copy link
Member

@DJMcNab DJMcNab left a comment

Choose a reason for hiding this comment

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

Awesome. I looked into this previously, but thought the hot reload support would have been dropped - I missed that from_dir function.

It's unfortunate that we can't use the pipeline struct of vello_shaders (because it doesn't allow parallel initialisation), but that could be resolved in the future, I guess?

I've not ran this locally, but CI passes (and since #439, that actually means something)

@@ -55,6 +55,7 @@ workspace = true

[dependencies]
vello_encoding = { workspace = true }
vello_shaders = { workspace = true }
Copy link
Member

Choose a reason for hiding this comment

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

Do we still need the compile feature to be enabled even if we're not hot reloading?

I guess it doesn't really matter, because we share the Naga version with wgpu

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's a good point. Even though the features match, I think there is merit to excluding the feature when it's not needed. Though I generally tend to rely on linkers being smart about excluding unused code from binaries (which is desirable in the wasm build case) I went ahead and made "compile" an optional feature, which feels right to me.

src/shaders.rs Outdated
)
};
($name:ident, $bindings:expr) => {
add_shader!($name, $bindings, &full_config)
add_shader!($name, $bindings)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is the exact as above and can be removed

Copy link
Collaborator

@DasLixou DasLixou May 1, 2024

Choose a reason for hiding this comment

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

Pretty interesting, does rust/clippy not cover equal macro branches? O.o

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good catch!

@armansito
Copy link
Collaborator Author

It's unfortunate that we can't use the pipeline struct of vello_shaders (because it doesn't allow parallel initialisation), but that could be resolved in the future, I guess?

This is something I've been considering. I'm not entirely convinced yet that the shaders crate should abstract the pipeline initialization (which is what the currently unused PipelineHost trait declarations were intended for). If we do that, then it might make sense to have the shaders crate handle multi-threaded pipeline compilation.

I should write down my thoughts on this in an issue. I'm particularly thinking about non-wgpu engines and I'm not sure yet how to best organize that code.

@waywardmonkeys waywardmonkeys added this to the Vello 0.2 release milestone May 3, 2024
@armansito armansito added this pull request to the merge queue May 3, 2024
Merged via the queue into main with commit e4ca553 May 3, 2024
15 checks passed
@armansito armansito deleted the use-shaders-crate branch May 3, 2024 14:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

vello should import vello_shaders
4 participants