Remove `Val::Undefined`

[ickshonpe]

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

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

[alice-i-cecile]

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]

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]

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]

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]

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]

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

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

[ickshonpe]

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

[nicoburns]

Or even have the chained builder functions take an impl Into<Val> argument, and implement Into<Val> 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]

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

[alice-i-cecile]

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.

[ickshonpe]

Some of the doc comments may need to be rewritten for this, I haven't worked on them yet because there might still be more changes to be made that would need to be reflected in the documentation.

I've only made the minimum changes to the examples (except ui.rs) required to keep them working as before. I've made sure that each example's output is identical to main, but there are a lot and they all required changes, so it's possible I missed something.

With /examples/ui/ui.rs I made significant changes, and this is where to look to see the impact all this has. With this PR, the changes from my two Size PRs and improving my understanding of where the Style properties can be elided, I was able to eliminate about 20% of the styling code for this example.

[ickshonpe]

Appreciate the clarifications. A lot of my knowledge comes from working backwards from Bevy UI to CSS, so I have a lot of gaps. Agree with you that an Option<Val> seems like the right approach if some sort of undefined values are needed in the future.

[nicoburns]

A lot of my knowledge comes from working backwards from Bevy UI to CSS, so I have a lot of gaps.

I would recommend MDN as a reference e.g. https://developer.mozilla.org/en-US/docs/Web/CSS/width

[ickshonpe]

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.

[ickshonpe]

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]

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]

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]

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]

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