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

pipeline-overridable constants #39

Open
9SMTM6 opened this issue Aug 8, 2024 · 7 comments
Open

pipeline-overridable constants #39

9SMTM6 opened this issue Aug 8, 2024 · 7 comments

Comments

@9SMTM6
Copy link

9SMTM6 commented Aug 8, 2024

wgsl now supports these, and I think they could speed up some computations.

wgsl_bindgen fails these because naga_oil, which in turn fails because of naga.

Naga uses a search-and-replace strategy for these (at least for backends other than webgpu, webgpu support came later). This wont work for the way wgsl-bindgen is intended to be used, as far as I can tell.

I created an issue for naga_oil with what I could scratch together, but it seems to me that this works fine enough for them.

IDK what the right approach would be if upstream doesn't want this. If you actually want to support this, we would perhaps have to find a way to avoid the connected errors, and then we could make a function that takes the values as arguments... IDK.

@9SMTM6
Copy link
Author

9SMTM6 commented Aug 8, 2024

note that any thoughts on this should probably wait for #37. The naga internals surrounding this were refactored significantly inbetween.

@Swoorup
Copy link
Owner

Swoorup commented Aug 9, 2024

wgsl now supports these, and I think they could speed up some computations.

wgsl_bindgen fails these because naga_oil, which in turn fails because of naga.

Naga uses a search-and-replace strategy for these (at least for backends other than webgpu, webgpu support came later). This wont work for the way wgsl-bindgen is intended to be used, as far as I can tell.

I created an issue for naga_oil with what I could scratch together, but it seems to me that this works fine enough for them.

IDK what the right approach would be if upstream doesn't want this. If you actually want to support this, we would perhaps have to find a way to avoid the connected errors, and then we could make a function that takes the values as arguments... IDK.

Only reason I can think of reason it doesn't work atm with wgsl_bindgen, is because the source is compiled and retranslated back to wgsl. The wgsl writer in naga doesn't support few features. Afaik this is only needed if for example you want to embed the final shader from naga_oil as single string into the build. Which lets you omit naga_oil as a runtime dependency.

wgsl_bindgen does support both embedded as well as generating code that uses naga_oil which would require naga_oil dependency.

We could probably make it work for both UseComposerWithPath and UseComposerEmbed option.

@9SMTM6
Copy link
Author

9SMTM6 commented Aug 9, 2024

is because the source is compiled and retranslated back to wgsl. The wgsl writer in naga doesn't support few features.

I think I understand more of whats going on thanks to that. Now it makes sense why the writer in naga source-code still did not seem to have that feature yet.

The problem is, 'adding' this feature wont work, because as far as I understand, before wgsl gets compiled to... whatever it gets compiled to, these pipeline-overrideable constants get replaced with the actual value.

wgsl_bindgen does support both embedded as well as generating code that uses naga_oil which would require naga_oil dependency.

We could probably make it work for both UseComposerWithPath and UseComposerEmbed option.

I wasn't aware of this feature of this feature. However I'd like to avoid that tbh. I just switched to UseComposerWithEmbed as a test - without adding any pipeline-overridable-constants yet -, and after I added the required naga and naga_oil runtime dependencies and fixed the compiler errors, my resulting brotli compressed WASM binary for the webpage grew by 60% in filesize. Additionally this will likely increase runtime. I don't develop a game or similar, just a visualization of some (2d) simulations, most of the nice features naga_oil at runtime would bring are not required for me.

@Swoorup
Copy link
Owner

Swoorup commented Aug 9, 2024

The problem is, 'adding' this feature wont work, because as far as I understand, before wgsl gets compiled to... whatever it gets compiled to, these pipeline-overrideable constants get replaced with the actual value.

Bummer I don't see much option, in getting UseEmbed to work with pipeline-overrideable constants if you want to avoid naga_oil as a dependency and only use embedded option, due to the way naga_oil is tightly integrated with naga.

Other option is to add a new option that only uses naga instead of naga_oil, but that would mean avoiding naga_oil's feature path imports, defines, etc. Not sure if it is worth the effort.

Out of curiosity, wouldn't naga_oils' defines fill the same use case as this feature though?

@Swoorup
Copy link
Owner

Swoorup commented Aug 9, 2024

I think I understand more of whats going on thanks to that. Now it makes sense why the writer in naga source-code still did not seem to have that feature yet.

Yeah sorry about the poor documentation, it is not expressive.

I wasn't aware of this feature of this feature. However I'd like to avoid that tbh. I just switched to UseComposerWithEmbed as a test - without adding any pipeline-overridable-constants yet -, and after I added the required naga and naga_oil runtime dependencies and fixed the compiler errors, my resulting brotli compressed WASM binary for the webpage grew by 60% in filesize. Additionally this will likely increase runtime. I don't develop a game or similar, just a visualization of some (2d) simulations, most of the nice features naga_oil at runtime would bring are not required for me.

I have the exact use case and requirements as you do for visualizations and not games. 😄

@9SMTM6
Copy link
Author

9SMTM6 commented Aug 9, 2024

Yeah sorry about the poor documentation, it is not expressive.

Eh. I mean, I realized earlier there was some kind of IR translation going on, since the embedded shader had some differences. But when I debugged this issue I did not bring this into connection with the source of the error in naga. Sure, documenting somewhere that the shader will go through some translation would be helpful, but I dont think it makes a huge difference. With how change-happy webgpu still is, which is getting worse when going through wgpu, then naga_oil, and then this project, I don't think documenting beyond this highlight would be an efficient use of time.

Bummer I don't see much option, in getting UseEmbed to work with pipeline-overrideable constants if you want to avoid naga_oil as a dependency and only use embedded option, due to the way naga_oil is tightly integrated with naga.

It seems so.

Out of curiosity, wouldn't naga_oils' defines fill the same use case as this feature though?

Not while staying in any way close to what default webgpu is doing, for certain, which was originally my intention. I when adding this project I just wanted away from the massive amount of boilerplate required for wgpu on rust side (which needs to be adjusted for any change to bind_groups etc, which in turn hurts my ability to experiment), and took imports and compile-time validation as a nice added bonus.

When I would use webgpu (wgpu) directly, then I would be able to write down these constants in the wgsl shader code, and then provide the values for these values in my Rust code when invoking create_*_pipeline.

I want my webgpu code to be understandable for someone (including me 😅) without having to get the whole bevy ecosystem.

To reach anything similar to pipeline-overridable constants with defines, from my understanding I would have to write a bunch of wrapper code. I don't know if I have ability to properly do that right now, and in any case, I would have to port that code when inevitable breaking changes to APIs land. And with how well naga_oil is documented (from my understanding not at all) that will likely just end with me being stuck on an old version, which will then in turn also end up freezing my egui dependency and anything else that interacts with wgpu. Too much work and risk. At that point doing stuff on my own will likely be less work and lead to a better understanding of potential issues on my end.

@Swoorup
Copy link
Owner

Swoorup commented Aug 10, 2024

Eh. I mean, I realized earlier there was some kind of IR translation going on, since the embedded shader had some differences. But when I debugged this issue I did not bring this into connection with the source of the error in naga. Sure, documenting somewhere that the shader will go through some translation would be helpful, but I dont think it makes a huge difference. With how change-happy webgpu still is, which is getting worse when going through wgpu, then naga_oil, and then this project, I don't think documenting beyond this highlight would be an efficient use of time.

I am not sure naga preserves enough information after parsing to be able to support this on its backend writer here. But I think that would be the most plausible place for fixing this.

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

No branches or pull requests

2 participants