Skip to content

Conversation

TheBlckbird
Copy link
Contributor

@TheBlckbird TheBlckbird commented Aug 13, 2025

Objective

  • Allow the Val-helper functions to accept more types besides just f32

Fixes #20549

Solution

  • Adds a new trait that can be implemented for numbers
  • That trait has a method that converts self to f32

Testing

  • I tested it using Rust's testing framework (although I didn't leave the tests in, as I don't deem them important enough)
Rust test
#[cfg(test)]
mod tests {
    use super::*;

    #[test]
    fn test_val_helpers_work() {
        let p = px(10_u8);
        assert_eq!(p, Val::Px(10.0));

        let p = px(10_u16);
        assert_eq!(p, Val::Px(10.0));

        let p = px(10_u32);
        assert_eq!(p, Val::Px(10.0));

        let p = px(10_u64);
        assert_eq!(p, Val::Px(10.0));

        let p = px(10_u128);
        assert_eq!(p, Val::Px(10.0));

        let p = px(10_i8);
        assert_eq!(p, Val::Px(10.0));

        let p = px(10_i16);
        assert_eq!(p, Val::Px(10.0));

        let p = px(10_i32);
        assert_eq!(p, Val::Px(10.0));

        let p = px(10_i64);
        assert_eq!(p, Val::Px(10.0));

        let p = px(10_i128);
        assert_eq!(p, Val::Px(10.0));

        let p = px(10.3_f32);
        assert_eq!(p, Val::Px(10.3));

        let p = px(10.6_f64);
        assert_eq!(p, Val::Px(10.6));
    }
}

Showcase

// Same as Val::Px(10.)
px(10);
px(10_u8);
px(10.0);

@TheBlckbird
Copy link
Contributor Author

This is still waiting for a decision on #20549.

And this should probably be mentioned with the helper functions in the release notes. If so, I'll have to look into that.

@alice-i-cecile alice-i-cecile added X-Controversial There is active debate or serious implications around merging this PR S-Needs-Testing Testing must be done before this is safe to merge A-UI Graphical user interfaces, styles, layouts, and widgets C-Usability A targeted quality-of-life change that makes Bevy easier to use labels Aug 13, 2025
@alice-i-cecile
Copy link
Member

This also needs testing against #20158, to ensure that it plays nice with BSN.

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 don't understand why we need a dedicated ToValNum trait. Why can't we just use impl Into<Val>?

@TheBlckbird
Copy link
Contributor Author

TheBlckbird commented Aug 13, 2025

You mean:

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

That would have the issues with having to specify a default unit, see #20517 and @cart's comment: #20517 (comment)

Or am I missing something?

@alice-i-cecile
Copy link
Member

Ah, I see. Can you please explain this in a doc comment on ToValNum?

@TheBlckbird TheBlckbird marked this pull request as ready for review August 13, 2025 17:26
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.

LGTM if we can get this working with BSN.

@TheBlckbird
Copy link
Contributor Author

This also needs testing against #20158, to ensure that it plays nice with BSN.

Ok, so I just switched to that branch and merged these changes locally and experimented with the examples. There don't seem to be any problems.

I'll update the examples in here tomorrow and will do the same for BSN once cart merges main into the feature branch again.

Copy link
Contributor

@ickshonpe ickshonpe left a comment

Choose a reason for hiding this comment

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

I don't like the extra conversions or the macro, it just seems useless to me.
The use case for this is just we want to be able to type px(10) instead of px(10.), which only needs i32. The times you'd need the other conversions are so rare, I don't see any problem with requiring an explicit cast. It could even hide errors, using i128/u128s for UI coords seems especially fishy.

Practically its not going to make any significant difference though, so I approve anyway with that objection.

@TheBlckbird
Copy link
Contributor Author

TheBlckbird commented Aug 14, 2025

The argument could be made to remove i128/u128 from the impl blocks, especially since the limit for these two is higher than for f32 (afaik), which could potentially lead to weird errors (if someone goes that high for any reason, but they should still decide to cast that themselves explicitly).

Thanks for the feedback

@TheBlckbird
Copy link
Contributor Author

Sorry for the big commit. I changed all the references to Val:: to just the helper functions using a script and edited some edge cases by hand. The examples all compile without any errors and I quickly looked over the diff, so it should be fine.

@alice-i-cecile alice-i-cecile removed the S-Needs-Testing Testing must be done before this is safe to merge label Aug 14, 2025
@alice-i-cecile alice-i-cecile added the S-Needs-SME Decision or review from an SME is required label Aug 14, 2025
@cart cart added this pull request to the merge queue Aug 29, 2025
Merged via the queue into bevyengine:main with commit 13877fa Aug 29, 2025
32 checks passed
@TheBlckbird TheBlckbird deleted the improvement/val-helper-types branch August 30, 2025 09:00
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 S-Needs-SME Decision or review from an SME is required 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.

Add a trait for the Val helper functions to allow the input of multiple types
4 participants