Ui Node Borders #7795

ickshonpe · 2023-02-23T16:14:12Z

Objective

Implement borders for UI nodes.

Relevant discussion: #7785
Related: #5924, #3991

Solution

Add an extraction function to draw the borders.

Can only do one colour rectangular borders due to the limitations of the Bevy UI renderer.

Maybe it can be combined with #3991 eventually to add curved border support.

Changelog

Added a component BorderColor.
Added the extract_uinode_borders system to the UI Render App.
Added the UI example borders

* Added the module `border` * Added `BorderStyle` and `CalculatedBorder` types. * Added `border_style` and `calculated_border` fields to `NodeBundle` and `ButtonBundle` * Added the `extract_uinode_borders` system to the UI Render App. * Added the `calculate_borders_system` which calculates the border geometry for each node. * Added the UI example `borders` * Changed the button example to give the button a border.

* Changed `calculate_borders_system` to remove the previous border geometry of degnerate nodes before continuing.

crates/bevy_ui/src/node_bundles.rs

ickshonpe · 2023-02-23T19:48:54Z

I'm not sure what the overflow behaviour for borders should be. I have it clamp the border thickness to the size of the node at the moment, which seems reasonable enough.

nicoburns · 2023-02-23T19:53:43Z

I'm not sure what the overflow behaviour for borders should be. I have it clamp the border thickness to the size of the node at the moment, which seems reasonable enough.

As far as I can see, Chrome will always expand nodes to fit borders. And it seems that Taffy also implements this. So from Bevy's perspective, I think this will be a case of "never happens" (or at least shouldn't).

nicoburns · 2023-02-23T20:01:44Z

Update: Taffy does not in fact do this. We'll probably want to fix Taffy here.

mockersf · 2023-02-23T23:14:00Z

Would it be possible to combine this PR with creating a new type specific for borders instead of UiRect (#7710) to have the width/color by border side in this PR?

crates/bevy_ui/src/render/mod.rs

…ers in `calculate_borders_system`

ickshonpe · 2023-02-24T00:22:34Z

Would it be possible to combine this PR with creating a new type specific for borders instead of UiRect (#7710) to have the width/color by border side in this PR?

I think I'd prefer to keep this PR just focused on the implementation if we can.

…nction

* moved `compute_matrix` out of loop * added comments to border module * removed border overflow clamping

* Renamed `BorderStyle` to `BorderColor` and made it a tuple-struct. * Added `BorderBundle` which contains the components needed by a node to draw a border. * Renamed the `border_style` fields of `NodeBundle` and `ButtonBundle` to `border_color`.

doup · 2023-03-07T08:14:35Z

I'm not sure what the overflow behaviour for borders should be. I have it clamp the border thickness to the size of the node at the moment, which seems reasonable enough.

If this refers to what happens when border overflows I suppose it depends on the parent node "overflow" property? Related though, is how the border affects the node size (CSS box-sizing), I would say that the most sane way is to grow the border inwards, see box-sizing: border-box: https://developer.mozilla.org/en-US/docs/Web/CSS/box-sizing

I mean, if the node has 200px width, and has a 10px border, which is the size of the node? 200px? 220px?

(ignore me if this unrelated 😅 )

nicoburns · 2023-03-07T15:33:45Z

I mean, if the node has 200px width, and has a 10px border, which is the size of the node? 200px? 220px?

It is indeed 200px (the border grows inwards) because we use box-sizing: border-box (historical note: it used to be 200px in internet explorer and 220px in Firefox - now you know why web dev was such a nightmare in the past). However, on the web if the node is 0px and has a 10px border, then the node will end up 20px wide. I'm currently implementing this in Taffy here: DioxusLabs/taffy#372, after which I don't think bevy will need to worry about this as borders will always fit in the computed layout.

doup · 2023-03-08T18:54:58Z

I'm old enough to have suffered those CSS quirks… 😅 Anyway, I was asking because ickshonpe was asking about overflow behaviour; for a moment I though Bevy wasn't doing border-box.

Although, I totally forgot that the sum of borders could be bigger than width… nice that it's being fixed. The PR you link means that size will be computed as: max(size, border + padding)?

Btw, unrelated, but just wanted to mention that you're doing an amazing job with Taffy. ❤️

nicoburns · 2023-03-08T20:31:07Z

for a moment I though Bevy wasn't doing border-box

We're definitely doing border-box! Making that the default (well, only) style is our one (intentional) deviation from the web. We have had content-box support requested, but I'm pretty reluctant to add it. Yoga, from which Taffy was originally derived mentions that it's model is (unintentionally I think) a bit of a hybrid between border-box and content-box, so it's possible we're not completely compliant with the border-box model. But I think we're pretty close now if not.

The PR you link means that size will be computed as: max(size, border + padding)?

Yes, although the computation of size in that formula is itself quite complex, involving the size, min_size,max_size and aspect_ratio styles. As well as the node's content based size and it's flexbox alignment.

Btw, unrelated, but just wanted to mention that you're doing an amazing job with Taffy. ❤️

Thanks!

crates/bevy_ui/src/render/mod.rs

mockersf · 2023-06-08T21:58:36Z

running example many_buttons without text:

without borders, around 250 fps
with borders on each button: around 120 fps

I think it's probably OK but there's definitely a perf impact

ickshonpe · 2023-06-08T23:21:07Z

running example many_buttons without text:

without borders, around 250 fps

with borders on each button: around 120 fps

I think it's probably OK but there's definitely a perf impact

Yep considering that by adding borders to many_buttons you draw an extra 50000 quads it's not that bad. If performance is really crucial you can always fall back to the old method of drawing a border by using margins or padding to draw a node within another node.

…ed from parent width.

alice-i-cecile · 2023-06-09T16:13:31Z

crates/bevy_ui/src/render/mod.rs

+    match value {
+        Val::Auto => 0.,
+        Val::Px(px) => px.max(0.),
+        Val::Percent(percent) => (parent_width * percent / 100.).max(0.),


I'd like to see a comment here about why we decided to use parent width specifically. IIRC it's consistent with CSS, but it's arbitrary enough that it's helpful to call out.

crates/bevy_ui/src/render/mod.rs

alice-i-cecile · 2023-06-09T16:19:39Z

crates/bevy_ui/src/render/mod.rs

+                },
+            ];
+
+            let transform = global_transform.compute_matrix();


This name is quite unclear.

This is just the regular transform of the uinode. In the loop it's translated to the center of each rectangle that constitutes the border. It feels to me like it might be confusing to call it anything else.

alice-i-cecile · 2023-06-09T16:23:34Z

Approach seems good, code is generally good. Examples are fine, if still pretty artificial. There are a few tricky bits of math that would really benefit from a bit more explanation.

crates/bevy_ui/src/render/mod.rs

… `height`

…orderColor`.

…nner loop.

alice-i-cecile · 2023-06-11T15:42:26Z

@natasria could we get your opinion on this PR vs 8381? They're both trying to do the same thing: we should pick one and merge it for the upcoming release :)

togetherwithasteria · 2023-06-12T00:07:39Z

@natasria could we get your opinion on this PR vs 8381? They're both trying to do the same thing: we should pick one and merge it for the upcoming release :)

Well, feature-wise #8381 has border radiuses, but it forces the borders to be the same for all sides, this PR allows specifying for different borders in the sides.

#8381 still have rough edges to polish on border radiuses (it's just aliased borders so the name is true? <3 ) and sharp transition issues too, but I don't feel it's a blocker.

togetherwithasteria · 2023-06-12T00:08:09Z

I've just realized the border limitations, oh my god. T_T

bevyengine/bevy#7795

alice-i-cecile added C-Enhancement A new feature A-UI Graphical user interfaces, styles, layouts, and widgets labels Feb 23, 2023

alice-i-cecile added this to the 0.11 milestone Feb 23, 2023

alice-i-cecile self-requested a review February 23, 2023 16:19

ickshonpe added 3 commits February 23, 2023 16:34

fix up borders example

f91e4f3

changes:

b485e17

* Changed `calculate_borders_system` to remove the previous border geometry of degnerate nodes before continuing.

lint fix

de7c9d8

nicoburns reviewed Feb 23, 2023

View reviewed changes

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

nicoburns mentioned this pull request Feb 23, 2023

Make padding/border size floor node size DioxusLabs/taffy#372

Merged

ickshonpe added 2 commits February 23, 2023 20:32

improvements to example

caa72fd

removed duplicate child insertion from example

d77ebe9

mockersf reviewed Feb 23, 2023

View reviewed changes

crates/bevy_ui/src/render/mod.rs Outdated Show resolved Hide resolved

ickshonpe added 2 commits February 23, 2023 23:41

force inner_min <= inner_max to prevent overlap of opposite edge bord…

df56157

…ers in `calculate_borders_system`

cargo fmt --all

ba63de0

ickshonpe added 6 commits February 24, 2023 00:30

move the image handle conversion out of the loop in the extraction fu…

f3f9a16

…nction

Added borders to the buttons in the many_buttons stress test example.

457648c

move compute_matrix() out of loop

e2b214c

fmt

51e8088

changes:

6630107

* moved `compute_matrix` out of loop * added comments to border module * removed border overflow clamping

changes:

0e7fdff

* Renamed `BorderStyle` to `BorderColor` and made it a tuple-struct. * Added `BorderBundle` which contains the components needed by a node to draw a border. * Renamed the `border_style` fields of `NodeBundle` and `ButtonBundle` to `border_color`.

nicoburns mentioned this pull request Jun 2, 2023

Editor-Ready UI #254

Open

mockersf reviewed Jun 8, 2023

View reviewed changes

crates/bevy_ui/src/render/mod.rs Show resolved Hide resolved

mockersf approved these changes Jun 9, 2023

View reviewed changes

Added a comment explaining that percentage border values are calculat…

af4c565

…ed from parent width.

alice-i-cecile requested review from JMS55 and IceSentry June 9, 2023 16:11

alice-i-cecile reviewed Jun 9, 2023

View reviewed changes

crates/bevy_ui/src/render/mod.rs Show resolved Hide resolved

alice-i-cecile reviewed Jun 9, 2023

View reviewed changes

crates/bevy_ui/src/render/mod.rs Outdated Show resolved Hide resolved

ickshonpe added 3 commits June 9, 2023 23:59

Replaced logical size calculations with WindowResolution::width and…

fbf687e

… `height`

Added FromReflect derive, and reflect(FromReflect, Default) to `B…

7adbf2c

…orderColor`.

Added a comment explaning the purpose of the translation inside the i…

d2bc8c6

…nner loop.

nicoburns approved these changes Jun 14, 2023

View reviewed changes

alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Jun 14, 2023

cargo fmt --all

b34f363

mockersf added this pull request to the merge queue Jun 14, 2023

Merged via the queue into bevyengine:main with commit f7aa83a Jun 14, 2023

alice-i-cecile mentioned this pull request Jun 15, 2023

Better bevy version/license algorithm for asset generation bevyengine/bevy-website#648

Merged

ickshonpe mentioned this pull request Jun 27, 2023

UI Node Border Radius and Shadows #8973

Closed

This was referenced Jun 28, 2023

News: Release 0.11 (wrong branch ... see #657) bevyengine/bevy-website#656

Closed

News: Release 0.11 bevyengine/bevy-website#657

Merged

hafiidz added a commit to hafiidz/bevy_proto that referenced this pull request Jul 20, 2023

Ui Node Borders #7795

5cae089

bevyengine/bevy#7795

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ui Node Borders #7795

Ui Node Borders #7795

ickshonpe commented Feb 23, 2023 •

edited

Loading

ickshonpe commented Feb 23, 2023

nicoburns commented Feb 23, 2023

nicoburns commented Feb 23, 2023

mockersf commented Feb 23, 2023 •

edited

Loading

ickshonpe commented Feb 24, 2023

doup commented Mar 7, 2023 •

edited

Loading

nicoburns commented Mar 7, 2023 •

edited

Loading

doup commented Mar 8, 2023

nicoburns commented Mar 8, 2023 •

edited

Loading

mockersf commented Jun 8, 2023

ickshonpe commented Jun 8, 2023

alice-i-cecile Jun 9, 2023

alice-i-cecile Jun 9, 2023

ickshonpe Jun 11, 2023

alice-i-cecile commented Jun 9, 2023

alice-i-cecile commented Jun 11, 2023

togetherwithasteria commented Jun 12, 2023

togetherwithasteria commented Jun 12, 2023

Ui Node Borders #7795

Ui Node Borders #7795

Conversation

ickshonpe commented Feb 23, 2023 • edited Loading

Objective

Solution

Changelog

ickshonpe commented Feb 23, 2023

nicoburns commented Feb 23, 2023

nicoburns commented Feb 23, 2023

mockersf commented Feb 23, 2023 • edited Loading

ickshonpe commented Feb 24, 2023

doup commented Mar 7, 2023 • edited Loading

nicoburns commented Mar 7, 2023 • edited Loading

doup commented Mar 8, 2023

nicoburns commented Mar 8, 2023 • edited Loading

mockersf commented Jun 8, 2023

ickshonpe commented Jun 8, 2023

alice-i-cecile Jun 9, 2023

Choose a reason for hiding this comment

alice-i-cecile Jun 9, 2023

Choose a reason for hiding this comment

ickshonpe Jun 11, 2023

Choose a reason for hiding this comment

alice-i-cecile commented Jun 9, 2023

alice-i-cecile commented Jun 11, 2023

togetherwithasteria commented Jun 12, 2023

togetherwithasteria commented Jun 12, 2023

ickshonpe commented Feb 23, 2023 •

edited

Loading

mockersf commented Feb 23, 2023 •

edited

Loading

doup commented Mar 7, 2023 •

edited

Loading

nicoburns commented Mar 7, 2023 •

edited

Loading

nicoburns commented Mar 8, 2023 •

edited

Loading