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

Split out new crate egui-winit from egui_glium #735

Merged
merged 14 commits into from
Sep 28, 2021
Merged

Split out new crate egui-winit from egui_glium #735

merged 14 commits into from
Sep 28, 2021

Conversation

emilk
Copy link
Owner

@emilk emilk commented Sep 22, 2021

This extracts a lot of code from egui_glium into a reusable crate egui_for_winit egui-winit. This can be reused for egui_glow (#685) and many other integrations.

This replaces the 3rd party egui_winit_platform which is used by a few integrations: https://crates.io/crates/egui_winit_platform/reverse_dependencies. egui_for_winit handles a lot more things, like touch input, screen reader, etc.

The name egui_for_winit is not great, but egui_winit was already taken. I would welcome a better name suggestion!

API

egui_for_winit_docs

@emilk
Copy link
Owner Author

emilk commented Sep 22, 2021

@hasenbanck @LU15W1R7H @parasyte @derivator @t18b219k @Gui-Yom - You may be interested in this, and have feedback on the API?

@luiswirth
Copy link
Contributor

I'll take a look in the next days.

Since I own the egui-winit name, we can discuss a transfer of it, if you're interested.

@AlexApps99
Copy link
Contributor

What about egui_base? Might be a bit generic though

Copy link
Contributor

@AlexApps99 AlexApps99 left a comment

Choose a reason for hiding this comment

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

Just some trivial typos, I'll test the code later today if I can

README.md Outdated Show resolved Hide resolved
egui_for_winit/README.md Outdated Show resolved Hide resolved
egui_for_winit/README.md Outdated Show resolved Hide resolved
egui_for_winit/README.md Outdated Show resolved Hide resolved
ARCHITECTURE.md Outdated Show resolved Hide resolved
@KentaTheBugMaker
Copy link
Contributor

I will test this Input integration with egui_vulkano_backend as fast as possible.

egui_glium/Cargo.toml Outdated Show resolved Hide resolved
@AlexApps99
Copy link
Contributor

AlexApps99 commented Sep 23, 2021

It works! 🎉
Rebased my PR (#685) and most things seem to be working
image
Once I clean things up a bit, I'll push it to a branch.

Edit: pushed it to AlexApps99/egui@based, there's some bugginess around resizing but that's the only issue I noticed.

Copy link
Contributor

@TimonPost TimonPost left a comment

Choose a reason for hiding this comment

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

Great work, i can't comment on major decisions or fine nuances yet, however didn't find anything that stood out.

Cargo.lock Outdated Show resolved Hide resolved
ARCHITECTURE.md Outdated
@@ -5,7 +5,7 @@ Also see [`CONTRIBUTING.md`](https://github.com/emilk/egui/blob/master/CONTRIBUT


## Crate overview
The crates in this repository are: `egui, emath, epaint, egui, epi, egui_web, egui_glium, egui_demo_lib, egui_demo_app`.
The crates in this repository are: `egui, emath, epaint, egui, epi, egui_for_winit, egui_web, egui_glium, egui_demo_lib, egui_demo_app`.
Copy link
Contributor

@TimonPost TimonPost Sep 23, 2021

Choose a reason for hiding this comment

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

Maybe egui_for_winit isn't the right name in the current convention? I read that egui-winit crate name was maybe transferable, So ill just leave this as a reminder.

egui_for_winit/src/lib.rs Outdated Show resolved Hide resolved
egui_for_winit/src/lib.rs Outdated Show resolved Hide resolved
egui_glium/src/backend.rs Show resolved Hide resolved
egui_for_winit/src/lib.rs Outdated Show resolved Hide resolved
@KentaTheBugMaker
Copy link
Contributor

I succeeded to integrate this work to my backend .
LGTM
SS_FC
SS_Offscreen

@derivator
Copy link

Looks very nice. My thoughts:

  • Maybe document somewhere that take_egui_input takes care of setting RawInput::time as well. It's very convenient, but confusing for someone coming from egui_winit_platform
  • egui_winit_platform had a convenience function to check if egui wants to handle an event. What are your thoughts on providing something like this?

Copy link

@Gui-Yom Gui-Yom left a comment

Choose a reason for hiding this comment

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

I agree that documenting what this library handles and what it's leaving for the user would be cool.

Also, I spotted a small error with the scale factor.

egui_for_winit/src/lib.rs Outdated Show resolved Hide resolved
@luiswirth
Copy link
Contributor

@emilk
I skimmed over the code and egui_for_winit pretty much provides the same functionality as my egui-winit.

I'm ready to give you the crates.io ownership of the egui-winit crate in order for you to claim the name.
Just be sure to increment the version number accordingly, to not screw up semantic versioning (which I already kinda did), since my crate gets basically replaced.

Since egui-winit is part of my eww (egui + winit + wgpu) crate, I would be very excited if we would also officially support a wgpu renderer. Maybe even someday replacing glium.
You can take a look at egui-wgpu if you're interested in such an addition.

eww itself is a wrapper crate, taking care of the interactions between the winit platform and the wgpu renderer. Maybe that's also a consideration.

@emilk
Copy link
Owner Author

emilk commented Sep 25, 2021

I'm ready to give you the crates.io ownership of the egui-winit crate in order for you to claim the name.

That would be greatly appreciated!

I would be very excited if we would also officially support a wgpu renderer. Maybe even someday replacing glium.

Let's discuss that in #93 instead so this PR doesn't go off topic!

@emilk
Copy link
Owner Author

emilk commented Sep 25, 2021

Maybe document somewhere that take_egui_input takes care of setting RawInput::time as well. It's very convenient, but confusing for someone coming from egui_winit_platform

Done!

egui_winit_platform had a convenience function to check if egui wants to handle an event. What are your thoughts on providing something like this?

This is a good idea. on_event could even return a true if egui is going to use the given event (though that would complicate that already very long function).

One wrinkle to consider is the TAB key which can be pressed to move focus in egui, e.g. to the first text field. The linked code does not take this into consideration. For many applications all TAB:s should go to egui, but maybe for some games no TAB:s should go to egui.

@coderedart coderedart mentioned this pull request Sep 25, 2021
@luiswirth
Copy link
Contributor

@emilk
I've sent you a invitation to become crates.io owner of egui-winit!

@emilk
Copy link
Owner Author

emilk commented Sep 26, 2021

The kebab-case of egui-winit is inconsistent with the snake case of all other egui crates (egui_glium etc), but it can't be helped, and it is still a better name than egui_for_winit.

@emilk
Copy link
Owner Author

emilk commented Sep 26, 2021

TODO:

I'm planning to publish egui-winit together with egui 0.15.0. I have a busy week ahead of me, so it will likely not happen until October.

@emilk emilk changed the title New crate: egui_for_winit Split out new crate egui-winit from egui_glium Sep 28, 2021
@emilk emilk merged commit 1b36863 into master Sep 28, 2021
@emilk emilk deleted the egui_for_winit branch September 28, 2021 15:33
mankinskin pushed a commit to mankinskin/egui that referenced this pull request Sep 29, 2021
@emilk emilk mentioned this pull request May 3, 2022
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.

8 participants