Decouple `BackgroundColor` from `UiImage`

[benfrankel]

Objective

Fixes https://github.com/bevyengine/bevy/issues/11157.

Solution

Stop using BackgroundColor as a color tint for UiImage. Add a UiImage::color field for color tint instead. Allow a UI node to simultaneously include a solid-color background and an image, with the image rendered on top of the background (this is already how it works for e.g. text).

---

Changelog

• BackgroundColor no longer applies color tint to UI images, but adds a solid-color background behind the image instead.
• Added UiImage::color.
• Removed ButtonBundle::image.
• Updated RenderUiSystem variants.
• Renamed bevy_ui::extract_* functions for consistency.

Migration Guide

• BackgroundColor no longer applies color tint to UI images. Use UiImage::color instead.
• ButtonBundle::image has been removed. You can still include a UiImage component with your button entity in addition to the bundle.
• bevy_ui::extract_text_uinodes has been renamed to extract_uinode_text.
• bevy_ui::extract_uinodes has been split into bevy_ui::extract_uinode_background_colors and bevy_ui::extract_uinode_images.

[benfrankel]

I'm not sure the changelog / migration guides are written the way they should be / covering what they should.

Also, I removed Without<ContentSize> from the UI node query in extract_uinode_border so that I could add a border to the ui_texture_atlas example, but there may be a good reason for it that I'm not aware of (@ickshonpe ?).

[benfrankel]

Also I'm curious if there's a reason that ExtractedUiNodes::uinodes is a hash map with Entity key instead of just a Vec. The code isn't straightforward so I couldn't tell how the Entity gets used. Most of the extract_uinode_xyz systems just create a new entity with commands.spawn_empty().id() anyways, and now only extract_uinode_background uses the actual original UI node entity as a key.

[pablo-lua]

isn't the Entity used in bevy_ui\src\render\mod.rs by the Render phase? Btw, good changes, the examples probably will change too.

[ickshonpe]

I put in a similar PR a year ago https://github.com/bevyengine/bevy/pull/7451 but it got no reviews. I'm happy to close that though in favour of this one, as this one targets the latest bevy.

I think as well in my PR the colour was a field on the UiImage type, which isn't as flexible as a separate component and makes change detection awkward.

[pablo-lua]

I think as well in my PR the colour was a field on the UiImage type, which isn't as flexible as a separate component and makes change detection awkward.

We can probably solve that if we go with the ColorTint idea in the future

[ickshonpe]

I think as well in my PR the colour was a field on the UiImage type, which isn't as flexible as a separate component and makes change detection awkward.

We can probably solve that if we go with the ColorTint idea in the future

Ah yeah I wrote that before I started my review, I assumed we were going with a separate component for the color.

[ickshonpe]

I'm not sure the changelog / migration guides are written the way they should be / covering what they should.

Also, I removed Without<ContentSize> from the UI node query in extract_uinode_border so that I could add a border to the ui_texture_atlas example, but there may be a good reason for it that I'm not aware of (@ickshonpe ?).

ContentSize uses different layout rules and may not work correctly with borders in all cases, I can't remember though it's been a while since I tested it. I think the filter should remain for now.

You should be able to display atlas images with a border with a construction like this I think

 commands.spawn((
        NodeBundle {
            style: Style {
                border: UiRect::all(Val::Px(20.)),
                ..Default::default()
            },
            border_color: Color::RED.into(),
            ..Default::default()
        }, 
        UiTextureAtlasImage { index, ..Default::default() }, 
        texture_atlas_handle
    ));

Or you could use an Outline instead of a border.

[ickshonpe]

Also I'm curious if there's a reason that ExtractedUiNodes::uinodes is a hash map with Entity key instead of just a Vec. The code isn't straightforward so I couldn't tell how the Entity gets used. Most of the extract_uinode_xyz systems just create a new entity with commands.spawn_empty().id() anyways, and now only extract_uinode_background uses the actual original UI node entity as a key.

I made a couple of PRs improving UI extraction performance, but there were a lot of changes to rendering for Bevy 0.12, and not all of them have been merged yet so things are in kind of an inbetween state. Some of them were: #9212 #9668 #9889 #9853 #9877

[alice-i-cecile]

@benfrankel I'd like to review this and get it merged: can you resolve merge conflicts?

[benfrankel]

@alice-i-cecile done. Note that I effectively reverted https://github.com/bevyengine/bevy/pull/11205 because with background color and images being extracted separately, skipping loading images won't also skip their background color. I might have misunderstood the issue there though, in which case I can remove the skip again.

[alice-i-cecile]

@alice-i-cecile done. Note that I effectively reverted https://github.com/bevyengine/bevy/pull/11205 because with background color and images being extracted separately, skipping loading images won't also skip their background color. I might have misunderstood the issue there though, in which case I can remove the skip again.

Thanks! That's a question for @JMS55 and/or @mockersf.

[ManevilleF]

UiImage is starting to become very similar to Sprite

[benfrankel]

UiImage is starting to become very similar to Sprite

It's out of scope for this PR of course, but in my opinion Sprite should also have UiImage's texture: Handle<Image> field instead of reading a separate Handle<Image> that's awkwardly floating around as a non-newtyped component on the same entity. And then... maybe they should just be the same component.

[benfrankel]

Helping migration by removing BackgroundColor from the image bundles as part of this change makes sense. My only concern is that that leaves the image bundles inconsistent with TextBundle (and NodeBundle).

I guess that's territory for a later PR, though, for other inconsistencies as well:

• NodeBundle includes a BorderColor that defaults to transparent
• ButtonBundle includes a BorderColor that defaults to white
• TextBundle and image bundles don't include a BorderColor at all (because of taffy support, relevant: https://github.com/bevyengine/bevy/pull/10690)

^\_(``/)_/^

[ManevilleF]

You are absolutely right, we need to schedule a cleanup to fix inconsistent components in bundles, for this PR I think we can focus on iso-functionality