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

feat: Winit 0.30.0 #1675

Closed
wants to merge 11 commits into from
Closed

Conversation

marc2332
Copy link
Contributor

@marc2332 marc2332 commented Apr 30, 2024

Closes #1674

  • Tested on all platforms changed
  • Added an entry to CHANGELOG.md if knowledge of this change could be valuable to users
  • Updated documentation to reflect any user-facing changes, including notes of platform-specific behavior
  • Created or updated an example program if it would help users understand this functionality

image

@marc2332
Copy link
Contributor Author

I bumped the MSRV to 1.70, updated to winit 0.30 and migrate both glutin-winit and the winit-dependant examples to the new trait-based EventLoop API

I would appreciate it if you could test it works in other platforms, I have tried on Windows 11 and it works great!

@marc2332 marc2332 marked this pull request as ready for review April 30, 2024 22:21
@justincredible
Copy link

It's likely undesirable, but the deprecated API can still be supported temporarily by implementing a sealed trait for EventLoop<T> and ActiveEventLoop, then using &impl MySealedTrait instead of &ActiveEventLoop as the parameter to DisplayBuilder::build, create_display, and finalize_window. An enum or Fn + closures also work but are even more awkward.

@@ -1,6 +1,6 @@
[package]
name = "glutin-winit"
version = "0.4.2"
version = "0.5.0"
Copy link
Member

Choose a reason for hiding this comment

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

Drop the version bump.

@@ -36,7 +36,7 @@ jobs:
strategy:
fail-fast: false
matrix:
rust_version: [1.65.0, stable, nightly]
rust_version: [1.70.0, stable, nightly]
Copy link
Member

Choose a reason for hiding this comment

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

Bump rust-version in a separate commit or in a separate PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Separate commit? How does that matter? Anyway, I downgraded it again to 1.65 in this PR, but it now fails https://github.com/rust-windowing/glutin/actions/runs/8950601264/job/24586112813?pr=1675#step:8:30, I can open another PR if you want

Copy link
Member

Choose a reason for hiding this comment

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

I'll do that myself, in general, you should just do a separate commit before your work because you require 1.70.

I'll just send a PR myself bumping MSRV.

match event {
WindowEvent::CloseRequested => event_loop.exit(),
WindowEvent::Resized(size) => {
if size.width != 0 && size.height != 0 {
Copy link
Member

Choose a reason for hiding this comment

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

Can move to guard while at it.

) {
match event {
WindowEvent::Resized(size) => {
if size.width != 0 && size.height != 0 {
Copy link
Member

Choose a reason for hiding this comment

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

move to guard as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What guard? There is no multithreading on the standard example

Copy link
Member

Choose a reason for hiding this comment

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

WinodwEvent::Resized(size) if size.width

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah! Sure, I misunderstood

Comment on lines 54 to 55
} else {
// Only Windows requires the window to be present before creating the display.
Copy link
Member

Choose a reason for hiding this comment

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

Could you extract all of that into a separate method on Application?

Comment on lines 40 to 45
not_current_gl_context: Option<NotCurrentContext>,
gl_context: Option<PossiblyCurrentContext>,
gl_surface: Option<Surface<WindowSurface>>,
window: Option<Window>,
gl_config: Option<Config>,
renderer: Option<Renderer>,
Copy link
Member

Choose a reason for hiding this comment

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

For the context you should use the same field (treat_as_possibly_current). All gl resources, must be dropped before the window, also add a note that Window is dropped at the end.

Render should be dropped first, because it uses the global GL state. Should document that as well.

All of that applies to multithreaded example as well.

Copy link
Contributor Author

@marc2332 marc2332 May 4, 2024

Choose a reason for hiding this comment

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

For the context you should use the same field (treat_as_possibly_current).

Can you clarify this?
treat_as_possibly_current consumes the NotCurrentContext so I need to store PossiblyCurrentContext in order to use it 🤔

Copy link
Member

Choose a reason for hiding this comment

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

yes, just always store PossiblyCurrentContext

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done :-)

@kchibisov
Copy link
Member

It's likely undesirable, but the deprecated API can still be supported temporarily by implementing a sealed trait for EventLoop and ActiveEventLoop, then using &impl MySealedTrait instead of &ActiveEventLoop as the parameter to DisplayBuilder::build, create_display, and finalize_window. An enum or Fn + closures also work but are even more awkward.

Yeah, we should do that as well.

@marc2332
Copy link
Contributor Author

Hey @kchibisov , just updated with the latest changes from master as I just noticed you bumped the MSRV some days ago, do you mind reviewing again? Thanks 😄

Copy link
Member

@MarijnS95 MarijnS95 left a comment

Choose a reason for hiding this comment

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

Comparing this to #1678, it looks like that PR is less intrusive with the glutin_examples (i.e. maintaining the original app/state separation), but the glutin-winit changes are almost identical minus your trait.

Comment on lines 8 to 9
/// Even though [ActiveEventLoop] is the recommended way for using the event
/// loop we still want to have support for [EventLoop], for now.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// Even though [ActiveEventLoop] is the recommended way for using the event
/// loop we still want to have support for [EventLoop], for now.
/// Even though [`ActiveEventLoop`] is the recommended way for using the event
/// loop we still want to have support for [`EventLoop`], for now.

Copy link
Member

Choose a reason for hiding this comment

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

Also: "we still want" is not a valid/useful documentation commit, if you don't first outline why this trait exists and what it does.

/// with it when the [`WindowBuilder`] was passed with
/// [`Self::with_window_builder`]. It's optional, since on some
/// with it when the [`WindowAttributes`] was passed with
/// [`Self::with_window_attributes`]. It's optional, since on some
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// [`Self::with_window_attributes`]. It's optional, since on some
/// [`Self::with_window_attributes()`]. It's optional, since on some

/// **WGL:** - [`WindowBuilder`] **must** be passed in
/// [`Self::with_window_builder`] if modern OpenGL(ES) is desired,
/// **WGL:** - [`WindowAttributes`] **must** be passed in
/// [`Self::with_window_attributes`] if modern OpenGL(ES) is desired,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// [`Self::with_window_attributes`] if modern OpenGL(ES) is desired,
/// [`Self::with_window_attributes()`] if modern OpenGL(ES) is desired,

Comment on lines -97 to -98
#[cfg(android_platform)]
println!("Android window available");
Copy link
Member

Choose a reason for hiding this comment

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

Comparing to #1678, you lost this but still kept Android window removed.

@@ -1,5 +1,9 @@
# Unreleased

# Version 0.5.0

- **Breaking:** Update _winit_ to `0.30`. See [winit's CHANGELOG](https://github.com/rust-windowing/winit/releases/tag/v0.30.0) for more info.
Copy link
Member

Choose a reason for hiding this comment

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

Should mention changes to glutin-winit itself.

Comment on lines 12 to 25
fn create_window(&self, window_attributes: WindowAttributes) -> Result<Window, OsError>;

fn glutin_display_handle(&self) -> Result<DisplayHandle<'_>, HandleError>;
Copy link
Member

Choose a reason for hiding this comment

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

Should document what both are for.

glutin_examples/Cargo.toml Outdated Show resolved Hide resolved
@kchibisov
Copy link
Member

Could apply something like that

diff --git a/glutin-winit/CHANGELOG.md b/glutin-winit/CHANGELOG.md
index a5c458f..9935361 100644
--- a/glutin-winit/CHANGELOG.md
+++ b/glutin-winit/CHANGELOG.md
@@ -1,8 +1,7 @@
 # Unreleased
 
-# Version 0.5.0
-
 - **Breaking:** Update _winit_ to `0.30`. See [winit's CHANGELOG](https://github.com/rust-windowing/winit/releases/tag/v0.30.0) for more info.
+- Add trait `GlutinEventLoop` to handle `{Active,}EventLoop` simultaneously.
 
 # Version 0.4.2
 
diff --git a/glutin_examples/Cargo.toml b/glutin_examples/Cargo.toml
index 981760a..281e1bd 100644
--- a/glutin_examples/Cargo.toml
+++ b/glutin_examples/Cargo.toml
@@ -23,7 +23,7 @@ glutin = { path = "../glutin", default-features = false }
 glutin-winit = { path = "../glutin-winit", default-features = false }
 png = { version = "0.17.6", optional = true }
 raw-window-handle = "0.6"
-winit = { version = "0.30.0", default-features = false, features = ["rwh_05"] }
+winit = { version = "0.30.0", default-features = false, features = ["rwh_06"] }
 
 [target.'cfg(target_os = "android")'.dependencies]
 winit = { version = "0.30.0", default-features = false, features = ["android-native-activity", "rwh_06"] }
diff --git a/glutin_examples/examples/switch_render_thread.rs b/glutin_examples/examples/switch_render_thread.rs
index fea5352..b751c87 100644
--- a/glutin_examples/examples/switch_render_thread.rs
+++ b/glutin_examples/examples/switch_render_thread.rs
@@ -81,19 +81,21 @@ impl Application {
 
 impl ApplicationHandler<PlatformThreadEvent> for Application {
     fn resumed(&mut self, active_event_loop: &ActiveEventLoop) {
-        if self.render_thread_senders.is_none() {
-            let (window, render_context) = create_window_with_render_context(active_event_loop)
-                .expect("Failed to create window.");
-            let render_context = Arc::new(Mutex::new(render_context));
+        if self.render_thread_senders.is_some() {
+            return;
+        }
 
-            let (_render_threads, render_thread_senders) =
-                spawn_render_threads(render_context, &self.event_loop_proxy);
+        let (window, render_context) =
+            create_window_with_render_context(active_event_loop).expect("Failed to create window.");
+        let render_context = Arc::new(Mutex::new(render_context));
 
-            self.window = Some(window);
-            self.render_thread_senders = Some(render_thread_senders);
+        let (_render_threads, render_thread_senders) =
+            spawn_render_threads(render_context, &self.event_loop_proxy);
 
-            self.send_event_to_current_render_thread(RenderThreadEvent::MakeCurrent);
-        }
+        self.window = Some(window);
+        self.render_thread_senders = Some(render_thread_senders);
+
+        self.send_event_to_current_render_thread(RenderThreadEvent::MakeCurrent);
     }
 
     fn user_event(&mut self, _event_loop: &ActiveEventLoop, event: PlatformThreadEvent) {
diff --git a/glutin_examples/src/lib.rs b/glutin_examples/src/lib.rs
index 496efdb..b3e498c 100644
--- a/glutin_examples/src/lib.rs
+++ b/glutin_examples/src/lib.rs
@@ -49,14 +49,9 @@ impl Application {
     fn create_window(&mut self, active_event_loop: &ActiveEventLoop) {
         // Only Windows requires the window to be present before creating the display.
         // Other platforms don't really need one.
-        //
-        // XXX if you don't care about running on Android or so you can safely remove
-        // this condition and always pass the window builder.
-        let window_attributes = cfg!(wgl_backend).then(|| {
-            Window::default_attributes()
-                .with_transparent(true)
-                .with_title("Glutin triangle gradient example (press Escape to exit)")
-        });
+        let window_attributes = Window::default_attributes()
+            .with_transparent(true)
+            .with_title("Glutin triangle gradient example (press Escape to exit)");
 
         // The template will match only the configurations supporting rendering
         // to windows.
@@ -69,7 +64,7 @@ impl Application {
         let template =
             ConfigTemplateBuilder::new().with_alpha_size(8).with_transparency(cfg!(cgl_backend));
 
-        let display_builder = DisplayBuilder::new().with_window_attributes(window_attributes);
+        let display_builder = DisplayBuilder::new().with_window_attributes(Some(window_attributes));
 
         let (window, gl_config) = display_builder
             .build(active_event_loop, template, gl_config_picker)

Since at the current state of things it doesn't work on anything other than windows, I guess.

@marc2332
Copy link
Contributor Author

marc2332 commented Jun 4, 2024

Just pushed @ErichDonGubler changes in this PR so we get the best of both 😄

@marc2332
Copy link
Contributor Author

marc2332 commented Jun 6, 2024

Closing as for some reason #1678 has been merged instead of this.
Both have the same changes anyway, so if anybody was using this branch from git you shall not have any issues when changing the branch to master or when using the official release when it comes out.

@marc2332 marc2332 closed this Jun 6, 2024
@ErichDonGubler
Copy link
Contributor

I think @kchibisov missed what was happening here. Perhaps I should have closed that PR, to make it clear that this was the one we had united behind?

@kchibisov
Copy link
Member

I looked into both PRs, and I was only able to push into @ErichDonGubler one, so I just used the later, because I needed to push some minor fixes myself.

Both PRs were the same, since I checked the diffs and just used my own patch on top of whatever worked for me to push.

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

Successfully merging this pull request may close these issues.

Support winit v0.30
5 participants