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 Val::Zero #9533

Closed
jim-ec opened this issue Aug 22, 2023 · 7 comments · Fixed by #9566
Closed

Add Val::Zero #9533

jim-ec opened this issue Aug 22, 2023 · 7 comments · Fixed by #9566
Labels
A-UI Graphical user interfaces, styles, layouts, and widgets C-Usability A targeted quality-of-life change that makes Bevy easier to use D-Trivial Nice and easy! A great choice to get started with Bevy

Comments

@jim-ec
Copy link
Contributor

jim-ec commented Aug 22, 2023

What problem does this solve or what need does it fill?

When requesting a UI length of zero, there is no clear choice of unit.

In CSS e.g., the zero literal does not have to be followed by any unit:

p {
  right: 0;
}

What solution would you like?

Add the Val::Zero case to the ui_node::Val enum.

What alternative(s) have you considered?

Just continue using Val::Px(0.0) / Val::Percent(0.0) / etc.

Additional context

https://stackoverflow.com/questions/7923042/when-specifying-a-0-value-in-css-should-i-explicitly-mark-the-units-or-omit

@jim-ec jim-ec added C-Feature A new feature, making something new possible S-Needs-Triage This issue needs to be labelled labels Aug 22, 2023
@ickshonpe
Copy link
Contributor

ickshonpe commented Aug 22, 2023

Having different representations of zero is annoying but I'm not sure another variant would help. Like now we not only have Val::Px(0.) or Val::Percent(0.) or Val::VMin(0.) etc to choose from but also Val::Zero, is that progress?

I've suggested before that we change all the UI functions to take Into<Val> and implement:

impl From<f32> for Val {
    fn from(value: f32) -> Self {
        Val::Px(value)
    }
}

to enable:

margin: UiRect::all(0.0),

It wasn't a popular idea though and I withdrew the PR.

@jim-ec
Copy link
Contributor Author

jim-ec commented Aug 22, 2023

Link to PR mentioned above: #7500

@jim-ec
Copy link
Contributor Author

jim-ec commented Aug 22, 2023

As far as I see it, the problem with #7500 is the ambiguity between units. Val::from(2.0) should mean pixels, but that was not clear from context. For zero, however, the unit does not matter, so the problem with unit ambiguity vanishes.

Of course, it is somewhat paradoxical to complain about too many options and trying to fix the problem by adding another option. However, I personally find it more elegant to use Val::Zero instead of deciding on a specific unit, but I guess that is a purely personal preference. That's why I am raising this issue, to see how others think about it.

@mockersf
Copy link
Member

Val::ZERO could be a const value that means Val::Px(0.0)

@ickshonpe
Copy link
Contributor

Val::ZERO could be a const value that means Val::Px(0.0)

Yeah, that's not so bad.

@mockersf mockersf added A-UI Graphical user interfaces, styles, layouts, and widgets and removed S-Needs-Triage This issue needs to be labelled labels Aug 22, 2023
@ickshonpe
Copy link
Contributor

ickshonpe commented Aug 22, 2023

This got me thinking, and I realised that the Val arithmetic functions are wrong. We should be able to add numeric Vals of zero to other numeric Vals, regardless of variant: #9534 (needs reviews)

@alice-i-cecile
Copy link
Member

I think we should go with the const approach instead of another variant, because of the multiple representations problem.

@alice-i-cecile alice-i-cecile added D-Trivial Nice and easy! A great choice to get started with Bevy C-Usability A targeted quality-of-life change that makes Bevy easier to use and removed C-Feature A new feature, making something new possible labels Aug 23, 2023
github-merge-queue bot pushed a commit that referenced this issue Aug 26, 2023
# Objective

- Fixes #9533

## Solution

* Added `Val::ZERO` as a constant which is defined as `Val::Px(0.)`.
* Added manual `PartialEq` implementation for `Val` which allows any
zero value to equal any other zero value. E.g., `Val::Px(0.) ==
Val::Percent(0.)` etc. This is technically a breaking change, as
`Val::Px(0.) == Val::Percent(0.)` now equals `true` instead of `false`
(as an example)
* Replaced instances of `Val::Px(0.)`, `Val::Percent(0.)`, etc. with
`Val::ZERO`
* Fixed `bevy_ui::layout::convert::tests::test_convert_from` test to
account for Taffy not equating `Points(0.)` and `Percent(0.)`. These
tests now use `assert_eq!(...)` instead of `assert!(matches!(...))`
which gives easier to diagnose error messages.
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 targeted quality-of-life change that makes Bevy easier to use D-Trivial Nice and easy! A great choice to get started with Bevy
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants