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

stdweb support for eventloop 2.0 #797

Closed
wants to merge 31 commits into from

Conversation

@ryanisaacg
Copy link
Member

commented Feb 20, 2019

Unresolved design questions:

  • Should multiple canvases be supported? I'm leaning no
  • Should fullscreen be supported? If it should be, should it be the size of the window or some attempt to make the canvas actually fullscreen?
  • Is the window icon synonymous with the favicon? I'm leaning yes
  • Should file dropping be supported? It's a JS File object which isn't as useful as it is on desktop
  • Should show / hide be supported?
  • How should refresh rate (opaque to a web page) be handled by VideoMode?

Implementation:

  • Creating a canvas / setting window title
  • DOM -> winit event binding support
  • Event cause
  • ControlFlow handling
    • Poll
    • Wait
    • WaitUntil
    • Exit
  • Pointer events
  • Keyboard events
  • Requesting redraws
  • Canvas positioning within the window
  • KeyboardEvent::which PR to stdweb
  • Pointer capture API (stdweb PR)
  • DPI handling (the browser handles it)
  • Handle on close events (stdweb PR)
  • Mouse wheel events
  • Fullscreen (stdweb PR)
  • Monitor APIs
  • Add logging

Testing / docs:

  • Tested on all platforms changed
  • 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
  • Add stdweb to CI
Create the type layout
Everything typechecks, but nothing is implemented

@ryanisaacg ryanisaacg referenced this pull request Feb 20, 2019

Closed

[WIP] Add wasm32-unknown-unknown support through stdweb #589

1 of 14 tasks complete

@icefoxen icefoxen referenced this pull request Feb 26, 2019

Closed

Highdpi is awful. #796

ryanisaacg added some commits Mar 11, 2019

@ryanisaacg

This comment has been minimized.

Copy link
Member Author

commented Mar 12, 2019

Ok, so I've made a bunch of progress on this (most of it was just churning through various bad designs that didn't work at all) and it's in a state where you can make a window, run an event loop, and receive events! 🎉

I made a roadmap in the PR text to give an indication of where I'm at on the implantation, and the unresolved questions I have about the design. If any maintainers (or users for that matter) have opinions, please let me know!

@solson

This comment has been minimized.

Copy link

commented Mar 13, 2019

Apologies if this question is naive, but would it make sense for winit to support wasm-bindgen's web-sys directly, rather than stdweb? According to this thread, stdweb plans to become a wrapper around web-sys that's a bit higher level (but also more expensive), while web-sys provides the bare minimum only-pay-for-what-you-use interface for Web APIs.

Either way, it would be fine to support both, or stdweb now and web-sys later, so I don't mean to imply the PR should be blocked over this.

@ryanisaacg

This comment has been minimized.

Copy link
Member Author

commented Mar 13, 2019

Essentially: stdweb is what I'm more familiar with and what my downstream project uses. I would be open to supporting both (for example the rand crate allows either via feature flags) if someone else contributes the web-sys backend.

@Osspial

This comment has been minimized.

Copy link
Member

commented Mar 13, 2019

General thoughts on the API questions you have:

Should multiple canvases be supported? I'm leaning no

I'd agree with that for now. We can revisit this once we figure out #696, but for now only having a single canvas is simplest.

Should fullscreen be supported? If it should be, should it be the size of the window or some attempt to make the canvas actually fullscreen?

Yes, and I'd say to actually make the canvas fullscreen. This is a feature that's commonly supported across browsers (video fullscreening, for instance), it's useful to have, and it's consistent with the other backends.

Is the window icon synonymous with the favicon? I'm leaning yes

Eeeeh, I'm less sure about this. If we do end up supporting multiple canvases at some point there's no clean solution for what to do when multiple icons get set on different Window objects. This feels like it should be handled by the parent webpage.

Should file dropping be supported? It's a JS File object which isn't as useful as it is on desktop
Should show / hide be supported?

I'd say no for now. Using the same File API across both desktop platforms and web platforms implies that the data they provide can be used the same way, which doesn't seem to be the case (I'd be surprised if std::fs::File::open works on the web, but I may be wrong there). We can come back to this later though, and see about maybe exposing a different API to make clear that it has different semantics.

@grovesNL grovesNL referenced this pull request Mar 15, 2019

Merged

Add wasm32-unknown-unknown/WebGL support #2554

8 of 8 tasks complete

ryanisaacg added some commits Mar 16, 2019

@ryanisaacg

This comment has been minimized.

Copy link
Member Author

commented Mar 25, 2019

Just a heads up: my progress on this is going to slow down for a while, as I'll probably not have as much time to work on it for the next few weeks.

bors bot added a commit to gfx-rs/gfx that referenced this pull request Jun 8, 2019

Merge #2554
2554: Add wasm32-unknown-unknown/WebGL support r=kvark a=grovesNL

Start to make a more maintainable version of my old branch from #1900 (comment). Heavily WIP but maybe somebody wants to start giving some feedback as I work through the remainder of issues.

This PR replaces gl_generator with glow (https://github.com/grovesNL/glow), a experimental crate meant to unify WebGL/OpenGL types and function signatures. Currently unsupported functions will just panic, but I think it makes sense to move some of the version logic and extension fallbacks in there too (removing them from the gl backend). The WebGL types are now just keys (and `Copy`) which fixes some of the issues I mentioned previously.

This is for the wasm32-unknown-unknown target and uses web-sys.

TODO:
- [X] ~~(major) spirv_cross wrapper on wasm32-unknown-unknown and C++ library through emscripten. For now the quad shaders are hardcoded.~~ See grovesNL/spirv_cross#92. (I'll probably leave the PR open until we work out how the integration between wasm bundles should work)
- [X] ~~Consolidate render loop somehow (see WIP in glow example https://github.com/grovesNL/glow/blob/master/examples/hello/src/main.rs#L118). I need some more ideas on how to work around the `'static` bounds for wasm32 here. The basic idea is that wasm32 has to use callbacks and we use spinloops on the native target, so we would ideally like to unify these (at least for use in examples but also simple applications). For wasm32 with the quad example I just render once and quit instead.~~ Currently winit is working through the implementation of "Event Loop 2.0" and there's been [some effort to write a stdweb backend for this](rust-windowing/winit#797), which we could later help port to wasm32-unknown-unknown. I think for now we can just keep the wasm render loop path separate in gfx examples, and things will work naturally once winit wasm32-unknown-unknown support is ready.
- [X] ~~Remove all remaining `#[cfg(not(target_arch = "wasm32"))]` wherever possible to make this more maintainable.~~ There are a few places remaining that can be removed when we move some logic into glow, like version parsing and extension checking. Some other parts like the runloop can't be addressed without winit support for wasm32-unknown-unknown or other workarounds in quad.
- [X] ~~Decide where to put things like `index.html` for examples (I've excluded it for now) and guide about how to create wasm bundle with wasm-bindgen.~~
- [X] ~~Consider how logging should work – I don't see how to redirect stdout to `console.log` so it's a bit manually at the moment. It would be really cool if things like `info!` just work automatically with wasm32-unknown-unknown.~~ We should be able to use https://github.com/iamcodemaker/console_log for the wasm backend without any major challenges.
- [X] ~~Publish glow~~ Published 0.2.0
- [x] ~~Publish updated spirv_cross~~ Published 0.14.0
- [X] ~~Add wasm-bindgen to CI~~ This has been added

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/gfx-rs/gfx/2554)
<!-- Reviewable:end -->


Co-authored-by: Joshua Groves <josh@joshgroves.com>
@blm768

This comment has been minimized.

Copy link

commented Jun 8, 2019

After a little testing, it's looking like the basic canvas creation and event handling seem to be working (at least with a slightly generous definition of "working") with the web-sys version. At this point, my thought is that I should focus on exploring the integration of the three existing efforts so we can avoid having too much duplication of effort. It's probably too early in the design and implementation process to just merge everything into one massive PR, but some early consideration for integration will probably pay off in the long run.

While I'm thinking about it, I've got a couple of thoughts I've been considering:

  • I noticed that both this PR and #845 use an exception to bail out of the main event loop. If it's not too late in the design process for eventloop-2.0, it seems like providing a "sanctioned" way to bail out of the event loop early would be useful, especially if winit ever picks up another callback-driven backend. I'm not familiar enough with winit yet to propose any particular design, but it seems like it's worth some consideration.
  • When handling the closures in wasm-bindgen, it's necessary to keep the Closure objects alive on the Rust side at least as long as they might be used on the JavaScript side; after the Rust object is dropped, trying to call the JavaScript closure produces an exception. The Closures can just be leaked, but it seems better to clean them up properly. This requires a small design change, which may have some bearing on the integration efforts. It might be worth including something similar to this change in the stdweb backend since it allows for de-registration of the event handlers when the event loop and/or window go out of scope on the Rust side.
@ZeGentzy

This comment has been minimized.

Copy link
Member

commented Jun 12, 2019

@ryanisaacg Are you intending to maintain the stdweb backend after this PR lands?

@ryanisaacg

This comment has been minimized.

Copy link
Member Author

commented Jun 12, 2019

@ZeGentzy Yes, but without a guarantee that I'll have a lot of time for it. I'll do my best to keep it updated, though; at the very least I'll be using it downstream.

@ZeGentzy

This comment has been minimized.

Copy link
Member

commented Jun 12, 2019

Okay. Should prolly add a new column and row to the table in https://github.com/rust-windowing/winit/blob/master/CONTRIBUTING.md then.

Also to the tables in https://github.com/rust-windowing/winit/blob/master/FEATURES.md when you are done.

@Osspial can probably add you to rust-windowing

@Osspial Osspial changed the base branch from eventloop-2.0 to master Jun 13, 2019


event_loop.run(|event, _, control_flow| {
println!("{:?}", event);
console!(log, format!("{:?}", event));

This comment has been minimized.

Copy link
@felixrabe

felixrabe Jun 17, 2019

Contributor

You might want to revert the changes in this file.

This comment has been minimized.

Copy link
@ryanisaacg

ryanisaacg Jun 21, 2019

Author Member

Yup, before merge I'll be doing some cleanup (this is still actively in development and I use this example to test.)

@hecrj
Copy link

left a comment

Looks great! Let me know if I can help!

Show resolved Hide resolved src/platform_impl/stdweb/event_loop.rs Outdated
@hecrj

This comment has been minimized.

Copy link

commented Jun 25, 2019

I am currently working on unifying both the stdweb and web_sys efforts. I am just restructuring the work by @ryanisaacg and @blm768 while removing duplication. Take a look here: https://github.com/hecrj/winit/tree/web/src/platform_impl/web

In summary, I have created a new web platform that contains two backends: stdweb and web_sys. Most of the code in the web platform is backend agnostic. Each backend simply provides Canvas and Timeout types which is where the actual wiring happens. These types are also useful to avoid leaking closures.

The web_sys backend is already usable. I have been able to use it successfully with wgpu and WebGL. Here is an example that renders a triangle and outputs the window events on console: http://hello-triangle.surge.sh/

The stdweb backend should be quite straightforward to implement. I will work on that later today.

Additionally, I have made some other changes:

  • Added support for the ReceivedCharacter event.
  • Made the keyboard and focused events local to the canvas by adding a tabindex attribute. Now you need to click on the canvas to interact with it. This should allow us to easily implement support for multiple windows in the same document, and it also removes awkward global listeners.
  • Because of the previous change and given that a document has only one title, set_title now only changes the alt attribute of the canvas.

Let me know what you think!

@blm768

This comment has been minimized.

Copy link

commented Jun 26, 2019

@hecrj: I haven't looked through your changes thoroughly, but I'm thrilled to learn that it's working (particularly with wgpu, which should be really useful in making my own toy project more portable).

Have you happened to look at the changes in #845? If I recall correctly, they had already started on multi-canvas support, and the WebsysWindowBuilderExt trait might be a useful starting pattern. For the single-canvas cases, it may also be possible to extend that trait to provide some mechanism to use the document title as the window title, ensure that the canvas has focus so it receives events immediately, etc.

That raises a question about winit in general, actually: does it generally report any input events for windows that don't have focus (i.e. "mouse over" events that happen "in passing")? If so, we'll probably want to retain some kind of support for those events.

@felixrabe

This comment has been minimized.

Copy link
Contributor

commented Jun 26, 2019

(Just noticed that this is a Draft PR that I didn't know about previously. Quick intro with very helpful screenshot here: https://github.blog/2019-02-14-introducing-draft-pull-requests/)

@hecrj

This comment has been minimized.

Copy link

commented Jun 26, 2019

@blm768

For the single-canvas cases, it may also be possible to extend that trait to provide some mechanism to use the document title as the window title, ensure that the canvas has focus so it receives events immediately, etc.

I see that #845 uses platform-specific builder attributes to take control of an already existing canvas in the document. Maybe we could take this approach and simply make the Window take control of the whole document when the attribute is not provided.

That raises a question about winit in general, actually: does it generally report any input events for windows that don't have focus (i.e. "mouse over" events that happen "in passing")? If so, we'll probably want to retain some kind of support for those events.

Not sure what the desired behavior is in this case, but the current one reports mouse events just fine even when the canvas loses focus. Right now, the only events that are focus-dependent are keyboard events.

@hecrj hecrj referenced this pull request Jul 2, 2019

Draft

Web support #63

11 of 26 tasks complete
@ZeGentzy

This comment has been minimized.

Copy link
Member

commented Jul 10, 2019

@ryanisaacg As we discussed on Discord, I've made a new web branch with @hecrj's fork. It should be noted that it was four commits behind your branch, so you might want to push those changes to it, if they are still applicable.

Anyways, closing. Thanks!

@ZeGentzy ZeGentzy closed this Jul 10, 2019

@ZeGentzy

This comment has been minimized.

Copy link
Member

commented Jul 26, 2019

@ryanisaacg Can you open a tracking issue with the things at the top? Thanks.

@ryanisaacg

This comment has been minimized.

Copy link
Member Author

commented Jul 26, 2019

@ZeGentzy sure, opened #1072

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.