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

Refactor epi #212

Closed
parasyte opened this issue Mar 8, 2021 · 8 comments
Closed

Refactor epi #212

parasyte opened this issue Mar 8, 2021 · 8 comments
Labels
feature New feature or request

Comments

@parasyte
Copy link
Contributor

parasyte commented Mar 8, 2021

I'm experimenting with egui_winit_platform, which is intentionally lightweight. It does not attempt to provide persistence, or take over the event loop (unlike, say, egui_glium::run). But this lightweight approach means that application developers need to interact with epi directly. This isn't a bad thing, but it has exposed some code smells in epi that I want to address here.

Is your feature request related to a problem? Please describe.

A short list of curiosities I've encountered so far:

  • epi::IntegrationInfo requires some unusual fields, including:
    • seconds_since_midnight is only used by the demo app.
    • native_pixels_per_point doesn't feel like it belongs because physical pixels should only be relevant to the presentation layer. E.g. the GPU is concerned with this metric for rendering, but epi doesn't do any rasterization. (And it looks like epi::Frame::set_pixels_per_point was deprecated in 0.10 anyway.)
    • cpu_usage? What purpose does this serve for generic applications? Is this another leaky abstraction like the two above?
  • epi::backend::FrameBuilder has some unusual fields, including:
    • http is a conditionally compiled field that requires crates to use feature flags to create a FrameBuilder. (E.g. there is no Default impl and it doesn't use the builder pattern to customize optional functionality.) This field is completely missing from documentation because it is behind a feature gate.
    • output is an AppOutput field which is peculiar because it tries to provide some feedback mechanism that is separate from the normal immediate mode GUI control flow. These actions do nothing on web apps by definition.

I'm calling these out specifically because Frame is necessary for epi::App implementors, even when unnecessary . See:

fn update(&mut self, ctx: &egui::CtxRef, _frame: &mut epi::Frame<'_>) {
self.demo_windows.ui(ctx);
}
And these are cross-cutting concerns that app implementors should not have to deal with.

Describe the solution you'd like

  1. Ideally, remove the cross-cutting pieces, like those described above. A second best would be hiding them with Option<T> and impl Default.
  2. AppOutput feels like an anti-pattern. What does this solve that can't be done with normal GUI event handling?
  3. Apart from reducing things that seem superfluous, I believe the epi::App trait could be more useful if it focused solely on application/GUI functionality like persistence, and less on window manager and rendering duties (IIUC, these should be provided by the platform and backend, respectively).

Describe alternatives you've considered

N/A.

Additional context

The apps I build with the winit platform support crate are probably just not going to use epi::App. Maybe when they get sufficiently complex, I might want to take advantage of persistence. But to take advantage of persistence with the current trait, I need to deal with all of this craziness.

Granted, it isn't a lot to deal with, but running the demo app is 15 lines of useless boilerplate (ugh!) and 1 line to draw and handle events for the whole app (wow!)

@parasyte parasyte added the feature New feature or request label Mar 8, 2021
@KentaTheBugMaker
Copy link
Contributor

split http glium as egui_http_native and http web as egui_http_web

@emilk
Copy link
Owner

emilk commented Mar 8, 2021

I'm experimenting with egui_winit_platform, which is intentionally lightweight. It does not attempt to provide persistence, or take over the event loop (unlike, say, egui_glium::run). But this lightweight approach means that application developers need to interact with epi directly

There is no need to interact with epi directly unless you are making apps for eframe. I have no idea why https://github.com/hasenbanck/egui_wgpu_backend chose to use epi. For instance https://github.com/mvlabat/bevy_egui and https://github.com/not-fl3/emigui-miniquad only depend on egui.

If using https://github.com/hasenbanck/egui_wgpu_backend forces you to 15 lines of boilerplate, then I suggest you open an issue in that repo instead of this one.

@emilk
Copy link
Owner

emilk commented Mar 8, 2021

I know epi, eframe, egui_web and egui_glium is a confusing mess that interact in difficult ways. My motivating goals is to be able to write the same code and compile it both to native and to web, and this was the easiest way I could accomplish that. I'd love to boil them down to one simple crate (eframe) and replace all the leaky abstractions with third-party crates. I'd love to focus more on egui and (much) less on the above crates.

Some paths of investigation:

egui-miniquad can be compiled both natively and to web, but is missing some features (high-dpi support, cursors, opening urls, maybe more). Personally I really like miniquad for it's minimalism and fast compile times. But maybe we could compile egui_winit_platform for web?

Find or create a http request crate that worked both natively and on web. The lack of this is the only reason for the http feature in epi (maybe we could get ureq to work with a web-sys backend?)

Find or create a crate for opening links that works both native and with the web-sys backend (and with options to open in same or background tab).

Find or create a crate that can do persistence that is backed either by a file system or by local storage, depending on compile target (WASI?).

Find or create a crate for getting the time of day that works both natively and on the web (WASI?).

@parasyte
Copy link
Contributor Author

parasyte commented Mar 8, 2021

There is no need to interact with epi directly unless you are making apps for eframe. I have no idea why https://github.com/hasenbanck/egui_wgpu_backend chose to use epi. For instance https://github.com/mvlabat/bevy_egui and https://github.com/not-fl3/emigui-miniquad only depend on egui.

To be clear, the crate mentioned does not use epi at all. The demo app based on egui_demo_lib does, and that's how I ran into this.

@emilk
Copy link
Owner

emilk commented Mar 9, 2021

Oh, I see! I saw epi in https://github.com/hasenbanck/egui_wgpu_backend/blob/master/Cargo.toml#L13 and thought that was the source of the headache.

Yes, it would be awesome if egui_demo_lib could be divorced from epi. Beyond the problems mentioned above (e.g. having a http library that works on all platforms) there is also the problem of texture allocation to solve there.

Note that parts of egui_demo_lib can be used without epi::Frame, e.g. egui_demo_lib::DemoWindows.

I used to have struct DemoInput with things like seconds_since_midnight and cpu_usage. I guess we could revert back to something like that to remove some of the ugly parts of epi.

@parasyte
Copy link
Contributor Author

parasyte commented Mar 9, 2021

In fact, epi::TextureAllocator is implemented for egui_wgpu_backend::RenderPass so it can support user-defined textures. epi is also re-exported for convenience, but the crate user should not need to interact with epi to write their app. That explains why you saw the dependency in its Cargo.toml.

It isn't that epi is intrinsically problematic. I think you have identified many of the headaches within epi already, and that's all I'm really asking. I am not blocked on any of these issues. Just felt it would be diligent to create a ticket so that some of the cleanups and improvements could be tracked.

emilk added a commit that referenced this issue Oct 19, 2021
chrono works both natively and on web.

Related: #212
@emilk
Copy link
Owner

emilk commented Apr 16, 2022

A lot of eframe/epi has been refactored now (on master). http is gone, and so is the output field (or is hidden).

Do you think we can close this issue? Or perhaps clarify it to be more up-to-date with how epi/eframe looks today?

@parasyte
Copy link
Contributor Author

I agree, let's close this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants