-
Notifications
You must be signed in to change notification settings - Fork 193
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the PR!
I have a few concerns with it, in addition to the notes I listed.
One concern is the use case. What need is this library going to cover? Are we only talking about glsl-to-spirv path, or something else on top of that? Let's discuss the future applications here before settling on the design. For example, I don't think wgsl2msl
will be hugely helpful, since it requires the BindingMap
and other things to be provided, and there is nothing that would take advantage of the produced MSL, externally.
Another concern is vendoring into Gecko. Unfortunately, cargo-vendor doesn't consider the platform-specific cfgs in Cargo manifests, it just vendors everything. That means that even if wasm-bindgen and friends are wasm-only dependencies, they are still going to be vendored in Gecko, which is something Gecko engineering don't appreciate. For example, in spirv-cross we are using a fork with stripped WASM dependencies currently - kvark/spirv_cross@40aca9f
This sucks quite a bit, I know, but at least there is a way forward. We can just have this as a separate repository, aka naga-wasm
. I know I asked you to do all the wasm stuff in here, but it appears to not be the greatest idea :)
} | ||
|
||
#[wasm_bindgen] | ||
pub fn wgsl2msl(input: &str) -> String { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens when we add any functions to this module? If I understand correctly, these are public extern functions, so nothing is going to strip unused logic parts. If we add wgsl2hlsl
for example, the produced WASM module would have to contain both.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah this isn't really the best solution, just something to get started. The wasm interface should also be split in front-end returning AST and backend taking AST and returning text(or bin) output. But that requires someway to pass the AST efficiently.
Maybe could use features to flag individual front/back-ends?
@@ -9,14 +9,43 @@ repository = "https://github.com/gfx-rs/naga" | |||
keywords = ["shader", "SPIR-V"] | |||
license = "MPL-2.0" | |||
|
|||
[lib] | |||
crate-type = ["cdylib", "rlib"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately that would make the cdylib
target to be generated for every build of naga as a dependency, even of other Rust code. See rust-lang/cargo#4280 .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm, not sure how to solve that, wasm-pack told me this is needed for being able to compile to wasm 😟
NP, at least I learned a bit more about how Rust works :) For reference I originally did this in separate repo: https://github.com/pjoe/wasm-naga In terms of use case, I think the most usable ones will be glsl2spirv and glsl2wgsl, I just added wgsl2msl, as only msl backend was available, and wgsl seemed easier for testing than spirv as frontend |
Hopefully, that's not too painful :) For reference I originally did this in separate repo: https://github.com/pjoe/wasm-naga Let's point to it from our README!
Yes, that sounds great. Also, you are right that the client could control the WASM binary size by changing the requested features, so this concern is addressable. (better than having multiple library targets in a repo) Finally, the concern about What is a showstopper, however, if is the WASM dependencies with regards to cargo-vendor. That's annoying, but the only workaround would be for us to maintain a fork with them removed. It seems cleaner to instead officially maintain |
@kvark: got stubborn 😁, so managed to get the wasm-bingen stuff optional, not quite sure about the crate-type, found this open rust issue: rust-lang/cargo#4881 |
For reference: cargo vendor issue about cargo vendor for single platform: rust-lang/cargo#7058 |
Closing this PR. For now wasm work continues in https://github.com/pjoe/wasm-naga |
Adds support for building to wasm, using wasm-pack.
Includes size optimization settings for release build in Cargo.toml