Add basic support for window icons #8130

emwalker · 2023-03-19T14:55:16Z

Objective

This PR adds an icon to the left of the title in the window taskbar on Windows and X11.

Looking for high-level feedback on whether this implementation is going in the right direction before cleaning up and submitting. A lot of the concepts in Bevy are new to me, and it sounds like the approach that has been suggested is a workaround and might not have a lot of prior art to consult. More context here.

Solution

TBD

Open questions

What to do about failing test run in the bevy_render crate? Perhaps this will require adding back bevy_winit = ["bevy_winit/x11", "bevy_winit/wayland"] and adding the wayland-sys library to CI. But there might be another way to fix the failing test run.
Is there a modification to the current approach that will allow the systems to run several times and update the icon each time?

Changelog

Add an icon to the left of the title in the window taskbar on Windows and X11.

Migration Guide

N/A

emwalker · 2023-03-19T15:24:30Z

examples/window/window_settings.rs

+fn setup(mut commands: Commands, asset_server: Res<AssetServer>) {
+    let icon_handle = asset_server.load("branding/icon.png");
+    commands.spawn(WindowIcon(Some(icon_handle)));
+}


It might be possible to simplify all of this by adding an icon_path: Option<PathBuf> or an enum with variants for a path and for bytes for specific image formats to a field on the Window struct and then adding the two systems above when one of the low-level Bevy plugins runs. But first I thought it would be good to check whether this PR gets the general idea right.

A benefit of having the developer queue up the two systems is that they could potentially change the icon during the running of the game. I'm not sure anyone would ever want that, but this is something that was demonstrated in an earlier PR for this story.

Yeah, let's keep this simple here.

Yeah, let's keep this simple here.

By "simple," are you thinking API-simple (adding a field to Window), or implementation-simple (sticking with the current approach)?

Thinking you were probably referring to the enum idea, so sticking with the current approach.

Yeah, I meant that I prefer the current approach :)

Once it occurred to me that several moving parts would need to be orchestrated in order to make this work without issues, I took the liberty of making a plugin. I'm not attached to it, though, so no problem if it's dropped.

emwalker · 2023-03-19T15:33:42Z

crates/bevy_render/src/texture/icon.rs

+        info!("window icon set");
+        commands.entity(entity).remove::<WindowIcon>();
+    }
+}


There's two uncorrelated collections here — the WindowIcons in flight, and the various windows. In this PR we're just ignoring that complexity. I wasn't sure how much precision would be needed in a first pass.

I think we probably want to align the window icon to the corresponding window, since we're storing it as a component.

emwalker · 2023-03-19T15:49:10Z

crates/bevy_render/Cargo.toml

@@ -78,3 +79,4 @@ encase = { version = "0.5", features = ["glam"] }
 # For wgpu profiling using tracing. Use `RUST_LOG=info` to also capture the wgpu spans.
 profiling = { version = "1", features = ["profile-with-tracing"], optional = true }
 async-channel = "1.8"
+winit = { version = "0.28", default-features = false }


Might make sense to re-export winit::window::Icon from bevy_winit instead of including winit here.

Agreed, I think that's cleaner.

emwalker · 2023-03-19T15:53:18Z

crates/bevy_render/src/texture/image.rs

@@ -613,6 +615,23 @@ impl CompressedImageFormats {
    }
 }

+// Convert an [`Image`] to `winit::window::Icon`.
+impl TryInto<Icon> for Image {
+    type Error = anyhow::Error;


A previous PR had a purpose-specific error type, which might be good to use here (not sure).

Yeah I'd prefer to use a custom error type over anyhow.

alice-i-cecile · 2023-03-19T16:03:52Z

crates/bevy_render/src/texture/icon.rs

+        }
+
+        info!("window icon set");
+        commands.entity(entity).remove::<WindowIcon>();


Definitely don't think we should be removing the Window icon component here once it's set.

I was worried about a busy loop of always attempting to check whether an icon had been set and then exiting early if it was. I looked into using the typestate pattern, e.g., WindowIcon<Loading>, WindowIcon<Loaded>, etc., and then only querying for WindowIcon<Loading>, but ran into difficulties with that.

Is there a way to remove this code from the hot path once the icon has been set?

Set a run condition on the system itself, checking if any of the WindowIcon components have been changed. That will stop it from running at all, and will be extremely quick to check.

As I acquaint myself with Bevy, I'm realizing that the Changed<WindowIcon> state will only last a tick, until the next &mut WindowIcon occurs, while the image asset itself will take several ticks to load, by which point the Changed<WindowIcon> state will have been cleared two or more ticks in the past. This delay between the Changed<T> and the loaded image asset seems to rule out a simple read-only check of some kind, instead requiring a component-specific state machine to track when things are in place to set the icon.

If the state machine is stored on the component itself, mutating the component to update the state enum will interfere with the Changed<WindowIcon> check. And polling in the hot path for whether to set the icon seems to be unavoidable unless loading is carried out in a separate app state. Using app state seems to have the drawback of not being generic and instead requiring an app-specific setup.

I'm sure this is all just me misunderstanding how one is supposed to do things in Bevy when an asset needs to be fully loaded. I'll poke around and try to better understand the desired approach in this case.

I'm thinking something similar to the implementation for run_once might work here.

alice-i-cecile · 2023-03-19T16:06:51Z

crates/bevy_render/src/texture/icon.rs

+    mut query: Query<(Entity, &mut WindowIcon)>,
+    mut winit_windows: NonSendMut<WinitWindows>,
+) {
+    for (entity, window_icon) in query.iter_mut() {


This whole system could probably be simplified significantly.

Basically:

for (id, window) in &mut winit_windows.windows { if let Ok(WindowIcon(maybe_handle)) = query.get(id) { window.set_window_icon(maybe_handle); } else { window.set_window_icon(None); } }

I tried to stick pretty close to this. One thing I noticed is that it might be more efficient for the outer loop to start with the query and check for associated windows than the other way around. Probably not needed, though.

alice-i-cecile · 2023-03-19T16:08:49Z

crates/bevy_render/src/texture/icon.rs

+
+/// An icon that can be placed at the top left of the window.
+#[derive(Component, Debug)]
+pub struct WindowIcon(pub Option<Handle<Image>>);


A simple new(handle: Handle<Image>) method would go a long way to improving usability here :)

alice-i-cecile · 2023-03-19T16:09:46Z

crates/bevy_render/Cargo.toml

@@ -44,6 +44,7 @@ bevy_render_macros = { path = "macros", version = "0.11.0-dev" }
 bevy_time = { path = "../bevy_time", version = "0.11.0-dev" }
 bevy_transform = { path = "../bevy_transform", version = "0.11.0-dev" }
 bevy_window = { path = "../bevy_window", version = "0.11.0-dev" }
+bevy_winit = { path = "../bevy_winit", version = "0.11.0-dev" }


This should be made into an optional (but on by default) dependency, behind a winit feature flag.

alice-i-cecile · 2023-03-19T16:10:41Z

Okay good! The overall module structure like this is much less bad than I was worried about. I think we can make the underlying system a bit less janky, but once that's done this should work nicely.

JMS55 · 2023-03-19T21:24:05Z

Does it make sense to provide an API for setting the window icon? On Linux, you're supposed to use a .desktop file (which also lets it show up in UI's). On Windows, you're supposed to embed a resource file into the exe. On macOS I'm not sure.

alice-i-cecile · 2023-03-19T22:19:28Z

Does it make sense to provide an API for setting the window icon? On Linux, you're supposed to use a .desktop file (which also lets it show up in UI's). On Windows, you're supposed to embed a resource file into the exe. On macOS I'm not sure.

I like the cross-platform nature of this: it'll make the build process significantly less annoying when releasing games. Although maybe this approach messes up how apps show up in e.g. the launcher too?

mockersf · 2023-03-21T21:52:33Z

Does it make sense to provide an API for setting the window icon? On Linux, you're supposed to use a .desktop file (which also lets it show up in UI's). On Windows, you're supposed to embed a resource file into the exe. On macOS I'm not sure.

macOS, Android and iOS are also handled by providing a file with the binary (and packaged in the apk or app)

mockersf · 2023-03-21T21:56:39Z

making bevy_render depends (optionally but enabled by default) on bevy_winit doesn't feel good for something that won't work on most platforms

emwalker · 2023-03-21T21:58:44Z

making bevy_render depends (optionally but enabled by default) on bevy_winit doesn't feel good for something that won't work on most platforms

I think everyone feels weird about it. See this thread for context.

emwalker · 2023-03-21T22:15:47Z

Looking into why the build is failing for wayland-sys. Someone might what's going on here.

mockersf · 2023-03-21T22:20:42Z

Looking into why the build is failing for wayland-sys. Someone might what's going on here.

you enabled the wayland feature of bevy_winit but wayland dependencies are not installed in CI. You change makes it always enabled.

emwalker · 2023-03-21T22:23:54Z

you enabled the wayland feature of bevy_winit but wayland dependencies are not installed in CI. You change makes it always enabled.

I probably misunderstood this request, in that case. Or, if not, I'm thinking I should add the missing dependency to CI.

mockersf · 2023-03-21T22:28:44Z

crates/bevy_render/Cargo.toml

@@ -27,6 +28,8 @@ wgpu_trace = ["wgpu/trace"]
 ci_limits = []
 webgl = ["wgpu/webgl"]

+bevy_winit = ["bevy_winit/x11", "bevy_winit/wayland"]


CI failures comes from this line, none of those features should be enabled here

Thank you! That fixed things.

Now I remember why those were added:

crates/bevy_render/src/extract_param.rs

emwalker · 2023-03-25T13:37:50Z

Is this approach in this PR basically sound? Should I clean it up and move it out of draft? Or should I abandon it and let someone else take a crack at it? I'm find abandoning it, if people think this isn't quite the right approach and it's going to be hard to get it into shape.

Add basic support for window icons

9f9f374

emwalker force-pushed the add-window-icon branch from 9d7147e to 9f9f374 Compare March 19, 2023 14:57

emwalker commented Mar 19, 2023

View reviewed changes

Use map_err

65ebb16

emwalker commented Mar 19, 2023

View reviewed changes

emwalker mentioned this pull request Mar 19, 2023

A field in WindowDescriptor resource to set the window's icon ? #1031

Open

alice-i-cecile added C-Enhancement A new feature A-Windowing Platform-agnostic interface layer to run your app in labels Mar 19, 2023

alice-i-cecile reviewed Mar 19, 2023

View reviewed changes

emwalker added 2 commits March 19, 2023 10:23

Re-export winit::windows::Icon

1ffaebd

Make winit optional

f8a208d

emwalker added 8 commits March 21, 2023 15:11

Iterate on PR feedback

67c0a13

Move icon-related helper structs into bevy_render::texture::icon

5ce861e

Merge branch 'main' into add-window-icon

f9dcf52

Fix stray feature gate

7b6fe97

Drop stray feature gate

323e226

Fix refactoring issues

08e205f

Restore image.rs

36c8e48

Drop stray feature gate

d9d6384

emwalker requested a review from alice-i-cecile March 21, 2023 22:15

mockersf reviewed Mar 21, 2023

View reviewed changes

emwalker added 2 commits March 21, 2023 20:09

Fix build failure

c9a7cda

Generalize the argument to icon::insert_component

8fe20ec

irate-devil reviewed Mar 22, 2023

View reviewed changes

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

emwalker added 2 commits March 22, 2023 18:49

Merge branch 'main' into add-window-icon

efdf5ce

Clarify wording of comment

617c531

emwalker force-pushed the add-window-icon branch from 687d662 to 617c531 Compare March 23, 2023 03:49

Step checking for loaded asset after a certain number of iterations

cf4aa79

Simplify max iterations check

48d0891

Tatsukiyoshi mentioned this pull request Apr 30, 2023

Can not set the icon to Windows desktop App with Bevy 0.10.1 Tatsukiyoshi/Weekend_Programming#771

Closed

mnmaita mentioned this pull request Oct 17, 2023

Add icon to WindowDescriptor and Window #5488

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add basic support for window icons #8130

Add basic support for window icons #8130

emwalker commented Mar 19, 2023 •

edited

emwalker Mar 19, 2023 •

edited

alice-i-cecile Mar 19, 2023

emwalker Mar 19, 2023 •

edited

alice-i-cecile Mar 19, 2023

emwalker Mar 21, 2023

emwalker Mar 19, 2023 •

edited

alice-i-cecile Mar 19, 2023

emwalker Mar 19, 2023

alice-i-cecile Mar 19, 2023

emwalker Mar 19, 2023 •

edited

alice-i-cecile Mar 19, 2023

alice-i-cecile Mar 19, 2023

emwalker Mar 19, 2023 •

edited

alice-i-cecile Mar 19, 2023

emwalker Mar 21, 2023

emwalker Mar 21, 2023

alice-i-cecile Mar 19, 2023

emwalker Mar 21, 2023

alice-i-cecile Mar 19, 2023

alice-i-cecile Mar 19, 2023

alice-i-cecile commented Mar 19, 2023

JMS55 commented Mar 19, 2023

alice-i-cecile commented Mar 19, 2023 •

edited

mockersf commented Mar 21, 2023 •

edited

mockersf commented Mar 21, 2023 •

edited

emwalker commented Mar 21, 2023 •

edited

emwalker commented Mar 21, 2023

mockersf commented Mar 21, 2023 •

edited

emwalker commented Mar 21, 2023 •

edited

mockersf Mar 21, 2023

emwalker Mar 22, 2023

emwalker Mar 22, 2023

emwalker commented Mar 25, 2023 •

edited

Add basic support for window icons #8130

Are you sure you want to change the base?

Add basic support for window icons #8130

Conversation

emwalker commented Mar 19, 2023 • edited

Objective

Solution

Open questions

Changelog

Migration Guide

emwalker Mar 19, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emwalker Mar 19, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emwalker Mar 19, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emwalker Mar 19, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emwalker Mar 19, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alice-i-cecile commented Mar 19, 2023

JMS55 commented Mar 19, 2023

alice-i-cecile commented Mar 19, 2023 • edited

mockersf commented Mar 21, 2023 • edited

mockersf commented Mar 21, 2023 • edited

emwalker commented Mar 21, 2023 • edited

emwalker commented Mar 21, 2023

mockersf commented Mar 21, 2023 • edited

emwalker commented Mar 21, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emwalker commented Mar 25, 2023 • edited

emwalker commented Mar 19, 2023 •

edited

emwalker Mar 19, 2023 •

edited

emwalker Mar 19, 2023 •

edited

emwalker Mar 19, 2023 •

edited

emwalker Mar 19, 2023 •

edited

emwalker Mar 19, 2023 •

edited

alice-i-cecile commented Mar 19, 2023 •

edited

mockersf commented Mar 21, 2023 •

edited

mockersf commented Mar 21, 2023 •

edited

emwalker commented Mar 21, 2023 •

edited

mockersf commented Mar 21, 2023 •

edited

emwalker commented Mar 21, 2023 •

edited

emwalker commented Mar 25, 2023 •

edited