Implemented num traits for Val

[maccesch]

Objective

• Fixes Add missing numeric trait implementations to Val #5525

Solution

I tried to "extrapolate" the match patterns from the f32 implementations. The principle I ended following is:

• Anything touches undefined becomes undefined
• Else anything that touches auto becomes auto
• Only the same unit can do the math operation
• If they're different units it becomes undefined

I feel there's a high probability that this is not exactly what you had in mind. 😄
Let me know!

[maccesch]

Also fixes #5456 because I thought this tiny change didn't warrant a separate PR

[alice-i-cecile]

Also fixes #5456 because I thought this tiny change didn't warrant a separate PR

Can you split this out? I see your point, but it'll keep the commit history cleaner and make sure that it doesn't get held up in review :

[alice-i-cecile]

Anything touches undefined becomes undefined
Else anything that touches auto becomes auto
Only the same unit can do the math operation
If they're different units it becomes undefined

I'm quite happy with these as rules; they're both intuitive and teachable. Can you document this in the Val docs?

[IceSentry]

If they're different units it becomes undefined

I don't know if I like that idea. It seems very prone to cause hard to find bugs. I'd much prefer at least logging a warning, but I wouldn't be against straight up crashing. Trying to operate on incompatible value should always be a bug in my opinion.

I also think this should be the behaviour when operating on anything that isn't an exact match on both side.

[maccesch]

Also fixes #5456 because I thought this tiny change didn't warrant a separate PR

Can you split this out? I see your point, but it'll keep the commit history cleaner and make sure that it doesn't get held up in review :

Removed

[maccesch]

If they're different units it becomes undefined

I don't know if I like that idea. It seems very prone to cause hard to find bugs. I'd much prefer at least logging a warning, but I wouldn't be against straight up crashing. Trying to operate on incompatible value should always be a bug in my opinion.

I also think this should be the behaviour when operating on anything that isn't an exact match on both side.

I have to agree, operating on incompatible values should be a bug.

But before making things crash I'd prefer to not have these math traits implemented at all for Val and force users to take out the values from Pxor Percent themselves in oder to mutliply, add, ... so they are made aware of this before runtime.

If we're expecting this not to be a big issue in practice I'll add the warnings for now and the docs.

If we want to change it to panic then we'd have to think about the num traits with f32 as well.

[maccesch]

Also while we're at it, if I remember correctly from my CSS Flexbox experience, the default value for a flexbox is auto. So this kind of would make undefined == auto, right?

[maccesch]

I added the docs for now.
Let me know if I need to add warnings and where or whatever the decision is.

[alice-i-cecile]

Yep :) I'm not entirely sure what the right behavior is, so I've slapped on a Controversial label.

[IceSentry]

But before making things crash I'd prefer to not have these math traits implemented at all for Val and force users to take out the values from Px or Percent themselves in oder to mutliply, add, ... so they are made aware of this before runtime.

I like the idea, but this has the risk of making it possible to operate on px and percent at the same time without the compiler catching it, but I like that it would force people to convert the value. Personally I still prefer the current approach as long as it at least logs a warning when incompatible types are used because it makes it impossible to operate on incompatible values.

[maccesch]

I mean people can always manually take out values from pixel and percent and do whatever they want to them. No way of preventing that.

But ok I’ll add warnings then. I guess I’ll add them as well to the f32 impls, correct?

[Weibye]

Implementation looks good but like the others I'm a bit hesitant to allow users operate on incompatible types.

1. Can we come up with any kind of sample code where it would be valid to add incompatible types?
2. If yes and the use-case makes sense then I'm happy to leave it as is (maybe emit warning)
3. If no, then we should just panic until someone has a real situation where this panic breaks their code and they can make a good case for why we should no longer panic?

[alice-i-cecile]

I'm very reluctant to introduce insidious panics in basic operators. I think it's a mistake in indexing and I think it would be a mistake here.

[IceSentry]

My issue is that Val shouldn't just coerce to undefined whenever it can't compute something because it will spread everywhere and will be extremely hard to debug.

Operating on a mixture of px and percent should actually be possible and neither be a panic nor convert to undefined. The issue with that is that percent needs to be evaluated to it's equivalent value in px. We should have something higher level, similar to calc in css that does let you operate on mixed values because it will evaluate the pixel value of a percentage. I don't think just implementing math operators on Val is the right direction if we want to limit hard to find bugs. With a calc equivalent it would also be possible to make it fallible and let the user decide if they want to fail or not. Reference: https://developer.mozilla.org/en-US/docs/Web/CSS/calc

I also disagree that panicking on basic operators is a problem, but that's a separate discussion that I don't want to have here.

I have had to deal with so many bugs in js caused by values being evaluated to NaN or undefined and breaking something that seemed totally unrelated. Those bugs are extremely easy to create and extremely hard to find because there are no warning when the conversion happens. I really don't think we should emulate that, this is one of js biggest flaw and is the main reason it has such a terrible reputation. Here's a big list of common example of js silently converting stuff unexpectedly https://github.com/denysdovhan/wtfjs

So here's the possible solutions I see:

1. Keep the operators and panic on invalid inputs
   • This makes finding those bugs much easier, but it does have a risk of unexpectedly crashing at runtime which I admit isn't perfect if it happens in a shipped application.
2. Log a warning
   • This won't cause unexpected crashing while still helping users find bugs in their code
3. Force people to convert values to the type they actually expect it to be at that moment
   • This could be in the form of a Val::px() -> f32 and Val::get_px() -> Result<f32>, just like a lot of other apis in bevy.
   • This forces people to think a bit more about what they are doing and makes it impossible to add a Px with an Undefined or Auto but still keeps the footgun of adding a Px and a Percent, but it won't spread as easily.
4. Keep this PR as is
   • Let user deal with it and be like js

Personally, I'd prefer 1 or 3, 2 is fine but less ideal and 4 is unacceptable.

There's technically a 5 that would be the calc thing I mentioned, but it's not mutually exclusive compared to the other solutions and is something that could be done later.

[IceSentry]

@maccesch just to clarify a bit, when I say that number 4 is unacceptable. Your PR was completely fine based on the issue description and I don't want you to take this personally. You did a good job with the PR I just disagree with the overall api direction.

[alice-i-cecile]

Yep, regardless of the final choice I really appreciate you pulling this together so we have a concrete implementation to debate. Much easier here than to argue in the abstract.

[Weibye]

I'm very reluctant to introduce insidious panics in basic operators. I think it's a mistake in indexing and I think it would be a mistake here.

I also disagree that panicking on basic operators is a problem, but that's a separate discussion that I don't want to have here.

Then approach 3. seems like the way to go?

That being said, I'm pretty sure that any operation with the Val::Percent is going to have to navigate up the entire tree to resolve what this percent means in pixels before it is meaningful to add a pixel and a percent value. If you have several layers of nested containers and more than one of them are defined using percentages, you'd need to resolve all the way up to the root.

Am I thinking about this incorrectly?

[IceSentry]

Yes, if we eventually have a calc function it will need to be able to go up the tree, but it won't necessarily need to go to the root. It just needs to go to the first parent with a fixed dimension.

[maccesch]

@maccesch just to clarify a bit, when I say that number 4 is unacceptable. Your PR was completely fine based on the issue description and I don't want you to take this personally. You did a good job with the PR I just disagree with the overall api direction.

@IceSentry That's very nice of you say! I didn't take it personally at all and I agree with you. For sure I agree on the JS NaN situation. That has tripped me up so many times!

I like solution 3 the most anyway because in my limited understanding it's the closest to the Rust "spirit" of things - don't allow the programmer to do bad stuff in the first place.

[Weibye]

So in the context of this PR and the general consensus to go with option 3:

• I've opened a new issue: Resolve Val::Percent into Val::Px on the Node #5574
• Please suggest improvements to the title or description

Are there any changes from this PR that it makes sense to merge or do we close this one?

[alice-i-cecile]

I think we close this down.

[maccesch]

lol my first PR to Bevy already went from good first issue to controversial to closed 🤣

[alice-i-cecile]

It was a valuable contribution :D

[Weibye]

lol my first PR to Bevy already went from good first issue to controversial to closed 🤣

It helped us see a problem we didn't know we had 😄