Expose winit's `MonitorHandle` #13669

tychedelia · 2024-06-04T06:42:46Z

Objective

Adds a new Monitor component representing a winit MonitorHandle that can be used to spawn new windows and check for system monitor information.

Closes #12955.

Solution

For every winit event, check available monitors and spawn them into the world as components.

Testing

TODO:

Test plugging in and unplugging monitor during app runtime
Test spawning a window on a second monitor by entity id
Since this touches winit, test all platforms

Changelog

Adds a new Monitor component that can be queried for information about available system monitors.

Migration Guide

WindowMode variants now take a MonitorSelection, which can be set to MonitorSelection::Primary to mirror the old behavior.

killercup

Good work! Left some comments with suggestions, but no big things.

I haven't tested this, and I'm not sure how to easily do that. Would it make sense to add an example that prints metadata about connected monitors to show how to use this?

crates/bevy_winit/src/winit_monitors.rs

crates/bevy_window/src/monitor.rs

crates/bevy_winit/src/system.rs

crates/bevy_window/src/monitor.rs

alice-i-cecile · 2024-06-04T14:05:26Z

I like this feature, and I'm on board with the general design.

killercup · 2024-06-04T15:03:03Z

Made a small example, feel free to pull in this commit if you think it's useful: killercup@62baba9

tychedelia · 2024-06-04T17:56:22Z

I haven't tested this, and I'm not sure how to easily do that.

Mostly we just need to check that things compile and run on most platforms, but I'm also going to do some experiments with un/replugging monitors just to make sure that we correctly spawn and despawn these as that happens.

Made a small example, feel free to pull in this commit if you think it's useful: killercup@62baba9

Thanks! Will absolutely take this.

mockersf · 2024-06-04T22:15:25Z

would it be possible in the example to open a window in each monitor detected, displaying that monitor information?

tychedelia · 2024-06-05T00:33:46Z

would it be possible in the example to open a window in each monitor detected, displaying that monitor information?

That's a great idea, I noticed we don't have any examples of that anywhere. Will add.

The text updates when displays are connected/disconnected.

tychedelia · 2024-06-05T22:31:55Z

Okay, so in working through this new example, I realized that we really needed to also make the window's mode configurable by a monitor selection, as this is actually the correct way to open a window on a particular monitor in winit. Setting the window's position worked sometimes in my experiments, but depended a lot on whether the app was starting on the primary or secondary monitor.

I did confirm that plugging and unplugging monitors works.

alice-i-cecile · 2024-07-30T19:09:31Z

For the record, I think we should document the limitations and merge this. It's immature, but fully opt-in, and virtually nothing more can be done on our end to improve this. These bugs need to be fixed in winit, and sending users / contributors their way is IMO the best way to actually improve things.

tychedelia · 2024-07-30T21:57:10Z

Unfortuantely had to revert the changes to use BtreeMap/BtreeSet due to a new clippy lint that was complaining about the key potentially being unstable on some platforms. Auditing the winit code it seems as if this isn't an issue, but I wasn't sure that adding an ignore for the lint and making note to audit winit every time they release was worth the minor code quality improvement.

Btw minor suggestion for the example: This could use observers instead of Added/RemovedComponents.

Tried doing this, but the on_remove hook was actually more ugly due to having to query for MonitorRef on DeferredWorld.

killercup

LGTM!

Let's merge this while noting the limitations and get fixes from winit over time as well as have people test this in main :)

killercup

Added a suggestion to add a note on the issues found here as docs to WinitMonitors. We might not want to to link to this PR but a tracking issue.

killercup · 2024-08-05T08:35:55Z

crates/bevy_winit/src/winit_monitors.rs

+use bevy_ecs::system::Resource;
+
+/// Stores [`winit`] monitors and their corresponding entities
+#[derive(Resource, Debug, Default)]


Any value in this being Reflect?

Resources being reflect is mildly useful, but IME not nearly as important as for things that want to be inspected or serialized.

crates/bevy_winit/src/winit_monitors.rs

Co-authored-by: Pascal Hertleif <[email protected]>

alice-i-cecile · 2024-08-06T01:48:07Z

@tychedelia build failure on Android looks related; check the CI logs and ping me when it's fixed up?

tychedelia · 2024-08-06T01:58:15Z

@alice-i-cecile should be fixed now?

crates/bevy_render/src/storage.rs

alice-i-cecile · 2024-10-20T14:26:02Z

Thank you to everyone involved with the authoring or reviewing of this PR! This work is relatively important and needs release notes! Head over to bevyengine/bevy-website#1668 if you'd like to help out.

tychedelia force-pushed the 12955-winit-monitors branch from ffad6f7 to 29458b4 Compare June 4, 2024 06:54

killercup reviewed Jun 4, 2024

View reviewed changes

alice-i-cecile reviewed Jun 4, 2024

View reviewed changes

crates/bevy_window/src/monitor.rs Outdated Show resolved Hide resolved

alice-i-cecile reviewed Jun 4, 2024

View reviewed changes

crates/bevy_window/src/monitor.rs Outdated Show resolved Hide resolved

tychedelia force-pushed the 12955-winit-monitors branch from 33759f8 to fa93d46 Compare June 4, 2024 18:58

tychedelia and others added 9 commits June 5, 2024 14:41

Expose winit's MonitorHandle

2d439fb

Rebase.

b90f972

Add example listing available monitors

eeabf1f

The text updates when displays are connected/disconnected.

Respond to feedback.

54df734

Fmt.

f2ee947

Fmt.

76f997f

Refactor example.

ba38c17

Use monitor position for all window modes.

26ba33c

Fmt.

cc65cb3

tychedelia force-pushed the 12955-winit-monitors branch from fa93d46 to cc65cb3 Compare June 5, 2024 22:30

tychedelia added 3 commits June 5, 2024 15:35

ci.

8e8520a

Clippy.

c151948

Clippy.

d96b352

tychedelia added 5 commits July 30, 2024 15:24

Merge branch 'main' into 12955-winit-monitors

f92840f

Ci.

4cdcfff

Docs.

6dfc335

Fmt.

a25f2be

Revert changes to use BtreeSet/BtreeMap.

b4ab4e5

Ci.

a9e8c68

tychedelia added S-Needs-Review Needs reviewer attention (from anyone!) to move forward and removed S-Blocked This cannot move forward until something else changes labels Jul 30, 2024

killercup approved these changes Aug 4, 2024

View reviewed changes

tychedelia added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Aug 4, 2024

killercup approved these changes Aug 5, 2024

View reviewed changes

Note limitations

e8f3dfc

Co-authored-by: Pascal Hertleif <[email protected]>

alice-i-cecile added X-Contentious There are nontrivial implications that should be thought through and removed X-Uncontroversial This work is generally agreed upon labels Aug 6, 2024

alice-i-cecile added this pull request to the merge queue Aug 6, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Aug 6, 2024

mockersf reviewed Aug 6, 2024

View reviewed changes

crates/bevy_render/src/storage.rs Outdated Show resolved Hide resolved

Fixes for Android.

063be9a

tychedelia force-pushed the 12955-winit-monitors branch from b7da07c to 063be9a Compare August 6, 2024 02:03

alice-i-cecile added this pull request to the merge queue Aug 6, 2024

Merged via the queue into bevyengine:main with commit 3360b45 Aug 6, 2024
30 checks passed

alice-i-cecile mentioned this pull request Oct 20, 2024

Write release notes for PR #13669: Expose winit's MonitorHandle bevyengine/bevy-website#1668

Closed

alice-i-cecile mentioned this pull request Oct 31, 2024

generate-release-notes tool missed some PRs that need stubs bevyengine/bevy-website#1724

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expose winit's `MonitorHandle` #13669

Expose winit's `MonitorHandle` #13669

tychedelia commented Jun 4, 2024 •

edited

Loading

killercup left a comment •

edited

Loading

alice-i-cecile commented Jun 4, 2024

killercup commented Jun 4, 2024

tychedelia commented Jun 4, 2024

mockersf commented Jun 4, 2024

tychedelia commented Jun 5, 2024

tychedelia commented Jun 5, 2024

alice-i-cecile commented Jul 30, 2024

tychedelia commented Jul 30, 2024

killercup left a comment

killercup left a comment

killercup Aug 5, 2024

alice-i-cecile Aug 6, 2024

alice-i-cecile commented Aug 6, 2024

tychedelia commented Aug 6, 2024

alice-i-cecile commented Oct 20, 2024

Expose winit's MonitorHandle #13669

Expose winit's MonitorHandle #13669

Conversation

tychedelia commented Jun 4, 2024 • edited Loading

Objective

Solution

Testing

Changelog

Migration Guide

killercup left a comment • edited Loading

Choose a reason for hiding this comment

alice-i-cecile commented Jun 4, 2024

killercup commented Jun 4, 2024

tychedelia commented Jun 4, 2024

mockersf commented Jun 4, 2024

tychedelia commented Jun 5, 2024

tychedelia commented Jun 5, 2024

alice-i-cecile commented Jul 30, 2024

tychedelia commented Jul 30, 2024

killercup left a comment

Choose a reason for hiding this comment

killercup left a comment

Choose a reason for hiding this comment

killercup Aug 5, 2024

Choose a reason for hiding this comment

alice-i-cecile Aug 6, 2024

Choose a reason for hiding this comment

alice-i-cecile commented Aug 6, 2024

tychedelia commented Aug 6, 2024

alice-i-cecile commented Oct 20, 2024

Expose winit's `MonitorHandle` #13669

Expose winit's `MonitorHandle` #13669

tychedelia commented Jun 4, 2024 •

edited

Loading

killercup left a comment •

edited

Loading