Decouple BackgroundColor from UiImage

[benfrankel]

Objective

Fixes #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

• The BackgroundColor component now renders a solid-color background behind UiImage instead of tinting its color.
• Removed BackgroundColor from ImageBundle, AtlasImageBundle, and ButtonBundle.
• Added UiImage::color.
• Expanded RenderUiSystem variants.
• Renamed bevy_ui::extract_text_uinodes to extract_uinodes_text for consistency.

Migration Guide

• BackgroundColor no longer tints the color of UI images. Use UiImage::color for that instead.
• For solid color buttons, replace ButtonBundle { background_color: my_color.into(), ... } with ButtonBundle { image: UiImage::default().with_color(my_color), ... }, and update button interaction systems to use UiImage::color instead of BackgroundColor as well.
• bevy_ui::RenderUiSystem::ExtractNode has been split into ExtractBackgrounds, ExtractImages, ExtractBorders, and ExtractText.
• bevy_ui::extract_uinodes has been split into extract_uinode_background_colors and extract_uinode_images.
• bevy_ui::extract_text_uinodes has been renamed to extract_uinode_text.

[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 #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 #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 #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]

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

[pablo-lua]

I think this is a good update and the cleanup can be done later on. All good