asny / three-d

2D/3D renderer - makes it simple to draw stuff across platforms (including web)
MIT License
1.29k stars 109 forks source link

Custom `Event::UserEvent(T)` support #322

Closed mdegans closed 1 year ago

mdegans commented 1 year ago

RE: #320 - Winit custom event support

This PR adds support for sending custom events into the winit EventLoop. This is handy when you want to wake up the event loop from another thread (eg. in response to some IO).

mdegans commented 1 year ago

Sorry about the breakage. I will fix the Window in test_window.rs tomorrow. I thought I had run all the tests locally, but apparently not.

asny commented 1 year ago

Sorry about the breakage. I will fix the Window in test_window.rs tomorrow. I thought I had run all the tests locally, but apparently not.

No problem at all, I broke master just before going on vacation 😬😄 to run an example with the test window, run it with the 'test' feature and not the 'window' feature.

Anyway, I think the changes look good, really nice work 👍

mdegans commented 1 year ago

No problem at all, I broke master just before going on vacation 😬😄 to run an example with the test window, run it with the 'test' feature and not the 'window' feature.

Yeah, I was testing with cargo test --all-features. I had a look through and think it wasn't running because:

#[cfg(all(feature = "test", not(feature = "window"), not(target_arch = "wasm32")))]
mod test_window;
#[cfg(all(feature = "test", not(feature = "window"), not(target_arch = "wasm32")))]
pub use test_window::*;

(and window feature would have been enabled as well, so that code wouldn't have been compiled or tested with --all-features). In the future I'll run the CI under my own account.

Anyway, I think the changes look good, really nice work 👍

Thanks. I am new to the language and have a lot to learn still. Hopefully it's rusty enough.

mdegans commented 1 year ago

Ok. I don't think I'm going to add any more commits to this. Hopefully it's not too much and can prove useful. Today I adapted an example from the mock alarm I am working on. On desktop, the example can be run from the repo root with cargo run --example user_event. I also did manage to add Window::from_event_loop for wasm32, but I can't figure a way to test it yet since EventLoopProxy is not Send on wasm32 and tokio::task::spawn, used in the example code, requires it to be.

I figure either the bounds on spawn would need to change for wasm (cause only single thread) or EventLoopProxy's web implementation would need to implement Send. If either of those things happen, it should build and run. I'm not familiar enough with Rust and it's async ecosystem to know if there's another way -- and without threads or some sort of task abstraction, I can't figure out a way to send events using the proxy from outside the render loop. Any advice on this would be appreciated since It'd be nice to demonstrate the feature on all platforms.

asny commented 1 year ago

Thanks for the hard work, as I said before it looks good 🚀

As I'm on vacation I can't even run your example, but it looks nice. However, I would like it to be a bit more minimalistic if possible? I would like to avoid adding more resources to the repo as it the repo size is really big already. Also, maybe you can make it work on web if you don't use threads to add custom events but add them from inside the render loop? I know it's not that interesting but I think it's ok that most work is left for the user. Alternatively, we can do without an example in this case I think, it's not necessary, it's a nice to have 🙂 maybe that's actually the best approach since it's not working on headless and web anyway.

About web and threading, I think it can be solved by thread_local but I don't want to use that in an example, it's not nice 😄

mdegans commented 1 year ago

I totally get it :) The example doesn't really make sense without threads or some thread abstraction. I thought about sending the event from within the loop but I don't think that can work because the events need to fire for the loop to iterate when waiting for events (so they would never fire from within the loop).

I should have asked first if you wanted an example in any case. I will revert the example today. After I do that, would you prefer I squash to a single commit or do you prefer to have the multiple commits and squash them yourself?

asny commented 1 year ago

No worries, if you asked I might have said it would be nice with an example, I probably wouldn't predict it's not a good idea 🙂

It might be possible to add custom events async on web, at least it would make sense to use async as a replacement for threads on web. But anyway, it's not something we need to worry about I think 🙂

This is just perfect, I usually rebase, I prefer as small commits as possible so it's easier to find bugs. Thank you so much for your contribution 💪🚀👍

mdegans commented 1 year ago

Yw. I wasn't sure it wasn't a good idea until I tried. I am still not sure there's not a way, but I don't know enough about tokio, wasm, or how async in rust behaves on web to do what I want.

The logic to my app is mostly synchronous, but there are opportunities to overlap work so the code is mostly synchronous with some threading and channels dedicated to IO so I can block the render thread waiting for external events from multiple external sources (sockets, signals). Most people probably don't want to do that, but I'm running on battery so I want everything to stop. Hopefully somebody else finds it useful. Thanks again for the great library and the useful examples.

asny commented 1 year ago

I think there is a way, but if nobody needs it, it's not worth spending time on. I've spend a lot of time on async stuff and I don't see why this would be any easier 🙂 and anyway, this is not a core feature of the crate, it's about event handling not rendering, so I don't think an example is necessary, the example should be in winit instead. That said, I still think it's a really nice addition and I'm sure others will find it useful 🥳

mdegans commented 1 year ago

I've spend a lot of time on async stuff and I don't see why this would be any easier 🙂

Yeah, I have done async stuff in Python and some other languages, so I know about how async+await works and coroutines implemented with generators and event loops and stuff, but not tokio and wasm. I would think there might be a way to send an event from the gui, but I don't know how. I took a look at your PBR example. The Window::render_loop still blocks, so inside an async fn, it's going to act the same as a sync fn. Perhaps if Window::render_loop was async fn and could yield when idle, but that would change the api, and yeah, probably require changes in winit.

asny commented 1 year ago

I think you're right, it requires changes to winit, but it is also possible to do the window handling on web yourself (I did that before when winit didn't have support for wasm). For example, see this example that uses web_sys::request_animation_frame. So the question is if we can give the request_animation_frame an async function? It might require that Rust enables async closures, which is a feature which hasn't been started. It might be possible to do without it, I don't know.