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

Add wasm-bindgen feature #554

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

AThilenius
Copy link

@AThilenius AThilenius commented Aug 22, 2024

See issue #292 and discussion #277

This adds a wasm-bindgen feature that decorates structs for wasm/JS bindings, and adds a wasm-only constructor.

There are a couple points worth discussing/considering before merging this:

  • It's a feature, instead of target_arch = "wasm32" because not everyone compiling for WASM needs to access the types from JS
  • const fn cannot be bound in wasm_bindgen, so I crated a separate new (called wasm_bindgen_ctor) function just for the wasm constructor
    • Additionally, we can't (probably shouldn't) just make the existing new non-const, as other const factory functions use it transitively
    • The function is only added when the wasm-bindgen feature is active, it won't pollute auto-complete et. al
  • I chose not to bind all the methods on types, though many could be bound
    • This decision was mostly out of laziness; I didn't want to bind them all if there isn't interest in this PR anyway
    • This will however add a lot of extra #[wasm_bindgen(skip)] decorators
  • SSE2 is exclusive with wasm_bindgen
  • I couldn't figure out why it was still mad when binding an affine, so I skipped it

I need this for my project, but I can use my own fork if this PR doesn't fit well with your direction. No worries.

@bitshifter
Copy link
Owner

The code looks simple enough but I'm not very familiar with bindgen so I have a few questions

It's a feature, instead of target_arch = "wasm32" because not everyone compiling for WASM needs to access the types from JS

Is there any benefit to enabling this for other architectures?

const fn cannot be bound in wasm_bindgen, so I crated a separate new (called wasm_bindgen_ctor) function just for the wasm constructor

Does this mean none of the const fn methods can be used? That seems like it would be quite limiting?

SSE2 is exclusive with wasm_bindgen

This doesn't appear to be consistent, I see that it's true for matrices but it looked like sse2 implementations of Quat, Vec3A and Vec4 still have bindgen enabled. Why did it need to be excluded on matrices?

I need this for my project, but I can use my own fork if this PR doesn't fit well with your direction. No worries.

It has been asked for before so it seems reasonable to consider it. I don't really have a good grasp on how it works though, how is this used in your project?

In addition to gaining some understanding of a new feature to accept a PR, I also need to be able to test it, so I'll know if I managed to break it through other changes. Is there a way this can be tested?

@AThilenius
Copy link
Author

@bitshifter

Is there any benefit to enabling this for other architectures?

Almost certainly no. But while wasm_bindgen implies target_arch = "wasm32", the other way around isn't true. In fact my guess would be that the vast majority of people compiling for WASM (to run on the web, or in some other WASM runtime) won't want to access glam types from JS. wasm-bindgen is just that, it generated bindings to marshal JS calls to WASM Rust, and the other way around. If you don't need to access glam types from JS, them just compiling to WASM is all you need.

Does this mean none of the const fn methods can be used? That seems like it would be quite limiting?

Correct. But... shims can be enabled when the feature is active that expose those functions to JS, just as I did with new and wasm_bindgen_ctor. It's a pain, but remember what wasm-bindgen is going. It's generating a marshal layer between a compiled unmanaged language and a GCed JS runtime. const fn has no meaning to JS.

This doesn't appear to be consistent, I see that it's true for matrices but it looked like sse2 implementations of Quat, Vec3A and Vec4 still have bindgen enabled. Why did it need to be excluded on matrices?

That's entirely possible, I just fused off things until the compiler was happy.

...how is this used in your project?

My project allows you to paint N and P type silicon on an infinite substrate. The core of it is written in Rust and compiled to WASM. It uses WebGL (soon to be WebGPU) to do rendering. I'm building a website around it now, and need to generate binding in-depth to minipulate things. For example, my camera type leans heavily on glam for all the space transforms. For example, this line wouldn't compile without Vec2 itself being bindgen'ed already. And it's much easier to expose the glam types directly then to wrap everything in a newtype, where operators would have to be re-implemented.

Is there a way this can be tested?

I don't think testing makes any logical sense beyond 'does it compile' 🤷 Anything worth testing would be a bug in wasm_bindgen and thus beyond the responsibility of glam to test for.

If you're happy with the direction of this PR I'll finish off the SSE stuff, clean things up and fix the lint errors?

@AThilenius
Copy link
Author

Also totally unrelated, thank you for Glam! You rock. I used a lot of nalgebra back in the Amethyst days, and when Bevy chose Glam I never looked back. Nalgebra might the the right pick for mathematicians, but the generics were pure hell. Glam is a joy to work with, it's my go-to on so many projects.

@bitshifter
Copy link
Owner

bitshifter commented Sep 1, 2024

Almost certainly no. But while wasm_bindgen implies target_arch = "wasm32", the other way around isn't true. In fact my guess would be that the vast majority of people compiling for WASM (to run on the web, or in some other WASM runtime) won't want to access glam types from JS. wasm-bindgen is just that, it generated bindings to marshal JS calls to WASM Rust, and the other way around. If you don't need to access glam types from JS, them just compiling to WASM is all you need.

I think having it on a feature makes sense, but perhaps it would be better to constrain it to wasm32 as well for now if that is sufficient for your needs? E.g. #[cfg_attr(all(feature = "wasm-bindgen", wasm_bindgen, target_arch = "wasm32"))]

It's generating a marshal layer between a compiled unmanaged language and a GCed JS runtime. const fn has no meaning to JS.

OK, so you effectively need to expose the types to JS but don't actually use them from there? That makes sense then.

I don't think testing makes any logical sense beyond 'does it compile'

In that case perhaps compiling with the feature enabled in wasm32 would probably be sufficient to know it's not broken?

Perhaps adding a step to build with this enabled to https://github.com/bitshifter/glam-rs/blob/main/build_and_test_wasm32_firefox.sh or https://github.com/bitshifter/glam-rs/blob/main/build_and_test_wasm32_chrome.sh would suffice? Would there be any way to confirm that types were exposed to bindgen. For example if there was some refectoring of glam and the binggen stuff was inadvertently removed from a type that's the kind of thing that would be good to catch.

You project looks pretty interesting btw, I'll definitely try it out once the website is ready!

@bitshifter
Copy link
Owner

For the lint failure, in this case it's fine to add a #[allow(clippy::too_many_arguments)]

@AThilenius
Copy link
Author

OK, so you effectively need to expose the types to JS but don't actually use them from there? That makes sense then.

Close. I do actually use the types from JS. The editor controls are implemented in TypeScript, because I want to allow for people making new tools as plugins. Even if those are written in a WASM language, until WASI ships you still need a JS bridge to get from WASM to WASM module.

In that case perhaps compiling with the feature enabled in wasm32 would probably be sufficient to know it's not broken?

Yep! Let me poke around your tests a bit today, and see what I can come up with. Wasm bindgen can generate TypeScript types, so an 'example usage' TS file could be checked with tsc to ensure the correct bindings / signatures are exposed. In theory anyway.

@simonask
Copy link
Contributor

Chiming in here because I recently added support for wasmtime's WIT components in glamour.

I'm not sure I see the benefit to exposing host glam functions to a WASM guest. The overhead of calling back and forth makes it a not-great solution.

But what does make sense is to support passing values, so that glam can potentially be used by both the host and the guest with zero overhead (modulo potentially some alignment adjustment). This is trivial to support because glam already supports bytemuck.

(The WIT component support in glamour does not actually leverage this, but just hooks into wasmtime's Lower/Lift traits in a way that should result in a simple memcpy in/out of guest memory. See the tests in that repo for how that looks from a user's perspective.)

@AThilenius
Copy link
Author

@simonask

I'm not sure I see the benefit to exposing host glam functions to a WASM guest. The overhead of calling back and forth makes it a not-great solution.

I was going to argue that it lent some ergonomics to the calling JS code (I don't need to find a JS lib to do common ops, even if the performance is terrible) but I don't actually have a single use case for doing so. Every glam fn I would want to call (apart from new) I've wrapped in other rust logic before exposing to JS. So perhaps binding methods is a solution in search of a problem 🤷‍♂️ Binding the structs themselves is super useful, as are the fields, as you noted.

Also: sorry for the lack of progress. I had my first kiddo, and free-time... well it doesn't really exist right now lol.

@bitshifter
Copy link
Owner

Ah congrats!

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.

3 participants