-
Notifications
You must be signed in to change notification settings - Fork 567
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
Wasm support #759
Wasm support #759
Conversation
I've been playing around with the demo (in Firefox and Chromium) and everything seems to work, one exception being zoom, which seems to break things quite a bit. When loading example with zoom 100%:
When loading example with zoom > 100%:
When loading example with zoom < 100%:
In both cases != 100%
|
Thanks @Finnerale! I believe I have resolve the issues you found with zooming. This relies on #158 being merged as well. It seems the zooming issue was in part because the context wasn't scaled properly during the resize callback. |
I organized a small repo for building and hosting these examples on the web at https://github.com/elrnv/druid-wasm-examples/, hopefully it can serve as a reference on how one would go about building and deploying these examples to the web. It certainly took me some time to figure it out. |
Woah! Amazing timing! I just discovered druid and was hoping for wasm support and here it is. It appears another limitation of the current state is that it doesn't capture the browser's native functionality of navigating back when you press "backspace" when a text box is focused |
Oh thanks for finding that @ijsnow, in my setup, the browser doesn't respond to backspace so I never noticed this. I was able to reproduce this on another machine, and indeed it makes the text widgets basically unusable. I think this would be an unacceptable limitation, so I patched it by disabling the default backspace functionality altogether. Perhaps this is too conservative and the default backspace behaviour should be enabled when the text widget is not focused, but for this PR I think this is acceptable since most likely the kinds of web applications that would require a GUI like druid would opt for a custom backspace behaviour anyway. The druid-wasm-examples should be updated with the fix now. It would be great if you can verify if this issue is resolved there on your end :) |
@cmyr, @raphlinus: Is there anything else that needs to be done to get this through, or is it just not a priority right now? |
No, I'd like to get this merged while it's warm, just haven't built up the courage to give it a closer look. will do that soon! |
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.
Okay I have some notes but this looks like a pretty good place to start, I'm happy to merge this as a checkpoint soon.
All review comments should be addressed in 3813e3c. Thanks for the feedback! |
The last PR linebender/piet#153 was merged, so this PR is just blocked on having another piet release. |
@elrnv the piet changes have made it to druid. |
A basic wasm backend is added as a continuation of linebender#30. Since std::time isn't available on wasm, an additional dependency is added to abstract std::time::Instant to work with wasm.
This commit ensures that all examples run with minimal changes. More specifically we don't need to remove the .use_simple_logger() call on all Apps.
The resize callback should be registered to the window, since resizing the window doesn't call the resize callback for canvases even if those do get resized.
The scroll example now works as expected on hi and low dpi screens.
This makes passing it easier to pass owned strings when creating a new KeyEvent, which is necessary when interfacing with WASM.
This should be the default behavior to make text widgets usable. At a later iteration it may be better to only disable the browser from going back when a widget is selected that is expecting keyboard input where backspace is intended for something else. I expect this functionality would require some way of accessing the web_sys event from within a druid callback.
The newly introduced lifetime parameter must be specified exmplicitly.
wasm targets cannot be run in the normal way.
The tests cannot be run in the normal way anyways. Leaving this for a future PR to flush out.
@cmyr Thanks! The PR is ready now. |
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.
That's a lot of work here, thanks 🎉 !
I've left some comments and requests tho.
The biggest open issues seem to me testing and clippy (which behaves different for wasm?)
But those can be handled in future PRs.
state | ||
.window | ||
.set_timeout_with_callback_and_timeout_and_arguments_0( | ||
Closure::once_into_js(f).as_ref().unchecked_ref(), | ||
interval, | ||
) | ||
.expect("Failed to call setTimeout with a callback"); |
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.
I haven't worked with wasm yet, but in other places with Closure
there is allways a call to forget
let closure = Closure::wrap(Box::new(f) as Box<dyn FnMut(_)>);
window_state
.window
.add_event_listener_with_callback(event_type, closure.as_ref().unchecked_ref())
.unwrap();
closure.forget();
Not sure if this would need one as well.
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.
From what I understand this is true for Closures created with wrap
or once
, but once_into_js
is special in that it does the forgetting for us. Quoting the wasm_bindgen docs for once_into_js
:
Unlike Closure::once, this does not return a Closure that can be dropped before the function is invoked to deallocate the closure.
druid/examples/wasm/src/examples.rs
Outdated
//! This module is intentionally left blank. | ||
//! The contents are automatically generated in the "build.rs" script. |
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.
I would prefer if this file would not exist at all before building the wasm examples.
Because this file was added to git, the .gitignore
entry for it will have no effect.
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.
Ok I readjusted this mechanism to avoid any changes to any committed files in e072555. Would this work?
#[cfg(not(target_arch = "wasm32"))] | ||
#[cfg(test)] | ||
mod tests; |
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.
Is there a benefit for making the tests work on wasm?
On a quick glance this seems to test nothing platform specific @cmyr?
If it does tho, there should be an issue opened for it.
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.
I am not sure about this one. I left it out for the time being because it requires changes in piet
to get this to compile, (I submitted a PR to add the missing types to piet in linebender/piet#167) and I'm not really familiar with the testing code. I suspect it can be used for testing although it needs to be run in a special way for wasm and not through cargo test
, but I'm not sure. Is this appropriate for a separate PR? For reference there is some code testing the wasm backend in piet.
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.
Sure, this can be done in a later PR, if it makes sens to do so.
Which is what I hoped @cmyr would enlighten me about.
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.
yea, happy for this to be followup.
#![allow(clippy::unreadable_literal)] | ||
|
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.
Wow, that's odd. Does not happen without --target wasm32-unknown-unknown
😒
I could not find out why clippy does that tho
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.
I could be wrong but my impression was that the examples were actually not checked by clippy before, but since the druid-wasm-examples
is now in the workspace, they all get checked with cargo clippy --all -- -D warnings
with or without --target wasm32-unknown-unknown
. I remember having to make some other tweaks to the examples to satisfy clippy.
This commit exposes the error of the missing implementation for file dialogs in the web backend using log::warn. This is a temporary solution until the `open_file_sync` and `save_as_sync` functions propagate the error downstream using Result instead of Option. Co-Authored-By: Leopold Luley <[email protected]>
Co-Authored-By: Leopold Luley <[email protected]>
Co-Authored-By: Leopold Luley <[email protected]>
The types in that module have since made their way into druid itself.
This is most likely not needed.
This commit attempts a different approach at including the automatically generated examples from the parent directory. This method satisfies both rustfmt and cargo test as before, but it also doesn't modify any files in the source tree, keeping the diff clean after a build.
Thanks for the feedback! I tried to address all of the comments as best as I can. Passing the baton back now :) |
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.
Code wise I'm happy now 😄
I installed wasm-pack and things worked out well!
Playing with the examples I've discovered two issues:
-
The
switch
example has an unfortunate name for JS -
The
identity
example color cycling is limited to red
probablyinstant::Instant
being less accurate
As this does not really matter for the example I think this can be ignored
I personally only care about the first point
This commit fixes the generated html file for the switch example to load "switch_demo" instead of "switch" for the switch example. The word switch conflicts with JavaScript's switch statement, which is the reason for this awkwardness.
Regarding the first bullet, for the time being the I did a little digging about the second bullet to find the [MDN article on performance.now()] |
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.
I've nothing more to add to this 👍
Would be happy to merge it.
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.
I've done a scan and everything seems reasonable; I'm happy to at least get this in as a checkpoint!
This is a continuation of #30 and includes all the changes made there + support for scrolling and keyboard input.
As mentioned in #30, there is an issue with
std::time
, which was solved with theinstant
crate, but I believe there are other options available as well.A demo of the working widgets is available.
The limitations in this PR AFAICT are:
This PR also depends on a pull request in
piet
#153, which makes returning thepiet
context easier.Edit: Fixes #128