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

Remove the Val::try_* arithmetic functions #9571

Closed
ickshonpe opened this issue Aug 25, 2023 · 5 comments · Fixed by #9609
Closed

Remove the Val::try_* arithmetic functions #9571

ickshonpe opened this issue Aug 25, 2023 · 5 comments · Fixed by #9609
Labels
A-UI Graphical user interfaces, styles, layouts, and widgets C-Code-Quality A section of code that is hard to understand or change D-Trivial Nice and easy! A great choice to get started with Bevy

Comments

@ickshonpe
Copy link
Contributor

ickshonpe commented Aug 25, 2023

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

Wondering if these are useful at all, or even make sense.
They aren't used internally or in any of the examples.

let mut r = UiRect::new(Val::Px(10.), Val::Percent(5.), Val::Auto, Val::VMin(50.));

// Just feels like nonsense; they don't compose or handle failure well
r.left.try_sub_assign(Val::Px(10.).unwrap();
r.right = r.right.try_add(Val::Px(10.)).unwrap_or(r.right.try_add(Val::Percent(10.)));

The try_*_assign_with_size functions seem problematic too:

r.bottom.try_add_assign_with_context(Val::Px(50.), 100., vec2(800., 600.));

r.bottom equals Val::VMin(50.) and is resolved to 600. * 50. / 100. = 300..

So r.bottom is assigned the value: Val::Px(350.).

This might seem like it's okay. But now r.bottom is no longer responsive to changes in the viewport size.

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

772 commented Aug 25, 2023

I use try_add_asign() and try_sub_assign() to move some background cloud images in my game from the right side to the left side.

fn move_background_cloud_images(
    mut background_images: Query<&mut Style, With<IsBackgroundImage1>>,
    time: Res<Time>,
) {
    let speed = 10.0;
    for mut image in background_images.iter_mut() {
        image
            .left
            .try_sub_assign(Val::Px(speed * time.delta_seconds()))
            .unwrap();
        let Val::Px(bar) = image.left else { unreachable!() };
        if bar < -1000.0 {
            image.left.try_add_assign(Val::Px(1000.0)).unwrap();
        }
    }
}

I agree that useless functions should be removed from the engine. And there are likely other ways to to what I did, but this is what I came up with using bevy docs.

@alice-i-cecile alice-i-cecile added A-UI Graphical user interfaces, styles, layouts, and widgets C-Code-Quality A section of code that is hard to understand or change and removed C-Feature A new feature, making something new possible S-Needs-Triage This issue needs to be labelled labels Aug 25, 2023
@alice-i-cecile
Copy link
Member

I'm fine to defer to you and remove these.

@ickshonpe
Copy link
Contributor Author

ickshonpe commented Aug 25, 2023

I agree that useless functions should be removed from the engine. And there are likely other ways to to what I did, but this is what I came up with using bevy docs.

That's a really helpful example. Rewritten using pattern matching it feels much more natural to me (hopefully I understood the intent of your code correctly):

fn move_background_cloud_images_3(
    mut background_images: Query<&mut Style, With<IsBackgroundImage1>>,
    time: Res<Time>,
) {
    let speed = 10.0;
    for mut style in background_images.iter_mut() {
        let Val::Px(ref mut bar) = style.left else { unreachable!(); };
        *bar -= speed * time.delta_seconds();
        if *bar < -1000. {
            *bar += 1000.;
        }
    }
}

@772
Copy link
Contributor

772 commented Aug 26, 2023

Yes, that does the same job. Sorry for the poor variable name.

@alice-i-cecile alice-i-cecile added the D-Trivial Nice and easy! A great choice to get started with Bevy label Aug 26, 2023
@ickshonpe
Copy link
Contributor Author

I'm fine to defer to you and remove these.

Definitely think this is the way to go. They seem actively unhelpful and confusing.

github-merge-queue bot pushed a commit that referenced this issue Aug 28, 2023
# Objective

Remove `Val`'s `try_*` arithmetic methods.

fixes #9571

## Changelog

Removed these methods from `bevy_ui::ui_node::Val`:
- `try_add`
- `try_sub`
- `try_add_assign_with_size`
- `try_sub_assign_with_size` 
- `try_add_assign`
- `try_sub_assign`
- `try_add_assign_with_size`
- `try_sub_assign_with_size`


## Migration Guide

`Val`'s `try_*` arithmetic methods have been removed. To perform
arithmetic on `Val`s deconstruct them using pattern matching.
rdrpenguin04 pushed a commit to rdrpenguin04/bevy that referenced this issue Jan 9, 2024
# Objective

Remove `Val`'s `try_*` arithmetic methods.

fixes bevyengine#9571

## Changelog

Removed these methods from `bevy_ui::ui_node::Val`:
- `try_add`
- `try_sub`
- `try_add_assign_with_size`
- `try_sub_assign_with_size` 
- `try_add_assign`
- `try_sub_assign`
- `try_add_assign_with_size`
- `try_sub_assign_with_size`


## Migration Guide

`Val`'s `try_*` arithmetic methods have been removed. To perform
arithmetic on `Val`s deconstruct them using pattern matching.
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-Code-Quality A section of code that is hard to understand or change 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.

3 participants