Remove `Val::Undefined` #7485

ickshonpe · 2023-02-03T11:05:11Z

Objective

Val::Undefined is unnecessary and very confusing.

Currently, it's used to represent a lot of different things in Bevy depending on the context.

The details, as I understand them:

CSS doesn't have "undefined" values. Only auto.
Undefined is Val's default variant.
UiRect's fields are default Val::Undefined
[Merged by Bors] - change the default width and height of Size to Val::Auto #7475 changes the default width and height of Size to Val::Auto.
For the width and height of size in Style, Val::Undefined is equivalent to Val::Px(0.) and Val::Percent(0.)
(except with text nodes because the implementation is incomplete. With a complete implementation, they would be equivalent).
For the width and height of min_size and max_size, Val::Undefined is equivalent to Val::Auto. This behaviour is different from size because otherwise when Val::Undefined was the Size default every node would have been constrained to a zero size by default.
When UiRect is used to represent positions, Val::Undefined is equivalent to Val::Auto. Positions are either specified explicitly by the user or determined automatically by Taffy. There are no undefined positions.
When UiRect is used to represent margins, Val::Undefined is equivalent to Val::Px(0.) and Val::Percent(0.). This difference from positions is because a position of zero still represents a position, whereas a margin of zero is no margin at all.
When UiRect is used to represent padding or borders, both Val::Undefined and Val::Auto are the same as Val::Px(0.) (no padding or borders). Unlike with margins, there is no concept of auto borders or padding.

The Taffy equivalent Dimension::Undefined has been removed in Taffy 3 without problems.

related issues: #7120, #5513

Solution

Remove the Undefined variant of Val.
Change the UiRect default values to Val::Px(0) (no margin/border/padding).
Remove the position field from Style, and replace it with separate left, right, top and bottom fields.
Set the default value for left, right, top and bottom on Style to Val::Auto.

Changelog

Removed the Undefined variant of the Val enum.
Changed UiRect::default() to set all fields to Val::Px(0.).
Added left, right, top and bottom fields to Style with default values of Val::Auto.
Updated the UI examples to reflect new changes.

Migration Guide

Val::Undefined has been removed. Bevy UI's behaviour with default values should remain the same.
The default values of UiRect's fields have been changed to Val::Px(0.).
Style's position field has been removed. Its left, right, top and bottom fields have been added to Style directly.
For the size, margin, border, and padding fields of Style, Val::Undefined should be replaced with Val::Px(0.).
For the min_size, max_size, left, right, top and bottom fields of Style, Val::Undefined should be replaced with Val::Auto

* Removed the `Undefined` variant from `Val`. * Set Val's default variant to `Auto`. * Updated ui examples. * Removed `Val::Undefined` conversion code.

* Removed the `Undefined` variant of the `Val` enum. * Changed `UiRect::default()` to set all fields to `Val::Px(0.0)`. * Added the `Inset` type that is a copy of `UiRect` but its defaults are all `Val::Auto`. * Renamed the `position` field of `Style` to `inset` and changed its type to `Inset`. * Updated the UI examples to remove or replace any use of `Val::Undefined`. * Updated the UI examples to use `Inset`.

alice-i-cecile · 2023-02-03T16:43:21Z

I am on board with removing Val::Undefined in favor of more explicit constructors. I need to review this PR in more detail though. Note that Cart previously expressed opposition to the Inset name, so we should consider avoiding that here.

ickshonpe · 2023-02-03T19:47:55Z

I am on board with removing Val::Undefined in favor of more explicit constructors. I need to review this PR in more detail though. Note that Cart previously expressed opposition to the Inset name, so we should consider avoiding that here.

np, changed it to Position. I really didn't want to include changes to Style at all, but the requirement for different defaults made it unavoidable.

nicoburns · 2023-02-03T20:23:46Z

I really didn't want to include changes to Style at all, but the requirement for different defaults made it unavoidable.

When I made the equivalent changes to Taffy I just removed the default values for the individual field structs (what in bevy would be UiRect, Val, etc) entirely in favour of just having a default implementation on Style. Although we did add style helpers at the same time. IMO:

position: UiRect { top: px(0.), left: px(0.), bottom: px(10.), right: px(15.) },

is much nicer than:

position: Position(UiRect {
   bottom: Val::Px(10.),
   right: Val::Px(15.),
   ..Default::default()  // left and top set to Px(0.), not Auto
}),

It would also be possible to make the style helpers accept integers which would give you the even nicer:

position: UiRect { top: px(0), left: px(0), bottom: px(10), right: px(15) },

ickshonpe · 2023-02-03T20:58:06Z

Oh, that's got me thinking.. unlike all the other fields like margin or size, you almost never want to set all four position values and I've always felt it's kind of annoying having them grouped together when you normally only ever set two of the fields at once.

Instead, we could just have the left, right, up, and down, fields on Style, and then as you are saying, there is no problem with the default implementations.

Like I think

Style {
    left: Val::Px(10.),
    top: Val::Px(15.),   
    ..Default::default()
}

is a lot more ergnomic than:

Style {
    position: Position {
        left: Val::Px(10.),
        top: Val::Px(15.),
        ..Default::default()
    },
    ..Default::default()
}

nicoburns · 2023-02-03T21:06:38Z

Indeed. I think my ultimate endgame is to have a builder for Style so you can do something like:

Style::new()
    .left(Val::Px(10.))
    .top(Val::Px(15.))

At which point the internal representation doesn't matter.

ickshonpe · 2023-02-03T21:17:27Z

and you could combine the variant helper functions into the interface, like:

Style::new()
.left(10.)   // `Val::Px(10.)`
.top_percent(15.);

ickshonpe · 2023-02-03T21:24:25Z

Or even have the chained builder functions take an impl Into<Val> argument, and implement Into<Val> for f32 into Val::Px

nicoburns · 2023-02-03T22:04:08Z

Or even have the chained builder functions take an impl Into argument, and implement Into for f32 into Val::Px

Yeah, that one would be ideal. It would be great if you could also do thing like .left(10.percent())

ickshonpe · 2023-02-04T16:23:54Z

Would there be any drawbacks to just adding Val::Px and Val::Percent to the prelude?

alice-i-cecile · 2023-02-04T16:48:52Z

Would there be any drawbacks to just adding Val::Px and Val::Percent to the prelude?

May be a bit unclear / implicit. I think it's likely worth it: I import them for my UI projects regularly. And Option/Result use the same pattern.

TimJentzsch

I added a small suggestions to improve some docs, but otherwise I really like it.

The undefined value has been a source of confusion which is nice to remove. This should also bring us closer to how things are defined in the web.

I think UiRect having a default of zero is confusing conceptually, but this is more because of the name: It doesn't represent a rectangle, but more a border around a rectangle.
Anyway, this is kind of a naming issue separate from this PR.

TimJentzsch · 2023-03-02T12:36:54Z

crates/bevy_ui/src/geometry.rs

+    /// assert_eq!(ui_rect.left, Val::Px(0.));
+    /// assert_eq!(ui_rect.right, Val::Px(0.));
+    /// assert_eq!(ui_rect.top, Val::Px(0.));


GitHub won't comment me there directly, but can we maybe add a line to the documentation to make it explicit that all other values will be zero?

So something like:

Creates a new [`UiRect`] where `bottom` takes the given value. All other values will be zero.

And then for the other methods (e.g. top) as well.

Yes that makes sense and the same should be done for the Size helper functions too.

…unctions set to fields with no given value.

ickshonpe · 2023-03-03T10:07:05Z

I think UiRect having a default of zero is confusing conceptually, but this is more because of the name: It doesn't represent a rectangle, but more a border around a rectangle. Anyway, this is kind of a naming issue separate from this PR.

UiRect is such a terrible name, we had a bunch of discussions spanning a couple of PRs; #7656 and #7569.
Frame seems like the best idea for a renaming, if we go that way. Geometrically it communicates the right concept.

My proposal is #7710 which replaces UiRect with separate Margin, Padding and Border structs generated with a macro.
The other option is builder methods on Style, nicoburns just put in a PR #7881 implementing some. I don't dislike that idea either and both #7710 and #7881 could coexist. But I think builder methods might be controversial and end up blocked a long time.

crates/bevy_ui/src/geometry.rs

doup

I've little understanding of Bevy internals, but as long as UiRect is used for margin, border, padding it should be fine (?). I have a hunch that if it was used for position, the default 0px would give problems with absolute positioning.

Other name ideas for UiRect: Box (like in "box model"), Edge. I think CSS spec calls it BoxEdge (?). I still like Frame the most.

Speaking of Val "shortcuts", what about?

15. for Val::Px(15.)
25_f32.percent() for Val::Percent(25)
() for Val::Auto()

Adding From for this then would need to change Style to accept Into, etc., no? Maybe px(), percent() and auto() would be the less intrusive, although pollutes the scope.

ickshonpe · 2023-03-08T10:17:50Z

I've little understanding of Bevy internals, but as long as UiRect is used for margin, border, padding it should be fine (?). I have a hunch that if it was used for position, the default 0px would give problems with absolute positioning.

Margins need to be separated from borders and padding too. There's no sensible way for padding and border values to be inferred by the layout algorithm, so it makes no sense for them to have an Auto variant.

The problem for positions is that 0px is still a valid position, whereas for a margin (or the others) a 0px value represents that margin not being present at all.

nicoburns · 2023-03-08T11:40:35Z

There's no sensible way for padding and border values to be inferred by the layout algorithm, so it makes no sense for them to have an Auto variant.

There actually is a sensible interpretation of "auto" padding. Which would be that the auto padding of a container takes up a share of the same space that the auto margins of it's children do. I am considering adding opt-in support for this to Taffy, because it is similar to way in which morphorm layout works. But CSS does not allow this, so it may well not make sense to enable such support in Bevy.

In any case, this wouldn't be the default value for padding, so the problem would remain that the default for margin/padding/border needs to be 0px while the default for inset (aka position) needs to be auto.

doup · 2023-03-09T08:32:57Z

Could this PR cause issues when defining the size via flex grow/shrink/basis in conjunction with height: 100%? In this case, what would be the value of width? Auto?

I'm saying this because width/height are joined in size: Size. Maybe it makes sense to split size?

nicoburns · 2023-03-09T11:12:46Z

In this case, what would be the value of width? Auto?

Yes, it should be auto. Which is it's default value in CSS (see https://developer.mozilla.org/en-US/docs/Web/CSS/width).

alice-i-cecile · 2023-03-13T15:16:55Z

I'm relieved to see this gone. Good work, and excellent reviews.

ickshonpe added 9 commits February 2, 2023 15:48

change Size default width and height to Val::Auto

ea7bc05

changes:

5573ea6

* Removed the `Undefined` variant from `Val`. * Set Val's default variant to `Auto`. * Updated ui examples. * Removed `Val::Undefined` conversion code.

Merge branch 'bevyengine:main' into main

78fe6ad

Changed inset doc comments

b274e9e

cargo fmt --all

9cffde9

fixed UiRect doc tests

8ed613f

Another Doc comment fix

9bdf85e

fix doc comments

54f68ce

alice-i-cecile added A-UI Graphical user interfaces, styles, layouts, and widgets C-Usability A targeted quality-of-life change that makes Bevy easier to use M-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide labels Feb 3, 2023

renamed Inset to Position

ce52876

ickshonpe added 2 commits February 3, 2023 19:56

cargo fmt --all

abdef25

update ui example

5e814cf

Removed Position struct, added its fields directly to Style

2087be6

ickshonpe added 2 commits February 3, 2023 22:14

fix doc comments

99b93f8

fix doc comment

5ab3ffd

ickshonpe added 2 commits February 7, 2023 19:50

added some simple tests

eb88a97

cargo fmt

6113911

TimJentzsch reviewed Mar 2, 2023

View reviewed changes

alice-i-cecile added this to the 0.11 milestone Mar 2, 2023

ickshonpe added 3 commits March 2, 2023 21:58

Changed the doc comments to be explicit about the values the helper f…

431d97e

…unctions set to fields with no given value.

Merge branch 'main' into remove-val-undefined

a38647f

fixed errors in ui example

c8f200e

TimJentzsch approved these changes Mar 2, 2023

View reviewed changes

fixed positions

f3f02d8

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 Mar 3, 2023

doup reviewed Mar 8, 2023

View reviewed changes

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

doup approved these changes Mar 8, 2023

View reviewed changes

Removed position example from UiRect

5e3e7cf

ickshonpe added 7 commits March 9, 2023 11:44

Merge branch 'main' into remove-val-undefined

d1c7e33

Merge remote-tracking branch 'origin/main' into remove-val-undefined

6215dc6

update convert module

e72dca8

Fixed position in bloom_3d example

29db012

Fixed position in bloom_2d example

5cca520

cargo fmt

51bd6df

fix test

9369aa1

alice-i-cecile added this pull request to the merge queue Mar 13, 2023

Merged via the queue into bevyengine:main with commit 87dda35 Mar 13, 2023

rparrett mentioned this pull request Mar 14, 2023

Blend modes example UI is broken #8091

Closed

ProfLander pushed a commit to shfty-rust/bevy that referenced this pull request Mar 19, 2023

Remove Val::Undefined (bevyengine#7485)

f831179

ProfLander pushed a commit to shfty-rust/bevy that referenced this pull request Mar 19, 2023

Remove Val::Undefined (bevyengine#7485)

4d02dfa

Uh oh!

Remove Val::Undefined #7485

Remove Val::Undefined #7485

Uh oh!

Conversation

ickshonpe commented Feb 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Objective

Solution

Changelog

Migration Guide

Uh oh!

alice-i-cecile commented Feb 3, 2023

Uh oh!

ickshonpe commented Feb 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nicoburns commented Feb 3, 2023

Uh oh!

ickshonpe commented Feb 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nicoburns commented Feb 3, 2023

Uh oh!

ickshonpe commented Feb 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ickshonpe commented Feb 3, 2023

Uh oh!

nicoburns commented Feb 3, 2023

Uh oh!

ickshonpe commented Feb 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alice-i-cecile commented Feb 4, 2023

Uh oh!

TimJentzsch left a comment

Choose a reason for hiding this comment

Uh oh!

TimJentzsch Mar 2, 2023

Choose a reason for hiding this comment

Uh oh!

ickshonpe Mar 2, 2023

Choose a reason for hiding this comment

Uh oh!

ickshonpe commented Mar 3, 2023

Uh oh!

Uh oh!

doup left a comment

Choose a reason for hiding this comment

Uh oh!

ickshonpe commented Mar 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nicoburns commented Mar 8, 2023

Uh oh!

doup commented Mar 9, 2023

Uh oh!

nicoburns commented Mar 9, 2023

Uh oh!

alice-i-cecile commented Mar 13, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Remove `Val::Undefined` #7485

Remove `Val::Undefined` #7485

ickshonpe commented Feb 3, 2023 •

edited

Loading

ickshonpe commented Feb 3, 2023 •

edited

Loading

ickshonpe commented Feb 3, 2023 •

edited

Loading

ickshonpe commented Feb 3, 2023 •

edited

Loading

ickshonpe commented Feb 4, 2023 •

edited

Loading

ickshonpe commented Mar 8, 2023 •

edited

Loading