Make default behavior for `BackgroundColor` and `BorderColor` more intuitive by alice-i-cecile · Pull Request #14017 · bevyengine/bevy

alice-i-cecile · 2024-06-25T18:51:07Z

Objective

In Bevy 0.13, BackgroundColor simply tinted the image of any UiImage. This was confusing: in every other case (e.g. Text), this added a solid square behind the element. #11165 changed this, but removed BackgroundColor from ImageBundle to avoid confusion, since the semantic meaning had changed.

However, this resulted in a serious UX downgrade / inconsistency, as this behavior was no longer part of the bundle (unlike for TextBundle or NodeBundle), leaving users with a relatively frustrating upgrade path.

Additionally, adding both BackgroundColor and UiImage resulted in a bizarre effect, where the background color was seemingly ignored as it was covered by a solid white placeholder image.

Fixes #13969.

Solution

Per @viridia's design:

if you don't specify a background color, it's transparent.

if you don't specify an image color, it's white (because it's a multiplier).

if you don't specify an image, no image is drawn.

if you specify both a background color and an image color, they are independent.

the background color is drawn behind the image (in whatever pixels are transparent)

As laid out by @benfrankel, this involves:

Changing the default UiImage to use a transparent texture but a pure white tint.
Adding UiImage::solid_color to quickly set placeholder images.
Changing the default BorderColor and BackgroundColor to transparent.
Removing the default overrides for these values in the other assorted UI bundles.
Adding BackgroundColor back to ImageBundle and ButtonBundle.
Adding a 1x1 Image::transparent, which can be accessed from Assets<Image> via the TRANSPARENT_IMAGE_HANDLE constant.

Huge thanks to everyone who helped out with the design in the linked issue and the Discord thread: this was very much a joint design.

@cart helped me figure out how to set the UiImage's default texture to a transparent 1x1 image, which is a much nicer fix.

Testing

I've checked the examples modified by this PR, and the ui example as well just to be sure.

Migration Guide

BackgroundColor no longer tints the color of images in ImageBundle or ButtonBundle. Set UiImage::color to tint images instead.
The default texture for UiImage is now a transparent white square. Use UiImage::solid_color to quickly draw debug images.
The default value for BackgroundColor and BorderColor is now transparent. Set the color to white manually to return to previous behavior.

crates/bevy_ui/src/ui_node.rs

eidloi

works for me!

benfrankel

The examples that were updated to use UiImage::color as a solid background for buttons should be reverted back to BackgroundColor to be more idiomatic. This isn't blocking 0.14 release though, so it can be a separate PR.

crates/bevy_ui/src/ui_node.rs

pablo-lua

I think we can follow up with that to the next release, LGTM

benfrankel

Small nits. Approved other than that.

crates/bevy_render/src/texture/mod.rs

crates/bevy_ui/src/ui_node.rs

eidloi · 2024-06-25T21:27:02Z

crates/bevy_render/src/texture/mod.rs

+///
+/// Like [`Handle<Image>::default`], this is a handle to a fallaback image asset.
+/// While that handle points to an opaque white 1 x 1 image, this handle points to a 1 x 1 transparent white image.
+// Number randomly selected by fair WolframAlpha query. Totally arbitrary.


better than a fair dice roll (xkcd)!

eidloi

Yeah this is a lot more elegant! Thanks!

eidloi · 2024-06-25T21:27:54Z

crates/bevy_render/src/texture/image.rs

+    pub fn transparent() -> Image {
+        // We rely on the default texture format being RGBA8UnormSrgb
+        // when constructing a transparent color from bytes.
+        // If this changes, this function will need to be updated.


How will we find out? Will it fail to compile?

This will panic when the examples are run :)

crates/bevy_ui/src/ui_node.rs

@viridia

…tuitive (#14017) # Objective In Bevy 0.13, `BackgroundColor` simply tinted the image of any `UiImage`. This was confusing: in every other case (e.g. Text), this added a solid square behind the element. #11165 changed this, but removed `BackgroundColor` from `ImageBundle` to avoid confusion, since the semantic meaning had changed. However, this resulted in a serious UX downgrade / inconsistency, as this behavior was no longer part of the bundle (unlike for `TextBundle` or `NodeBundle`), leaving users with a relatively frustrating upgrade path. Additionally, adding both `BackgroundColor` and `UiImage` resulted in a bizarre effect, where the background color was seemingly ignored as it was covered by a solid white placeholder image. Fixes #13969. ## Solution Per @viridia's design: > - if you don't specify a background color, it's transparent. > - if you don't specify an image color, it's white (because it's a multiplier). > - if you don't specify an image, no image is drawn. > - if you specify both a background color and an image color, they are independent. > - the background color is drawn behind the image (in whatever pixels are transparent) As laid out by @benfrankel, this involves: 1. Changing the default `UiImage` to use a transparent texture but a pure white tint. 2. Adding `UiImage::solid_color` to quickly set placeholder images. 3. Changing the default `BorderColor` and `BackgroundColor` to transparent. 4. Removing the default overrides for these values in the other assorted UI bundles. 5. Adding `BackgroundColor` back to `ImageBundle` and `ButtonBundle`. 6. Adding a 1x1 `Image::transparent`, which can be accessed from `Assets<Image>` via the `TRANSPARENT_IMAGE_HANDLE` constant. Huge thanks to everyone who helped out with the design in the linked issue and [the Discord thread](https://discord.com/channels/691052431525675048/1255209923890118697/1255209999278280844): this was very much a joint design. @cart helped me figure out how to set the UiImage's default texture to a transparent 1x1 image, which is a much nicer fix. ## Testing I've checked the examples modified by this PR, and the `ui` example as well just to be sure. ## Migration Guide - `BackgroundColor` no longer tints the color of images in `ImageBundle` or `ButtonBundle`. Set `UiImage::color` to tint images instead. - The default texture for `UiImage` is now a transparent white square. Use `UiImage::solid_color` to quickly draw debug images. - The default value for `BackgroundColor` and `BorderColor` is now transparent. Set the color to white manually to return to previous behavior.

…14033) # Objective - #14017 changed how `UiImage` and `BackgroundColor` work - one change was missed in example `color_grading`, another in the mobile example ## Solution - Change it in the examples

lufog

I guess it was missed

bevy/examples/ui/button.rs

Line 32 in 336fddb

image.color = PRESSED_BUTTON;

bevy/examples/ui/button.rs

Line 37 in 336fddb

image.color = HOVERED_BUTTON;

bevy/examples/ui/button.rs

Line 42 in 336fddb

image.color = NORMAL_BUTTON;

# Objective - After #14017 , I noticed that the drawcall increased 10x in the `many_buttons`, causing the `UIPassNode `to increase from 1.5ms to 6ms. This is because our UI batching is very fragile. ## Solution - skip extract UiImage when its texture is default ## Performance many_buttons UiPassNode ![image](https://github.com/bevyengine/bevy/assets/45868716/9295d958-8c3f-469c-a7e0-d1e90db4dfb7)

# Objective - After bevyengine#14017 , I noticed that the drawcall increased 10x in the `many_buttons`, causing the `UIPassNode `to increase from 1.5ms to 6ms. This is because our UI batching is very fragile. ## Solution - skip extract UiImage when its texture is default ## Performance many_buttons UiPassNode ![image](https://github.com/bevyengine/bevy/assets/45868716/9295d958-8c3f-469c-a7e0-d1e90db4dfb7)

- Fixed an error where the application failed to load a CSS asset, resulting in the error: `tomt_bevycss::system: Failed to load stylesheet from handle StrongHandle<StyleSheetAsset>{ id: Index(AssetIndex { generation: 0, index: 0 }), path: Some(sheets/index.css) }`. The issue was related to the loading order of systems. Moving the entire schedule right after the update phase resolved the problem. - Make hot reload enabled when Bevy has the file_watcher feature enabled. - Updated the alpha demo: due to commit bevyengine/bevy#14017, an image can now have a BackgroundColor. The fix ensures alpha is adjusted based on the component combination: if an entity has a UIImage component, the alpha is set on it; if not, the BackgroundColor component is updated.

alice-i-cecile added 7 commits June 25, 2024 13:40

Use NONE as the default UI image color

21e90c8

Default to transparent white instead

09c1b1e

Refine API

6f0c3dd

Make BackgroundColor transparent by default

a18c5e3

Make BorderColor transparent by default

5811cbe

Don't override default BorderColor and BackgroundColor

305f057

Add BackgroundColor to ImageBundle

c887fe9

alice-i-cecile requested review from pablo-lua and viridia June 25, 2024 18:54

pablo-lua reviewed Jun 25, 2024

View reviewed changes

crates/bevy_ui/src/ui_node.rs Outdated Show resolved Hide resolved

crates/bevy_ui/src/ui_node.rs Outdated Show resolved Hide resolved

eidloi approved these changes Jun 25, 2024

View reviewed changes

benfrankel reviewed Jun 25, 2024

View reviewed changes

crates/bevy_ui/src/ui_node.rs Outdated Show resolved Hide resolved

alice-i-cecile added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward S-Needs-Testing Testing must be done before this is safe to merge labels Jun 25, 2024

Strengthen warning

df678bf

alice-i-cecile added the X-Contentious There are nontrivial implications that should be thought through label Jun 25, 2024

alice-i-cecile added 2 commits June 25, 2024 16:06

Add BackgroundColor to ButtonBundle

9fd21e3

Don't use UiImage to set background colors in examples

63d2654

alice-i-cecile added S-Needs-Review Needs reviewer attention (from anyone!) to move forward and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Jun 25, 2024

Balance in all things, especially backticks

d62529e

alice-i-cecile requested review from benfrankel and pablo-lua June 25, 2024 20:30

pablo-lua approved these changes Jun 25, 2024

View reviewed changes

Use a default transparent image

d308fb7

alice-i-cecile added the A-Rendering Drawing game state to the screen label Jun 25, 2024

alice-i-cecile requested review from eidloi and pablo-lua June 25, 2024 21:20

benfrankel suggested changes Jun 25, 2024

View reviewed changes

crates/bevy_render/src/texture/mod.rs Outdated Show resolved Hide resolved

crates/bevy_render/src/texture/mod.rs Outdated Show resolved Hide resolved

crates/bevy_ui/src/ui_node.rs Outdated Show resolved Hide resolved

eidloi reviewed Jun 25, 2024

View reviewed changes

eidloi approved these changes Jun 25, 2024

View reviewed changes

alice-i-cecile commented Jun 25, 2024

View reviewed changes

crates/bevy_ui/src/ui_node.rs Outdated Show resolved Hide resolved

Apply suggestions from code review

d2b1266

alice-i-cecile enabled auto-merge June 25, 2024 21:42

alice-i-cecile added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jun 25, 2024

alice-i-cecile added this pull request to the merge queue Jun 25, 2024

Merged via the queue into bevyengine:main with commit 336fddb Jun 25, 2024

alice-i-cecile added this to the 0.14 milestone Jun 25, 2024

mockersf mentioned this pull request Jun 26, 2024

fix examples color_grading and mobile after BackgroundColor changes #14033

Merged

lufog reviewed Jul 1, 2024

View reviewed changes

alice-i-cecile mentioned this pull request Jul 1, 2024

Some examples incorrectly use the Image color rather than background color #14097

Closed

re0312 mentioned this pull request Jul 3, 2024

Skip extract UiImage When its texture is default #14122

Merged

AmonDeShir mentioned this pull request Sep 19, 2024

Bevy 0.14 TheBeardedQuack/tomt_bevycss#4

Open

Uh oh!

Conversation

alice-i-cecile commented Jun 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Objective

Solution

Testing

Migration Guide

Uh oh!

Uh oh!

Uh oh!

eidloi left a comment

Choose a reason for hiding this comment

Uh oh!

benfrankel left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pablo-lua left a comment

Choose a reason for hiding this comment

Uh oh!

benfrankel left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

eidloi Jun 25, 2024

Choose a reason for hiding this comment

Uh oh!

eidloi left a comment

Choose a reason for hiding this comment

Uh oh!

eidloi Jun 25, 2024

Choose a reason for hiding this comment

Uh oh!

alice-i-cecile Jun 25, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

lufog left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

alice-i-cecile commented Jun 25, 2024 •

edited

Loading

benfrankel left a comment •

edited

Loading

lufog left a comment •

edited

Loading