Refactored the clamp function (preserving the unit)#1608
Conversation
- updated the tests
| Length clampedMin = UnitMath.Clamp(value, min, max); | ||
|
|
||
| Assert.Equal(-1, clampedMin.Value); | ||
| Assert.Equal(LengthUnit.Meter, clampedMin.Unit); |
There was a problem hiding this comment.
Hm, not sure I like that we change the unit of value here. I'd expect it to keep its unit, but clamp it to the bounds of min/max.
There was a problem hiding this comment.
I did ask you about the two options..
There was a problem hiding this comment.
Hm, not sure I like that we change the unit of
valuehere. I'd expect it to keep its unit, but clamp it to the bounds of min/max.
I've reverted the behavior, although there is a strong argument against the extra conversions: if preserving the unit is required, the user can always do UnitMath.Clamp(value, min, max).ToUnit(value.Unit) (or pass in min.ToUnit(value.Unit), ..), while the opposite, i.e. avoiding the return of a huge value with a tiny unit, is not possible.
There was a problem hiding this comment.
For reference, here's the discussion about this a few years ago when I authored Clamp:
#1157 (comment)
I guess the principle of least surprise should still apply here.
lipchev
changed the title
Edit class xmldoc, no longer much math operations left here.
3 participants
ToUnit(Enum)and no longer comparing with theCompareTo(object)