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

Add egui_wgpu crate #1564

Merged
merged 24 commits into from
May 12, 2022
Merged

Add egui_wgpu crate #1564

merged 24 commits into from
May 12, 2022

Conversation

emilk
Copy link
Owner

@emilk emilk commented May 3, 2022

Adding a new crate egui-wgpu as an official backend of eframe. This adds feature flags glow and wgpu to eframe, and you can enable either or both. At runtime you can select which backend to use with NativeOptions::renderer.

Forked from #1561 so I can make commits. Based on Nils Hasenbanck's egui_wgpu_backend with additions by @s-nie and myself.

I decided not to touch egui_wgpu/src/renderer.rs too much, and instead create a egui_wpgu::winit::Painter wrapper around it, abstracting all wgpu-specific code away from eframe. eframe now has two very similar-looking run_glow and run_wgpu.

Closes #1555 #1561

Name

egui-wgpu is already taken (https://crates.io/crates/egui_wgpu) by @LU15W1R7H, who was kind enough to transfer ownership of the crate name to me (just like they did for egui-winit!)

Other

I haven't tested the WGPU backend on web yet

@emilk emilk marked this pull request as draft May 3, 2022 18:16
@emilk emilk mentioned this pull request May 3, 2022
@luiswirth
Copy link
Contributor

luiswirth commented May 3, 2022

exciting news!
Sure, we can do the same as with the egui-winit crate.
I'll transfer ownership to you @emilk. I've already sent you an invitation :).

@emilk
Copy link
Owner Author

emilk commented May 3, 2022

That was quick - thanks @LU15W1R7H ❤️

Copy link
Contributor

@s-nie s-nie left a comment

Choose a reason for hiding this comment

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

Works great. Thanks for integrating this so quickly!

How should we treat the feature flags for this?

If in any way possible, enabling both of them at the same time should be an option. This will let users create an app where the backend can be selected at runtime. Building with two backends might seem redundant at first, but in my experience, glow and wgpu complement each other pretty well in that one of them always works, even if the other doesn't because of platform incompatibility.

egui-wgpu/src/renderer.rs Outdated Show resolved Hide resolved
@s-nie
Copy link
Contributor

s-nie commented May 4, 2022

Love the approach with the Renderer struct. Can we derive serde::Deserialize and FromStr for it? For config and command line parsing.

impl FromStr for Renderer {
    type Err = Box<dyn std::error::Error>;

    fn from_str(name: &str) -> Result<Self, Self::Err> {
        match name.to_lowercase().as_str() {
            #[cfg(feature = "glow")]
            "glow" => Ok(Self::Glow),
            #[cfg(feature = "wgpu")]
            "wgpu" => Ok(Self::Wgpu),
            _ => {
                let message = format!(
                "Backend '{name}' is not available. Make sure that the corresponding feature is enabled."
            );
                Err(message.into())
            }
        }
    }
}

@emilk emilk marked this pull request as ready for review May 4, 2022 19:50
@Pjottos
Copy link

Pjottos commented May 7, 2022

I believe the creation of the wgpu::Surface is unsound. The docs for wgpu::Instance::create_surface state:

Safety

  • Raw Window Handle must be a valid object to create a surface upon and must remain valid for the lifetime of the returned surface.

The raw handle should be valid since it is part of the winit window, but the window is not guaranteed to outlive the surface.

There is a proposal for making the API safe (gfx-rs/wgpu#1463) but it doesn't seem like it will be implemented any time soon.

@emilk
Copy link
Owner Author

emilk commented May 9, 2022

@Pjottos good catch, but hard for us to fix. I guess the best we can do is make egui_wgpu::Painter::new unsafe and document this problem.

@emilk
Copy link
Owner Author

emilk commented May 11, 2022

If this looks good I will merge and also publish 0.18 of egui-wgpu to get it up-to-date.

@s-nie
Copy link
Contributor

s-nie commented May 12, 2022

Looks good to me! Thanks again :)

@emilk emilk merged commit 931e716 into master May 12, 2022
@emilk emilk deleted the wgpu-backend branch May 12, 2022 07:02
tracing = "0.1"

# optional:
egui_glow = { version = "0.18.0", path = "../egui_glow", optional = true, default-features = false }
Copy link
Contributor

@MarijnS95 MarijnS95 Jun 15, 2022

Choose a reason for hiding this comment

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

While randomly going through this Cargo.toml, there's a puffin = ["dep:puffin", "egui_glow/puffin"] that enables this optional egui_glow now whenever puffin is enabled. I think the feature is only used to emit profiling scopes in that crate, should it perhaps be updated to egui_glow?/puffin?

Copy link
Owner Author

Choose a reason for hiding this comment

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

good catch; will fix

Copy link
Owner Author

Choose a reason for hiding this comment

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

fixed in d76d055

Copy link
Contributor

@MarijnS95 MarijnS95 Jul 22, 2022

Choose a reason for hiding this comment

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

Thanks! I still had a little entry for this on my TODO list (to not forget), can be crossed off now :)

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.

Consider making epi functionality implementation-agnostic
5 participants