Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add an enum with only the numeric variants of Val #7569

Closed
wants to merge 7 commits into from

Conversation

ickshonpe
Copy link
Contributor

@ickshonpe ickshonpe commented Feb 8, 2023

Problem

The Style properties padding and border can be assigned values of Val::Auto or Val::Undefined when only the numeric variants make sense. There isn't a sensible way for the UI to determine these values automatically, except to just set them to zero.

Solution

  • Add a second enum Breadth with only the evaluatable variants of Val.
  • Make UiRect generic.
  • Change the types of Style's padding and border fields to UiRect<Breadth>.

I'm not sure I like the name Breadth, but it's shorter than Thickness and seems more descriptive than Length.

Another option would be to rename Val to AutoVal and rename Breadth to Val. AutoVal would be used a lot more though, so this might be annoying.

Perhaps NumVal or EVal for Evaluatable Value. Num would be nice but ambiguous with a lot of other things.

There might be objections to making UiRect generic. Could be divided into NumRect and AutoRect or something instead. Don't care either way, or about any of the namings.


Changelog

  • Added an enum Breadth, that is similar to Val but without Val's non-numeric variants.
  • Changed UiRect into a generic struct.
  • Changed the types of the padding and border fields of Style to UiRect<Breadth>.
  • Made the minimum necessary changes to the affected examples.

Migration Guide

  • UiRect is now a generic struct with a type parameter, which is Val by default.

  • A new enum Breadth has been added which is similar to Val but without Val's non-numeric variants Val::Auto and Val::Undefined.

  • The padding and border fields of Style have been changed to type UiRect<Breadth>. Any padding and border values of Val::Auto or Val::Undefined should be replaced with Breadth::Px(0.), which is the default value for Breadth.

        Added a type `Breadth`, that is similar to `Val` but with only evaluatable variants.
        Gave UiRect a type parameter, so it can take `Breadth` or `Val` values.
        Added tests for `Breadth` and `UiRect`.
        Changed style properties to use `Breadth` instead of `Val` for the padding and border properties.
        Changed `bevy_ui::flex::convert::from_rect` to take an `UiRect<T: Into<Val>>` instead of a `UiRect`.
        Made minimal necessary changes to the examples.
…padding and border values in `bevy_ui::flex::convert::from_style`
@alice-i-cecile alice-i-cecile added A-UI Graphical user interfaces, styles, layouts, and widgets C-Usability A simple quality-of-life change that makes Bevy easier to use X-Controversial There is active debate or serious implications around merging this PR labels Feb 8, 2023
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I definitely like this direction. Unusable values should be unrepresentable. Great implementations with nice error types!

Labelling this with controversial, since this is a major use-facing UX change.

@nicoburns
Copy link
Contributor

This seems like a reasonable change to introduce (although IMO it does add friction when authoring styles as you have to remember which properties correspond to which types). I'm not keen on the name though. Perhaps FixedVal could be an alternative name.

@ickshonpe
Copy link
Contributor Author

ickshonpe commented Mar 14, 2023

This seems like a reasonable change to introduce (although IMO it does add friction when authoring styles as you have to remember which properties correspond to which types). I'm not keen on the name though. Perhaps FixedVal could be an alternative name.

I think if we went that way, using Val for the numerical values and renaming Val to AutoVal would make the most intuitive sense.

And then we could implement From<Val> for AutoVal, and have the current constructor functions take an Into<AutoVal> so that most existing code using Bevy UI wouldn't need any changes.

@alice-i-cecile
Copy link
Member

I think if we went that way, using Val for the numerical values and renaming Val to AutoVal would make the most intuitive sense.

I like this better than FixedVal, and I think better than the current proposal.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-UI Graphical user interfaces, styles, layouts, and widgets C-Usability A simple quality-of-life change that makes Bevy easier to use X-Controversial There is active debate or serious implications around merging this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants