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

WIP - wasm_bindgen backend for eventloop-2.0 #845

Closed
wants to merge 14 commits into from

Conversation

spennydl
Copy link

@spennydl spennydl commented Apr 23, 2019

wasm_bindgen support

This PR adds a wasm backend using wasm_bindgen and web-sys. This backend will be used when target_arch=wasm32 and the websys feature is enabled.

Implemenation status and todo

  • selectable backend
  • basic event loop and window creation
  • wrap a canvas
  • control flow
    • ControllFlow::Poll
    • ControlFlow::Wait
    • ControlFlow::WaitUntil
    • ControlFlow::Exit
  • install handlers for mouse and keyboard events - In Progress
  • install handlers for resize events - In Progress
  • milestone 1 event loop feature-complete
  • pointer capture/manipulation - rethinking this. i'm not sure it makes sense to support?
  • canvas placement/resize through window's getters and setters
  • be responsive - play nice with media queries etc that resize the canvas
  • Tested on all platforms changed - I'm testing using the wasm-pack-template and wasm-app project templates
  • Added an entry to CHANGELOG.md if knowledge of this change could be valuable to users
  • Updated documentation to reflect any user-facing changes, including notes of platform-specific behavior
  • Created an example program if it would help users understand this functionality
  • Updated feature matrix, if new features were added or implemented
  • and more i'm sure! i'll be updating this list closer to milestone 1 being completed

Closes #395

Copy link
Contributor

@goddessfreya goddessfreya left a comment

Choose a reason for hiding this comment

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

Nit: A lot of your files are missing a new line at the end.

@goddessfreya
Copy link
Contributor

Question: Do you intend to update the emscripten backend in an other PR, or are you not interested?

@@ -18,9 +18,12 @@ mod platform;
#[cfg(target_os = "emscripten")]
Copy link
Contributor

Choose a reason for hiding this comment

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

Question: Will emscripten conflict with websys with some targets?

Copy link
Author

Choose a reason for hiding this comment

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

i don't believe so, the target triple used for websys is wasm32-unknown-unknown. i would assume that if someone is building for wasm32-unknown-emscripten they wouldn't also have the websys feature set. i could add something like target_os = "unknown" to the cfg condition below (line21) to be safe

Copy link
Contributor

@goddessfreya goddessfreya left a comment

Choose a reason for hiding this comment

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

Just a question.

@iceiix
Copy link

iceiix commented May 19, 2019

Is this PR missing some files or expected to not compile yet? Getting this error when trying to build, the CI systems are also failing with the same error:

error[E0463]: can't find crate for `wasm_bindgen`
 --> src/platform/websys.rs:1:1
  |
1 | extern crate wasm_bindgen;
  | ^^^^^^^^^^^^^^^^^^^^^^^^^^ can't find crate

@spennydl
Copy link
Author

Is this PR missing some files or expected to not compile yet? Getting this error when trying to build, the CI systems are also failing with the same error:

error[E0463]: can't find crate for `wasm_bindgen`
 --> src/platform/websys.rs:1:1
  |
1 | extern crate wasm_bindgen;
  | ^^^^^^^^^^^^^^^^^^^^^^^^^^ can't find crate

This backend needs to be built for the wasm32-unkown-unkown target, and it looks like none of the jobs are currently building for that. The build command we need is cargo build --target wasm32-unkown-unknown --features="websys".

I'll be pushing some changes tomorrow sometime, i can update the circleci config to include it.

@ryanisaacg ryanisaacg mentioned this pull request Jun 6, 2019
31 tasks
@blm768
Copy link
Contributor

blm768 commented Jun 6, 2019

So, I had no idea this PR existed, and I started hacking on the stdweb support PR (#797) to add web-sys support. So far, it seems like there will end up being a fair bit of duplication between the two implementations because they need to interface with the same Web APIs, so it might be worthwhile to consider whether all the WebAssembly efforts thus far could be merged into something more coherent.

@spennydl
Copy link
Author

spennydl commented Jun 9, 2019

So, I had no idea this PR existed, and I started hacking on the stdweb support PR (#797) to add web-sys support. So far, it seems like there will end up being a fair bit of duplication between the two implementations because they need to interface with the same Web APIs, so it might be worthwhile to consider whether all the WebAssembly efforts thus far could be merged into something more coherent.

I agree! In part I feel like that reflects the state of webassembly right now 😝 I'd be interested to hear @ryanisaacg's opinion given he's so far into the stdweb work, as well as from the project owners. Maybe open an issue?

@ryanisaacg
Copy link
Contributor

I think the stdweb version will be pretty much feature-complete within the month. At this point it can already run a full application, given you're not using one of the things I haven't implemented yet.

I think it probably makes the most sense to finish the stdweb version and then add web-sys alongside, possibly inline with cfg declarations to switch between them. This means that changes can be easily applied to both web backends at once. However, I don't know much about idioms in web-sys, and it may be difficult to exactly mirror the structure of the stdweb code.

@Osspial Osspial changed the base branch from eventloop-2.0 to master June 13, 2019 05:29
@goddessfreya
Copy link
Contributor

Hey, we got a new web branch for all this web stuff! Closing. If there is any features in this PR not present in the new branch, file us a new PR. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

Support wasm32-unknown-unknown backend?
5 participants