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

Library upgrades and updates #940

Merged
merged 29 commits into from
Jan 17, 2024
Merged

Conversation

tychedelia
Copy link
Collaborator

@tychedelia tychedelia commented Oct 10, 2023

Mostly working, but need to double check a few things around egui. And make sure all the shaders are still working with wgsl changes.

  • Switch from async-std to tokio.
  • Switch from egui_wgpu_backend to the in-tree egui-wgpu.
  • Upgrade to egui version 0.23.
  • Upgrade to wgpu version 0.17.
  • Upgrade to winit version 0.28.

TODO:

  • Ensure all targets (i.e. WASM) still working.
  • Figure out whether we can use eframe as a replacement for epi in the new egui version. (answer: no)
  • Resolve issue from Library upgrades and updates #940 (comment) in the draw_capture_hi_res example.
  • Figure out resize issue on Ubuntu.
  • Establish egui pattern to replace epi.
  • Egui demo colors.

@infinity-creative
Copy link

infinity-creative commented Oct 11, 2023

I can confirm that egui seems to be working correctly in my project. I will let you know if anything comes up.

@infinity-creative
Copy link

infinity-creative commented Oct 11, 2023

Wasm builds now break.

I think this is due to converting to tokio.

tokio+mio - seems to break on WASM

 Compiling mio v0.8.8
error[E0432]: unresolved import `crate::sys::IoSourceState`

@infinity-creative
Copy link

@tychedelia I attempted to test WASM compiling by pulling out tokio. But got suck (new to rust) - I have left a PR on your repo it might help it might not :)

@tychedelia
Copy link
Collaborator Author

@tychedelia I attempted to test WASM compiling by pulling out tokio. But got suck (new to rust) - I have left a PR on your repo it might help it might not :)

Thanks! I totally blanked on testing WASM. I'll give your change a shot today or otherwise make sure things are working with the WASM builds.

@tychedelia
Copy link
Collaborator Author

@infinitylunacreative just pushed some fixes that should allow tokio to run on wasm targets. will you give it a shot and let me know if that works (on the head of my fork)?

@tychedelia tychedelia mentioned this pull request Oct 11, 2023
6 tasks
@tychedelia
Copy link
Collaborator Author

Looks like @mitchmindtree ran into some of the same questions re: eframe in this previous upgrade attempt here #861. It does appear that the upstream might be willing to accept changes to eframe to make it work better with our use case. It would be wonderful if there was a more fully featured example of a nannou app that uses the previous epi functionality, but I couldn't find one.

@infinity-creative
Copy link

@infinitylunacreative just pushed some fixes that should allow tokio to run on wasm targets. will you give it a shot and let me know if that works (on the head of my fork)?

Thanks, I will take a look today.

@infinity-creative
Copy link

@tychedelia I can confirm the WASAM build is working for me 🥳 - Thank you!

@infinity-creative
Copy link

@tychedelia I decided to do some more testing. I tried it on linux. WASAM works great.

However the "normal" builds throw this error.

error[E0432]: unresolved import `winit::platform::unix`
   --> /home/infinity/work/nannou/nannou/src/window.rs:783:34
    |
783 |             use winit::platform::unix::WindowBuilderExtUnix;
    |                                  ^^^^ could not find `unix` in `platform`
error[E0599]: no method named `with_class` found for struct `WindowBuilder` in the current scope
   --> /home/infinity/work/nannou/nannou/src/window.rs:784:29
    |
784 |             window = window.with_class("nannou".to_string(), "nannou".to_string());
    |                             ^^^^^^^^^^ method not found in `WindowBuilder`

@tychedelia
Copy link
Collaborator Author

@tychedelia I decided to do some more testing. I tried it on linux. WASAM works great.

However the "normal" builds throw this error.

error[E0432]: unresolved import `winit::platform::unix`
   --> /home/infinity/work/nannou/nannou/src/window.rs:783:34
    |
783 |             use winit::platform::unix::WindowBuilderExtUnix;
    |                                  ^^^^ could not find `unix` in `platform`
error[E0599]: no method named `with_class` found for struct `WindowBuilder` in the current scope
   --> /home/infinity/work/nannou/nannou/src/window.rs:784:29
    |
784 |             window = window.with_class("nannou".to_string(), "nannou".to_string());
    |                             ^^^^^^^^^^ method not found in `WindowBuilder`

Looks like there was a breaking change in winit: https://github.com/rust-windowing/winit/blob/master/CHANGELOG.md?plain=1#L255.

Not sure the best approach here. I think I'm going to enable xorg by default and make wayland an additional feature flag.

@infinity-creative
Copy link

@RobWalt @tychedelia - Is there anything you need/want me to test?

Copy link
Contributor

@RobWalt RobWalt left a comment

Choose a reason for hiding this comment

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

@tychedelia What's the motivation behind the switch to tokio? I get that async-std isn't the latest async runtime. In the context of graphics programming with wgpu I just saw a lot of use of pollster instead and maybe that's useful here aswell since we're always just blocking anyways?

Imo it would be better if we split this PR into smaller ones which have a smaller scope. So the async refactor could be its own PR and so on.

@anselanza
Copy link
Contributor

I can't offer much help here, but just want to commend the effort to bring Nannou up to date regarding WGPU and Egui. I really like this framework - and have used it in at least one project in production - but I was getting concerned about the apparent lack of updates recently.

@tychedelia
Copy link
Collaborator Author

tychedelia commented Jan 13, 2024

Since I opened this, winit has released an 0.29 version which includes their event loop 3.0 rework that has a substantial number of changes. I've attempted an upgrade, and while there's a lot of little fiddly changes that aren't too bad, there are some substantial changes that are somewhat difficult to integrate.

  1. Key has been split into an an enum representing differences between character keys and "named" keys.

  2. LOTS of changes to events emitted by winit. The biggest is probably moving RedrawRequested to WindowEvent. These are probably not too bad to refactor but there's lots of changes here and so it's a bit hard to analyze what events might be relevant to users.

It would be really nice to be able to land a 29.x upgrade in order to get nannou up to a stable state with regards to one of its most important dependencies, but this potentially will incur a lot of user visible breakage for what's ultimately pretty minimal gain for the user.

@tychedelia
Copy link
Collaborator Author

36a724e fixes the issue with the egui demo app, but isn't the right pattern for our users. likely more to do here, it's rendering correctly now though.

@tychedelia tychedelia marked this pull request as ready for review January 17, 2024 02:38
let proxy = app.create_proxy();
egui.do_frame_with_epi_frame(proxy, |ctx, epi_frame| {
egui_demo_app.setup(&ctx, epi_frame, None);
let mut egui_demo_app = egui_demo_lib::DemoWindows::default();
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not sure the best way to clean this up. Maybe make the Egui constructors accept an App instance so we can wire that up internally?

egui.do_frame_with_epi_frame(proxy, |ctx, frame| {
egui_demo_app.update(&ctx, frame);
});
let ctx = egui.begin_frame();
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These should probably just be moved into a do_with_frame method on egui to keep it simple.

Copy link
Member

@mitchmindtree mitchmindtree left a comment

Choose a reason for hiding this comment

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

Wicked work @tychedelia, I really appreciate you diving into this!

I've opened up a PR with some small tweaks and fixes if you're interested here - once those are merged I'd be more than happy to see this land :)


As a small side-note, it seems like this PR runs into the same issues that I ran into in #861 w.r.t. colours not quite matching up. From memory, this has something to do with egui changing something related to the way they do gamma correction in their fragment shader and some assumptions they make around web targets vs desktop targets. I think nannou uniquely runs into this as we don't target the window surface texture directly like most egui apps do - instead we draw to an intermediary frame with a linear colour format.

All that said, I think we might be best to:

  1. Land this as is (with aforementioned tweaks PR)
  2. Open an issue for tracking the egui colour problem.
  3. Aim to solve the problem through our bevy refactor when switching from nannou_egui to the bevy_egui crate.

This still might require some thought around texture colour formats etc, but I think we're better off investing that effort as a part of the bevy-refactor, rather than delaying it longer? Open to other suggestions!

Comment on lines 419 to 425
// impl epi::RepaintSignal for RepaintSignal {
// fn request_repaint(&self) {
// if let Ok(guard) = self.0.lock() {
// guard.wakeup().ok();
// }
// }
// }
Copy link
Member

Choose a reason for hiding this comment

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

Happy for this to be removed if it's no longer required?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

good catch, thanks!

Copy link
Member

Choose a reason for hiding this comment

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

Oh woops was going to address this in the other PR but I forgot 😅

@tychedelia
Copy link
Collaborator Author

@mitchmindtree yeah, seeing the colors are super noticeable here in the color picker example.

i'm good with this plan! we can fast follow anything that might come up from further user testing.

image

@mitchmindtree
Copy link
Member

Okydokey let's land this!

I'll follow-up with a PR bumping versions so we can publish to crates.io 👍

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.

5 participants