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

Re-implement PaintCallbacks With Support for WGPU #1684

Merged
merged 5 commits into from
May 28, 2022

Conversation

zicklag
Copy link
Contributor

@zicklag zicklag commented May 26, 2022

This makes breaking changes to the PaintCallback system, but makes it
flexible enough to support both the WGPU and glow backends with custom
rendering.

Also adds a WGPU equivalent to the glow demo for custom painting.

Closes #1661.


This includes breaking changes to the way that PaintCallbacks are used. Instead of the paint callback downcasting a context argument to a specific backend context, the backend will downcast the callback to a backend-specific callback function.

This is required to allow greater divergence from the backend APIs. For example, the WGPU backend needs two callbacks with different arguments for each, and it also has arguments that are not 'static, meaning that having a dyn Any argument doesn't work.

@zicklag zicklag force-pushed the wgpu-paint-callbacks branch 4 times, most recently from 3f5e4aa to c83c281 Compare May 26, 2022 20:03
Copy link
Owner

@emilk emilk left a comment

Choose a reason for hiding this comment

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

Amazing job! Thank you ❤️

docs/egui_demo_app.js Outdated Show resolved Hide resolved
eframe/src/epi.rs Outdated Show resolved Hide resolved
egui-wgpu/Cargo.toml Outdated Show resolved Hide resolved
egui-wgpu/src/renderer.rs Show resolved Hide resolved
egui-wgpu/src/renderer.rs Show resolved Hide resolved
egui-wgpu/src/winit.rs Outdated Show resolved Hide resolved
egui_demo_app/Cargo.toml Outdated Show resolved Hide resolved
egui_glow/src/painter.rs Outdated Show resolved Hide resolved
egui_glow/src/painter.rs Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
zicklag and others added 3 commits May 27, 2022 08:18
This makes breaking changes to the PaintCallback system, but makes it
flexible enough to support both the WGPU and glow backends with custom
rendering.

Also adds a WGPU equivalent to the glow demo for custom painting.
Co-authored-by: Emil Ernerfeldt <emil.ernerfeldt@gmail.com>
@zicklag
Copy link
Contributor Author

zicklag commented May 27, 2022

Great review points, I addressed them all, I believe, and CI is happy, too!

@@ -230,13 +234,16 @@ pub fn run_wgpu(
painter
};

let render_state = painter.get_render_state().expect("Uninitialized");
Copy link
Contributor

Choose a reason for hiding this comment

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

This always panics on Android, so this breaks the support added in #1634.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well that's not cool! I don't have an Android to test on unfortunately.

Still, I did try to build it for Android real quick. A quick look over and, it appears that there is multiple things breaking the ability to build eframe for android right now, that isn't related to this PR. It looks like #1634 might not have actually gotten eframe building for Android, just egui_wgpu and egui_winit.

Some things I noticed need to be fixed for eframe will build for Android:

  • The inclusion of the arboard crate for clipboard support needs to be made optional, because it doesn't build for android.
  • Same fix for the glutin dependency
  • It appears that the winit::event::Event::Paused event doesn't exist, but it's used in the android event loop code.

For what I can tell, it looks like the eframe crate hasn't ever actually worked for Android yet, but it might not be a big deal to make work. I'm not sure if I'll be able to get the time to try to get it working yet, though.

Copy link
Contributor

Choose a reason for hiding this comment

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

I have things half working on Android. Buttons show up, but no text.

Paused is called Suspended now. glutin master works, and there's an open PR to fix arboard.

It would probably be a good idea to add cargo check --target aarch64-linux-android to CI to avoid build errors at least.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool! We shouldn't actually need glutin anyway if we are using the WGPU backend. I'm not sure why text wouldn't be working.

Copy link
Owner

Choose a reason for hiding this comment

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

Nice to hear some progress on this! Feel free to open a PR to add cargo check --target aarch64-linux-android to CI, and any other needed changes

@Zoxc
Copy link
Contributor

Zoxc commented Aug 21, 2022

This also apparently broke DX12's CPU renderer as egui_demo_app just gives a black window after this PR.

@zicklag
Copy link
Contributor Author

zicklag commented Aug 21, 2022

That's another platform I can't test for unfortunately. 😕

I just have a Linux machine with Vulkan and OpenGL.

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.

3 participants