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.

Sign up for GitHub

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

Jump to bottom

Update to winit 0.29 #3606

Closed

fornwall wants to merge 8 commits into emilk:master from fornwall:master

Contributor

fornwall commented Nov 21, 2023 •

edited

Loading

Closes Update to winit 0.29 #3542

This is a start for updating to winit 0.29.

A lot of things patched out, broken and not tested, but at least the egui_demo_app starts with both glow and wgpu, as well as pure_glow (but are not functional).

Review feedback and patches to fill out missing features and fix issues are welcome!

Replaces #3496 (closed that by mistake and couldn't reopen).

fornwall mentioned this pull request

WIP: Update to winit 0.29 #3496

Closed

MarijnS95 reviewed

View reviewed changes

Cargo.lock

+               "ndk-sys 0.5.0+25.2.9519653",
+               "num_enum 0.7.1",
+               "raw-window-handle 0.5.2",
+               "raw-window-handle 0.6.0",

Contributor

MarijnS95 Nov 21, 2023

You will want to disable default features on the ndk to get rid of this.

(If there's no direct ndk dependency, this will happen with the next winit 0.29.4 release - we forgot it in the update)

Contributor Author

fornwall Nov 21, 2023

There is no direct ndk dependency, I guess we can await winit 0.29.4.

MarijnS95 reviewed

View reviewed changes

Cargo.lock

               dependencies = [
                "libc",
                "log",
-               "ndk",
+               "ndk 0.7.0",

Contributor

MarijnS95 Nov 21, 2023

The diff-context suggests that this is ndk-glue, which is long gone and replaced with android-activity. Stale dependency?

Contributor

MarijnS95 Nov 21, 2023

Should be fixed in ndarilek/tts-rs#50.

fornwall commented

View reviewed changes

crates/eframe/src/native/glow_integration.rs

-                              }
-                              EventResult::Wait
-                          }

Contributor Author

fornwall Nov 22, 2023 •

edited

Loading

We should be able to remove this, as winit::event::Event::Resumed is handled above.

Let's make sure to test on Android - there is a eframe using example in android-activity.

Contributor

MarijnS95 Nov 22, 2023

While you're working on glow_integration, it's probably wise to remove this stale comment entirely:

egui/crates/eframe/src/native/glow_integration.rs

Lines 36 to 46 in e037489

    
           // Note: that the current Glutin API design tightly couples the GL context with 
        
           // the Window which means it's not practically possible to just destroy the 
        
           // window and re-create a new window while continuing to use the same GL context. 
        
           // 
        
           // For now this means it's not possible to support Android as well as we can with 
        
           // wgpu because we're basically forced to destroy and recreate _everything_ when 
        
           // the application suspends and resumes. 
        
           // 
        
           // There is work in progress to improve the Glutin API so it has a separate Surface 
        
           // API that would allow us to just destroy a Window/Surface when suspending, see: 
        
           // https://github.com/rust-windowing/glutin/pull/1435

The issue linked there has long been closed and fixed. Windows/Surfaces are only associated with context when/while it's made current, and it looks like eframe supports Android now with a proper Resumed/Suspended cycle?

fornwall commented

View reviewed changes

crates/eframe/src/native/glow_integration.rs

		repaint_asap = true;
		glutin.resize(viewport_id, **new_inner_size);

Contributor Author

fornwall Nov 22, 2023

In winit 0.29 ScaleFactorChanged does not expose the new OS suggested window size. But, a Resized event will come (recently fixed on macOS - rust-windowing/winit#3214).

fornwall commented

View reviewed changes

crates/eframe/src/native/run.rs

                   let mut builder = create_event_loop_builder(native_options);
                   crate::profile_scope!("EventLoopBuilder::build");
-                  builder.build()

Contributor Author

fornwall Nov 22, 2023

TODO: Add error handling.

fornwall commented

View reviewed changes

crates/eframe/src/native/run.rs Outdated Show resolved Hide resolved

fornwall commented

View reviewed changes

crates/eframe/src/native/run.rs

@@ @@ -160,7 +162,7 @@ fn run_and_return( @@
                           EventResult::Exit => {
                               log::debug!("Asking to exit event loop…");
                               winit_app.save_and_destroy();
-                              *control_flow = ControlFlow::Exit;
+                              event_loop_window_target.exit();

Contributor Author

fornwall Nov 22, 2023 •

edited

Loading

Do we still need this winit_app.save_and_destroy() - we are going to get a Event::LoopExiting event later?

fornwall commented

View reviewed changes

crates/eframe/src/native/run.rs

@@ @@ -132,7 +134,7 @@ fn run_and_return( @@
                       match event_result {
                           EventResult::Wait => {
-                              control_flow.set_wait();
+                              event_loop_window_target.set_control_flow(ControlFlow::Wait);
                           }
                           EventResult::RepaintNow(window_id) => {
                               log::trace!("Repaint caused by {}", short_event_description(&event));

Contributor Author

fornwall Nov 22, 2023

Cane some of this be simplified or removed for winit 0.29?

fornwall commented

View reviewed changes

crates/eframe/src/native/run.rs Outdated Show resolved Hide resolved

fornwall commented

View reviewed changes

crates/eframe/src/native/run.rs Outdated Show resolved Hide resolved


          WIP: Update to winit 0.29

c93defd

fornwall changed the title ~~WIP: Update to winit 0.29~~ Update to winit 0.29

fornwall mentioned this pull request

Embark's pull requests EmbarkStudios/rust-ecosystem#20

Open

26 tasks

fornwall added 3 commits

November 25, 2023 09:28


          Merge branch 'master' into master

1a07d9d


          Make it build again

60a1792


          Update to winit 0.29.4

b65e24d

emilk added eframe egui-winit dependencies labels

emilk reviewed

View reviewed changes

crates/egui-winit/Cargo.toml Outdated Show resolved Hide resolved

emilk reviewed

View reviewed changes

crates/egui-winit/Cargo.toml Outdated Show resolved Hide resolved

emilk reviewed

View reviewed changes

Owner

emilk left a comment

Thank you so much for working on this ❤️

crates/egui_glow/examples/pure_glow.rs Show resolved Hide resolved

gerwin3 commented Nov 27, 2023

I think this PR also fixes #3415 because the winit key representation changed :)

emilk added 4 commits

November 27, 2023 17:41


          Return winit::error::EventLoopError to caller

5b1b091


          Fix clippy lint

c6f0704


          Return errors from run_and_exit

4491fae


          Only allow winit 0.29.4 and up

5e326fb

Owner

emilk commented Nov 27, 2023 •

edited

Loading

Btw, creating a PR from your master branch is a pretty bad idea for a couple of resons:

It means commits added by me are pushed directly to your master
It creates an annoying branch name-clash for other contributors

…that's why the egui PR template suggest you don't do it ;)

In fact, I'm having trouble adding new commits to this PR now, for some obscure git reason

emilk mentioned this pull request

libwayland-client segfault introduced in a dep update #2952

Open

fornwall deleted the branch emilk:master

November 27, 2023 21:44

fornwall closed this

fornwall deleted the master branch

November 27, 2023 21:45

fornwall mentioned this pull request

Update to winit 0.29 #3648

Closed

Contributor Author

fornwall commented Nov 27, 2023 •

edited

Loading

@emilk Really sorry about that, was just a mistake to create from a master branch.

Replaced with #3649 to create from a winit-0.29 branch - does that work better?

fornwall mentioned this pull request

Update to winit 0.29 #3649

Merged

Owner

emilk commented Nov 28, 2023

Thanks @fornwall - I can push commits to the new branch

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

dependencies eframe egui-winit