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 icon to WindowDescriptor and Window #5488

Closed
wants to merge 9 commits into from
Closed

Add icon to WindowDescriptor and Window #5488

wants to merge 9 commits into from

Conversation

alt0173
Copy link

@alt0173 alt0173 commented Jul 29, 2022

Objective

Solution

  • A proxy struct WindowIcon that is equivalent to winit::window::Icon and convert it to an Icon when changing the icon. It is exposed to modification as mentioned above.

Notes

The icon's data is a Vec<u8> to match with winit. This feels sub-optimal to use without the image crate, but winit's icon uses specifically 8bit RGBA values, which would probably be very cumbersome to convert to.

I think it would be nice to have the default icon be the bevy logo, but I can't think of a way to do it without having a huge literal of bytes somewhere.

@alice-i-cecile alice-i-cecile added C-Enhancement A new feature A-Windowing Platform-agnostic interface layer to run your app in labels Jul 29, 2022
@alice-i-cecile
Copy link
Member

Awesome! Folks have been asking for this forever.


fn main() {
App::new()
.insert_resource(WindowDescriptor {
title: "I am a window!".to_string(),
icon: Some(WindowIcon::new(
Copy link
Member

Choose a reason for hiding this comment

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

Yeah this is way too low level for me to feel comfortable exposing directly to the user.

Are there reasonable choices for things to implement From for?

Copy link
Author

Choose a reason for hiding this comment

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

Would bevy::render::texture::Image be a good fit?

Copy link
Member

Choose a reason for hiding this comment

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

Ping @superdump for an informed opinion. That would be my natural first choice though.

/// Mirror of `Icon` from `winit`.
#[derive(Debug, Clone)]
pub struct WindowIcon {
/// 32-bit-per-pixel RGBA image data.
Copy link
Member

Choose a reason for hiding this comment

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

I am not keen on exposing something so raw, but I'm not sure it's worth the complexity to fix now.

Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

The main concern here is that a byte vec is a pretty gnarly API to expose. I'd like to see if we can make that nicer and more type-safe.

The obvious choice would be to reuse our various rendering types but a) I'm not sure we want to add that dependency and b) IIRC those rely on async asset loading to e.g. load an icon from a file.

@DJMcNab
Copy link
Member

DJMcNab commented Jul 29, 2022

A quick reading suggests that this is going to run into the same issue as the original (license locked so best not to look at the code I guess?):

#1163

@alice-i-cecile
Copy link
Member

Based on @cart's comments in #1163 I think we should a) re-add runtime icon modification b) use the image type c) support loading images via bevy_asset :) Sorry for not checking that before giving advice!

As far as I can tell it *needs* to be mutable for wasm to compile, but it gets block by CI due to being unused elsewhere, so this should actually fix it.
@alt0173
Copy link
Author

alt0173 commented Jul 30, 2022

Based on @cart's comments in #1163 I think we should a) re-add runtime icon modification b) use the image type c) support loading images via bevy_asset :) Sorry for not checking that before giving advice!

I'm working on this but I can't figure out how to use image (from bevy_render) in bevy_window due to a cyclical dependency issue, do you have any idea how I could work around that?

@alice-i-cecile
Copy link
Member

What will probably need to happen is a refactoring PR disentangling things. Let's wait to see what Rob and Cart think before attempting that (likely a couple of days), and then attempt that in a separate PR.

My instinct is that we could probably have rendering primitives like Image and Color in their own crate, and then rely on that for both bevy_render and bevy_window, which would solve the cycle. But I'm not a domain expert here.

@KirmesBude
Copy link
Contributor

Hi.
I still follow bevy development, but clearly I did not set up my github notifications correctly, so sorry for the radio silence on #2268.
In case it is still beneficial, I have relicensed my past contributions. (Is this comment enough? #2373 (comment)).

I was running into the same dependency issues, when trying to incorporate bevy_asset into the mix, so sadly I can not be of any help there. @alice-i-cecile suggestion is probably the way to go, because I do not remember trying to inverse the dependencies for bevy_render, bevy_window and bevy_camera going particularly well :D

@alice-i-cecile
Copy link
Member

In case it is still beneficial, I have relicensed my past contributions. (Is this comment enough? #2373 (comment)).

That's incredibly valuable, thanks!

@mnmaita mnmaita mentioned this pull request Aug 30, 2022
2 tasks
@alt0173 alt0173 closed this by deleting the head repository Sep 26, 2022
@cavivie
Copy link

cavivie commented Nov 6, 2022

Can we try to support the following code?

App::new()
    .insert_resource(WindowDescriptor {
        width: 288.,
        height: 512.,
        icon: include_bytes!("your_game_icon_path.ico"),
        ..default()
    })

@amfaber
Copy link

amfaber commented Sep 18, 2023

Any updates on this?

@mnmaita
Copy link
Member

mnmaita commented Oct 17, 2023

Any updates on this?

Looks like development of this feature was moved here #8130 but it still has some unresolved issues/discussions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Windowing Platform-agnostic interface layer to run your app in C-Enhancement A new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

A field in WindowDescriptor resource to set the window's icon ?
7 participants