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

Add basic support for window icons #8130

Closed
wants to merge 18 commits into from
Closed
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions crates/bevy_render/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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" }
Copy link
Member

Choose a reason for hiding this comment

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

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

bevy_utils = { path = "../bevy_utils", version = "0.11.0-dev" }
bevy_tasks = { path = "../bevy_tasks", version = "0.11.0-dev" }

Expand Down Expand Up @@ -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 }
Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

Agreed, I think that's cleaner.

76 changes: 76 additions & 0 deletions crates/bevy_render/src/texture/icon.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
use crate::texture::Image;
use bevy_asset::{Assets, Handle};
use bevy_ecs::{
prelude::{Component, Entity, NonSendMut, Query, Res},
system::Commands,
};
use bevy_log::{error, info};
use bevy_winit::WinitWindows;
use winit::window::Icon;

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

Choose a reason for hiding this comment

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

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


/// Set or unset the window icon, depending on whether `Some(image_handle)` or `None` is provided.
///
/// # Example
/// ```rust,no_run
/// use bevy_app::{App, Startup, Update};
/// use bevy_asset::AssetServer;
/// use bevy_ecs::prelude::*;
/// use bevy_render::texture::{set_window_icon, WindowIcon};
///
/// fn setup(mut commands: Commands, asset_server: Res<AssetServer>) {
/// let icon_handle = asset_server.load("branding/icon.png");
/// commands.spawn(WindowIcon(Some(icon_handle)));
/// }
///
/// fn main() {
/// App::new()
/// .add_systems(Startup, setup)
/// .add_systems(Update, set_window_icon)
/// .run();
/// }
/// ```
///
/// This functionality [is only known to work on Windows and X11](https://docs.rs/winit/latest/winit/window/struct.Window.html#method.set_window_icon).
pub fn set_window_icon(
images: Res<Assets<Image>>,
mut commands: Commands,
mut query: Query<(Entity, &mut WindowIcon)>,
mut winit_windows: NonSendMut<WinitWindows>,
) {
for (entity, window_icon) in query.iter_mut() {
Copy link
Member

Choose a reason for hiding this comment

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

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);
  } 
}

Copy link
Author

Choose a reason for hiding this comment

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

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.

let icon = {
if let Some(image) = &window_icon.0 {
let Some(icon) = images.get(image) else { continue };
let result: Result<Icon, _> = icon.clone().try_into();

match result {
Ok(icon) => Some(icon),
Err(err) => {
error!("failed to set window icon: {}", err);
commands.entity(entity).remove::<WindowIcon>();
continue;
}
}
} else {
None
}
};

if let Some(icon) = &icon {
for (_id, window) in &mut winit_windows.windows {
window.set_window_icon(Some(icon.clone()));
}
} else {
for (_id, window) in &mut winit_windows.windows {
window.set_window_icon(None);
}
}

info!("window icon set");
commands.entity(entity).remove::<WindowIcon>();
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Author

@emwalker emwalker Mar 19, 2023

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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

}
}
Copy link
Author

@emwalker emwalker Mar 19, 2023

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

19 changes: 19 additions & 0 deletions crates/bevy_render/src/texture/image.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,13 @@ use crate::{
renderer::{RenderDevice, RenderQueue},
texture::BevyDefault,
};
use anyhow::anyhow;
use bevy_asset::HandleUntyped;
use bevy_derive::{Deref, DerefMut};
use bevy_ecs::system::{lifetimeless::SRes, Resource, SystemParamItem};
use bevy_math::Vec2;
use bevy_reflect::{FromReflect, Reflect, TypeUuid};
use winit::window::Icon;

use std::hash::Hash;
use thiserror::Error;
Expand Down Expand Up @@ -613,6 +615,23 @@ impl CompressedImageFormats {
}
}

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

@emwalker emwalker Mar 19, 2023

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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


fn try_into(self) -> Result<Icon, Self::Error> {
let Ok(icon) = self.try_into_dynamic() else {
return Err(anyhow!("failed to convert Image to DynamicImage"));
};

let width = icon.width();
let height = icon.height();
let data = icon.into_rgba8().into_raw();
Icon::from_rgba(data, width, height)
.map_err(|err| anyhow!("failed to convert image to winit::window::Icon: {}", err))
}
}

#[cfg(test)]
mod test {

Expand Down
2 changes: 2 additions & 0 deletions crates/bevy_render/src/texture/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ mod exr_texture_loader;
mod fallback_image;
#[cfg(feature = "hdr")]
mod hdr_texture_loader;
pub mod icon;
#[allow(clippy::module_inception)]
mod image;
mod image_texture_loader;
Expand All @@ -27,6 +28,7 @@ pub use exr_texture_loader::*;
pub use hdr_texture_loader::*;

pub use fallback_image::*;
pub use icon::*;
pub use image_texture_loader::*;
pub use texture_cache::*;

Expand Down
9 changes: 9 additions & 0 deletions examples/window/window_settings.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
use bevy::{
diagnostic::{FrameTimeDiagnosticsPlugin, LogDiagnosticsPlugin},
prelude::*,
render::texture::{set_window_icon, WindowIcon},
window::{CursorGrabMode, PresentMode, WindowLevel},
};

Expand All @@ -24,9 +25,11 @@ fn main() {
}))
.add_plugin(LogDiagnosticsPlugin::default())
.add_plugin(FrameTimeDiagnosticsPlugin)
.add_systems(Startup, setup)
.add_systems(
Update,
(
set_window_icon,
change_title,
toggle_cursor,
toggle_vsync,
Expand All @@ -37,6 +40,12 @@ fn main() {
.run();
}

/// Add an icon to the window task bar. Only works in Windows and Linux.
fn setup(mut commands: Commands, asset_server: Res<AssetServer>) {
let icon_handle = asset_server.load("branding/icon.png");
commands.spawn(WindowIcon(Some(icon_handle)));
}
Copy link
Author

@emwalker emwalker Mar 19, 2023

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, let's keep this simple here.

Copy link
Author

@emwalker emwalker Mar 19, 2023

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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.


/// This system toggles the vsync mode when pressing the button V.
/// You'll see fps increase displayed in the console.
fn toggle_vsync(input: Res<Input<KeyCode>>, mut windows: Query<&mut Window>) {
Expand Down