Do not require a BackgroundColor for UI images #11157

pablo-lua · 2023-12-31T00:16:39Z

What problem does this solve or what need does it fill?

Actually, in order to use an image with Sprite or even UiImage, you are forced to use a BackgroundColor that is different from transparent, and that is misleading, can lead to one to think "well, I don't want an color in my Image, so I'll set it to transparent, right?" and then being confused because the image disappeared

What solution would you like?

The BackgroundColor, when being used with an image would apply an oppacity of determined color above the image. For example Rgb(255, 255, 0, 0.5) would taint the image slightly of yellow
but if one use Rgb(0, 0, 0, 1) the image would be tainted of black and the image itself would disappear (as alpha is 100%), leaving an black square on its place

What alternative(s) have you considered?

For that purpose, probably adding an child or something above the child with the desired color and leaving color in the background as Color::WHITE and leaving it as it is.

Additional context

A possible problem that would appear from this is if one wants to make a image appear only a little, with alpha set to for example 0.5, that wouldn't be possible for that approach, and a possible solution to this is adding another component that specifies the Image oppacity

The text was updated successfully, but these errors were encountered:

alice-i-cecile · 2023-12-31T00:20:16Z

The current design is quite confusing to me, as it conflates the background color with the tint of the image. IMO the node that stores the UI shouldn't have a background color by default: instead, it should have a distinct component / property that is used for tinting.

benfrankel · 2023-12-31T22:30:20Z

It would be nice to have a ColorTint component that applies color tint to an entire entity, including its Sprite, TextureAtlasSprite, Text, UiImage, etc. That would make certain animations easy to implement, like a fade away animation by tweening ColorTint's alpha from 1 to 0. The same animation code would automatically work for any visible entity.

The individual components probably still need their own color fields though, because you might want different colors for different aspects of the same entity. On that note, UiImage and UiTextureAtlasImage should probably have their own color fields too, which is what would replace the overloaded use of BackgroundColor in this issue.

pablo-lua · 2023-12-31T22:51:40Z

the ColorTint idea is good, like you said. would take the old function of BackgroundColor and would give the BackgroundColor another function that I hope would be more intuitive. Like I said in the issue, like applying an oppacity over the Image or something like that. In that case, the Color role in various bundles would change (Sprite would now have ColorTint and use the Color for another purpose)

benfrankel · 2023-12-31T22:57:21Z

Btw, BackgroundColor doesn't affect Sprites currently, it only tints UiImage and UiTextureAtlasImage (and for other UI nodes, it's the actual background color).

pablo-lua · 2023-12-31T23:10:39Z

Yeah, in the Sprite case I'm talking about the Color element, In the Sprite case and maybe others, the Color do the role of tint the image, what IMO is a little misleading too.

benfrankel · 2023-12-31T23:33:56Z

I'm gonna try to make a PR decoupling BackgroundColor from UiImage and UiTextureAtlasImage.

ColorTint can be a separate issue and a separate PR.

benfrankel · 2023-12-31T23:44:32Z

I guess unrelated to this issue, but I noticed another inconsistency. The Sprite, TextureAtlasSprite, and UiTextureAtlasImage components all rely on a separate component for their image handles (Handle<Image> or Handle<TextureAtlas>). But UiImage for some reason has its own field, UiImage::texture: Handle<Image>.

I'm not sure if there's a reason for this so I'm not gonna touch it.

ickshonpe · 2024-01-01T11:29:07Z

The current design is quite confusing to me, as it conflates the background color with the tint of the image. IMO the node that stores the UI shouldn't have a background color by default: instead, it should have a distinct component / property that is used for tinting.

I think I put in a similar PR back for 0.10 or something that implemented this. Everyone agreed with it but it got no reviews and I forgot about it.

It was #7451

ickshonpe · 2024-01-01T11:43:47Z

Another reason I wanted this change was that it allowed for things like letter-boxing of images with a fixed aspect ratio.
I had a second PR I made for it, but turned it into a third-party crate instead because the color changes didn't get merged.

ickshonpe · 2024-01-01T11:50:55Z

I guess unrelated to this issue, but I noticed another inconsistency. The Sprite, TextureAtlasSprite, and UiTextureAtlasImage components all rely on a separate component for their image handles (Handle<Image> or Handle<TextureAtlas>). But UiImage for some reason has its own field, UiImage::texture: Handle<Image>.

I'm not sure if there's a reason for this so I'm not gonna touch it.

This was because UiImage also had fields for flipping the image on its axes. This isn't need for sprites as you can just use the transform.

For UiTextureAtlasImage it needs two components, the texture atlas and also an index into the texture atlas, so the implementation was different.

pablo-lua · 2024-01-01T11:56:49Z

This was because UiImage also had fields for flipping the image on its axes. This isn't need for sprites as you can just use the transform.

This is only an idea, but maybe we can do this kind of transform on the image using style?

ickshonpe · 2024-01-01T12:57:22Z

This was because UiImage also had fields for flipping the image on its axes. This isn't need for sprites as you can just use the transform.

This is only an idea, but maybe we can do this kind of transform on the image using style?

I've thought that the Style component is misnamed for a long time. It's meant to be content agnostic, purely for use by Taffy to generate the layout geometry. It wouldn't make sense to add image orientation information to it.

benfrankel · 2024-01-01T18:59:07Z

This was because UiImage also had fields for flipping the image on its axes. This isn't need for sprites as you can just use the transform.

Sprite, TextureAtlasSprite, UiImage, and UiTextureAtlasImage all have flip_x and flip_y fields.

ickshonpe · 2024-01-01T19:12:27Z

This was because UiImage also had fields for flipping the image on its axes. This isn't need for sprites as you can just use the transform.

Sprite, TextureAtlasSprite, UiImage, and UiTextureAtlasImage all have flip_x and flip_y fields.

Oh yeah of course they do, I forgot 😅 .

# 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). ![2024-02-29_1709239666_563x520](https://github.com/bevyengine/bevy/assets/12173779/ec50c9ef-4c7f-4ab8-a457-d086ce5b3425) --- ## 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 `bevy_ui::extract_uinode_background_colors` and `bevy_ui::extract_uinode_images`. - `bevy_ui::extract_text_uinodes` has been renamed to `extract_uinode_text`.

# Objective Fixes bevyengine#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). ![2024-02-29_1709239666_563x520](https://github.com/bevyengine/bevy/assets/12173779/ec50c9ef-4c7f-4ab8-a457-d086ce5b3425) --- ## 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 `bevy_ui::extract_uinode_background_colors` and `bevy_ui::extract_uinode_images`. - `bevy_ui::extract_text_uinodes` has been renamed to `extract_uinode_text`.

pablo-lua added C-Feature A new feature, making something new possible S-Needs-Triage This issue needs to be labelled labels Dec 31, 2023

alice-i-cecile changed the title ~~Let images be showed without using BackgroundColor~~ Do not require a BackgroundColor for UI images Dec 31, 2023

benfrankel mentioned this issue Jan 1, 2024

Decouple BackgroundColor from UiImage #11165

Merged

alice-i-cecile closed this as completed in #11165 Mar 3, 2024

benfrankel mentioned this issue Sep 15, 2024

Add PropagateOpacity for controlling the alpha of entire UI node trees #15206

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not require a BackgroundColor for UI images #11157

Do not require a BackgroundColor for UI images #11157

pablo-lua commented Dec 31, 2023

alice-i-cecile commented Dec 31, 2023

benfrankel commented Dec 31, 2023 •

edited

Loading

pablo-lua commented Dec 31, 2023

benfrankel commented Dec 31, 2023 •

edited

Loading

pablo-lua commented Dec 31, 2023

benfrankel commented Dec 31, 2023

benfrankel commented Dec 31, 2023 •

edited

Loading

ickshonpe commented Jan 1, 2024 •

edited

Loading

ickshonpe commented Jan 1, 2024

ickshonpe commented Jan 1, 2024

pablo-lua commented Jan 1, 2024

ickshonpe commented Jan 1, 2024 •

edited

Loading

benfrankel commented Jan 1, 2024

ickshonpe commented Jan 1, 2024

Do not require a BackgroundColor for UI images #11157

Do not require a BackgroundColor for UI images #11157

Comments

pablo-lua commented Dec 31, 2023

What problem does this solve or what need does it fill?

What solution would you like?

What alternative(s) have you considered?

Additional context

alice-i-cecile commented Dec 31, 2023

benfrankel commented Dec 31, 2023 • edited Loading

pablo-lua commented Dec 31, 2023

benfrankel commented Dec 31, 2023 • edited Loading

pablo-lua commented Dec 31, 2023

benfrankel commented Dec 31, 2023

benfrankel commented Dec 31, 2023 • edited Loading

ickshonpe commented Jan 1, 2024 • edited Loading

ickshonpe commented Jan 1, 2024

ickshonpe commented Jan 1, 2024

pablo-lua commented Jan 1, 2024

ickshonpe commented Jan 1, 2024 • edited Loading

benfrankel commented Jan 1, 2024

ickshonpe commented Jan 1, 2024

benfrankel commented Dec 31, 2023 •

edited

Loading

benfrankel commented Dec 31, 2023 •

edited

Loading

benfrankel commented Dec 31, 2023 •

edited

Loading

ickshonpe commented Jan 1, 2024 •

edited

Loading

ickshonpe commented Jan 1, 2024 •

edited

Loading