Feature/issue 1031 window icon

[KirmesBude]

Adds the ability to set the window icon at runtime.
resolves #1031
based on feedback from this PR: #1163

window_icon.webm.mp4

Notes: since there was some confusion, winit does support changing the window icon at runtime.

Outstanding work:

• Feature Gating for non X11/Wayland/Windows? It is already feature gated on winit size, so I think it is fine to keep it like this on bevy side.
• Document how to work with window icons; especially Wayland

[NathanSWard]

I really appreciate this feature request, and other than some of my initial comments, this looks good so far :)

[KirmesBude]

Thanks for the quick review and the nice comments. Makes you really feel welcomed :).

About the whole Handle thing:
I understand why cart was pushing for it in the old PR. Using bevy_asset to handle the icon loading just makes sense.
But I am not sure, if this is the best solution.
In 99% of the cases, people will want to set the icon once, at the very start in the WindowDescriptor and as far as I understand at that point you have no way to get a Handle. That means you will have to manually load the asset and set the icon in you setup system.
Actually changing the icon at runtime (where bevy_asset is the most logical solution) I would assume is not something most people will do.

Ideally we would find a way to do both with Handle, but a simple (data,width,height) struct might be the most generic solution, that would also allow someone to include_bytes the icon.

[alice-i-cecile]

I agree with your assessment on use cases; I would in fact be mildly alarmed if my icon changed at runtime!

I personally think that synchronous, blocking image loading may be the correct choice here because it maps onto the use cases better and avoids the circular dependencies.

[NathanSWard]

Actually changing the icon at runtime (where bevy_asset is the most logical solution) I would assume is not something most people will do.

This is true, however just because most people may not want this functionality, does that me we should remove the possibility?

Ideally we would find a way to do both with Handle, but a simple (data,width,height) struct might be the most generic solution, that would also allow someone to include_bytes the icon.

I'm not the biggest fan of this approach as it's very easy to mess up just using Vec<u8> as the pixel data. And as a user, it's not intuitive on how to actually generate that data.

In 99% of the cases, people will want to set the icon once, at the very start in the WindowDescriptor and as far as I understand at that point you have no way to get a Handle. That means you will have to manually load the asset and set the icon in you setup system.

Yes this is the tricky part. I'm also in agreement with cart that we should use bevy_asset (e.g. the AssetServer to accomplish this). However, I also agree that it would be nice for this to be set in the WindowDescriptor... so I guess I'm a little torn.

Some possible paths forward:

• Add an Option<Handle<Texture>> field to the WindowDescriptor so it is possible to set it on startup. This way someone could manually creates an asset (not via the AssetServer).
• Allow setting/changing the icon at runtime. That way someone can utilize the AssetServer to create a handle to the appropriate texture and then set the icon in a startup system.
• Perhaps have the WindowDescriptor have an Option<PathBuf> so that we can specify what file/asset we want the icon to be, and then inside our window logic, try to load an asset that is pointed to by the PathBuf. This way we can still utilize the AssetServer and the user can set the icon at startup.

[cart]

Hmm yeah theres a bit more nuance here than I initially thought. Heres my current take:

1. Expose a set_icon_bytes(bytes: Vec<u8>) operation and set_icon_path(path: &Path) on window
2. Add a icon_path: Option<PathBuf> to WindowDescriptor and Window (but don't do anything to load it in bevy_window or bevy_winit as that violates the dependency tree). WindowDescriptor is only for queuing up new windows, not for polling runtime state.
3. In bevy_render, add a system that: detects new windows and queued SetIconPath operations, grabs their icon_paths, triggers loads, stores WindowId->Handle<Texture> maps for windows waiting on loads (holding the handle ensures it isnt dropped), and polls load status of each texture in that list until it either succeeds or fails. On success, it looks up the Texture and calls window.set_icon_bytes(), then removes the WindowId->Handle<Texture> mapping from the list, which drops the texture handle (which triggers a drop of the texture).

I do think supporting Handle<Texture> would be nice, but I think its cleaner to keep bevy_render a level above bevy_window (and this also allows bevy_window to be used without bevy_asset, which is an added benefit).

Ultimately (3) could be simplified once we add "async/await on AssetServer in systems", but that will take some time :)

[NathanSWard]

I really like where this implementation is going, it's starting to look quite a bit better 😄

In response to my comments I left above, here are some viable solutions:

An icon can either be a:

• PathBuf
• Vec<u8> + UVec2

When it's set as a PathBuf you don't add it to the queue yet. You simply store the path and then have a separate system (similar to your window_icon_changed function which will load the texture asynchronously, then when it's loaded, queue it up and send the bytes to winit.

When it's the bytes directly, you can just add it directly to the queue.

So I would personally have the SetIcon command to only be able to hold the bytes directly, and never hold path :)

Let me know if this makes sense or feel free to ask more questions!
I'm also always available on the discord if you want to message me there!!

[KirmesBude]

I really like where this implementation is going, it's starting to look quite a bit better smile

In response to my comments I left above, here are some viable solutions:

An icon can either be a:

* `PathBuf`

* `Vec<u8>` + `UVec2`

Yes, makes sense.

When it's set as a PathBuf you don't add it to the queue yet. You simply store the path and then have a separate system (similar to your window_icon_changed function which will load the texture asynchronously, then when it's loaded, queue it up and send the bytes to winit.

What if someone does:

set_icon(path); 
...(later) 
set_icon(path);

With the same path.
So far I have been operating on the understanding that I should not load anything again if the path did not change, but maybe that is not a scenario I should consider?
-> User-Error :D?
Edit; Actually nevermind. If I do not know if the path changes then I would need to load everytime the system runs.
I need some has_icon_changed indicator that is true once (according to the system).
I tried this by also storing the path in the system local map, but it did not look very nice and makes dropping the handle more "difficult".

When it's the bytes directly, you can just add it directly to the queue.

So I would personally have the SetIcon command to only be able to hold the bytes directly, and never hold path :)

Since bevy_winit can not do anything with SetIcon if it has a path I agree.
Should only have bytes.

Let me know if this makes sense or feel free to ask more questions!
I'm also always available on the discord if you want to message me there!!

[KirmesBude]

I implemented an alternative version just using window.icon().
I still busy check if a change has happened, but that was true in the previous version as well.

The upside: We do not need to expose command queue anymore and it should not matter when the icon_changed system in bevy_render runs anymore.
The downside: The system is noticeably more noisy. It is probably possible to condense it somehow, but I am not yet sure how.

[NathanSWard]

Something to consider with this system, is that it gets executed each frame.
I'm curious if we can find some fun way to only have it execute when a window's icon is updated, or when the asset server has loaded it's value.

Probably not, but it's something to think about 😅

[KirmesBude]

Hmm, yes.
We could do a roundabout way via a command to bevy_winit and an event from there, but I dont think we should involve bevy_winit for that.

[KirmesBude]

Sorry for the long absence.
What should I do to get this PR mergable?

@NathanSWard btw. Not sure who you are on discord. I assume "Bakuta", but I could not find a link between that account and you github account :D

[NathanSWard]

@NathanSWard btw. Not sure who you are on discord. I assume "Bakuta", but I could not find a link between that account and you github account :D

Yep, that's me :) feel free to ping me there whenever!
I think my nickname is Nathan [🍞👑] on the server also.

[KirmesBude]

I think this is done now. Happy to do any necessary changes though.

[TheRawMeatball]

Hmm, this mostly looks good but I'm not sure whether we should couple this to bevy_render - it feels like this'd be more at home in bevy_winit. Though that'd mean we can't use bevy_render textures for window icons, but perhaps we shouldn't be doing that anyway.

[KirmesBude]

I do think it is worthwhile to question the approach.
In #1163 a simpler solution was implemented, similar to using WindowIconBytes directly for this solution. But there was demand to make it work within the bevy_asset "ecosystem".

In fact this PR was initially only covering a similar usecase of the old PR and only afterwards I was trying to make it work with Assets. As you have said using Texure was not possible because of cyclic depencencies, so cart came up with the idea that is currently implemented here. (With the downside that my system is checking for icon changes every loop; I dont think that was his intention :D)

I guess it might be possible to remove the bevy_window dependency from bevy_render, but I am not sure that the motivation for all this work is justified (just to make icons works with textures :D).

You mention not using textures - but does that not mean not using bevy_asset?

[lukors]

What's needed for this to be merged? I could use this feature.

[aevyrie]

bevy/crates/bevy_winit/src/lib.rs

Line 47 in cba2b2e

.add_system_to_stage(CoreStage::PostUpdate, change_window.exclusive_system());

Is exclusive_system() always guaranteed to run on the main thread (now and in the future)?

If we can't always guarantee this, I'd suggest we add an assert to the change_window system. I just spent an inordinate amount of time trying to figure out why a Bevy app was hanging on startup intermittently. Turns out it was a system calling set_window_icon() running in the compute task pool instead of the main thread.

While this isn't an issue specific to this PR - in fact this PR will help prevent this mistake - I'd like to be doubly sure no one else needs to debug this issue in the future😆. An assert that is almost never triggered but can panic, is way better than mysterious intermittent application hangs.

[alice-i-cecile]

Is exclusive_system() always guaranteed to run on the main thread (now and in the future)?

Now yes, in the future maybe. See #3973 for my thoughts on how we could evolve the API. TL;DR: I think we need to split "main thread" from "not send and sync", where the former is a special case of the latter.

I'd suggest we add an assert to the change_window system

I approve of this!

[aevyrie]

Now yes, in the future maybe.

If #4027 is merged, it would resolve this issue because it would force the change_window() system to use

world.get_non_send::<WinitWindows>().unwrap();

instead of

world.get_resource::<WinitWindows>().unwrap();

[janus-culsans]

This seems done? There are some conflicts, but I can't seem to find any "github conflict ui" here anymore.

[alice-i-cecile]

@janus-culsans @lukors this PR seems relatively abandoned (sorry for the long limbo); if one of you are interested, pulling up a new rebased PR with a link to this thread and credit to @KirmesBude would help move this along.

[janus-culsans]

Yes, I would be interested. Working on rebasing.

[mockersf]

This pr is from before license change so it can't be used as a basis... you would pretty much have to not look at its code to redo it

[janus-culsans]

I can't even merge their branch into a new fork with attribution, even though it was MIT before?

Now that's funny.

[mockersf]

yes, the repo is now dual licensed MIT / Apache 2, so merging this PR would change its license without the author authorisation.

It's 238 lines, so probably not completely trivial, so it would need to be recoded without knowledge of the initial PR

[alice-i-cecile]

Closing this out; I don't think we're getting a relicense at this point and it's better to avoid misleading folks.

[janus-culsans]

Would it make sense to ask for relicense in all S-Pre-Relicense issues now, to avoid this problem from happening again and to give some time to the initial contributors?

[mockersf]

I think all PR authors were pinged several time at the relicensing time... you could try pinging them again, maybe they are active again after a few months of inactivity.

The issue to comment on is #2373

[mnmaita]

Based on the following comment, is anyone able to reopen this PR now @mockersf @alice-i-cecile ?

[alice-i-cecile]

I can reopen it now that the licensing is cleared up, but I'd prefer #5488 as we'll likely need a bit more back-and-forth :)