-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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 a Clamp method to System.Math #13994
Comments
This seems quite reasonable to me. Tagging a few more folks to take a look. |
I like the idea. I think we should consider both explicit specializations for the core numeric types as well as the generic version. I can imagine the code-gen might be better for the non-generic versions, but we should validate that assumption. |
I was about to write almost the exact same thing as @ellismg. |
You'll likely need specializations for at least float and double. That's partly due to double.CompareTo/float.CompareTo being poor inlining candidates (too large), partly to make sure that NaN are properly dealt with. For example, the min/max version produces NaN if the input value is NaN while the IComparable version produces 0 (the min value). It's more common for math functions to propagate NaNs so the IComparable result seems undesirable. |
You need to pay attention to epsilon when it comes to double and floats. Dr Diego Colombo PhD
|
Epsilon is normally associated with floating point equality tests, it has nothing to do with clamping. |
In video games code you tend to give epsilon options (can be overridden ) to tweak the clamp function behaviour. Dr Diego Colombo PhD
|
Neither Unity however has a read-only value. |
I've done graphics programming and I've never seen or need a clamp function with any epsilon option (or any other optional behaviors). In particular, neither HLSL nor GLSL offer any epsilon option for their |
This looks like a good addition to me. Regarding a generic Clamp method vs. individual specializations: right now, all of the methods in the Math class operate only on primitive numeric types, and there are no generic methods. It might be a bit odd having a method that can be used on any IComparable object on the "Math" class, but then again it's not that strange considering what the method does. Seeing something like Math.Clamp(DateTime.Now, min, max) feels a bit odd at first, just because I am used to seeing primitive numeric arguments to Math methods. I don't feel very strongly about that, though, and I think a generic overload would be very useful in a lot of situations. Regarding the epsilon discussion above: Could you clarify what exactly you are referring to, @colombod? I would second that I haven't seen any epsilon arguments in any clamp functions. I'm also not sure what the semantics of such a parameter would be, as, like @mikedn said, epsilon generally refers to an acceptable margin for equality comparisons. I'm not sure how that would translate to greater-than/less-than/equal-to comparisons. I do think this is a good candidate for addition, whatever we decide about the above point. Thanks for the suggestion, @sgtfrankieboy. |
Some game engine allows you to define custom precision (like float 16) and therefore you might want to tweak the "epsilon". That way is like reducing the resolution of steps for greater and lesser checks. Dr Diego Colombo PhD
|
Again, epsilon is not used in a clamp implementation because clamp doesn't do any equality checks. And there's no support for FP16 format in .NET, its smallest floating point format has 32 bit. In general, FP16 is something you see on GPUs, CPUs barely support it. This whole talk about epsilon is a red herring in this particular context. |
I agree
|
This seems like a good idea to me as well. Tangential nitpick: the example
always returns 100. It should be
|
I haven't tested example code, it was more to show the current methods. Now that you mentioned it, it also shows that the current methods can have easily overlooked mistakes which could be hard to track down when actually using it. I've updated the example. |
Seems like a sensible idea to me. My preference is option E1 - include the overloads for the built-in numeric data types and omit the generic IComparable variant unless the same is done for there rest of the Math class' API surface area. There's a potential for some very weird patterns using the generic variant - consider Clamp, for example. |
I see that System.Math is now available in mscorlib, will it be moved over from CoreCLR to CoreFX? Because the documentation said that new API additions should happen in CoreFX unless specified. |
We've reviewed the API and we believe we should go with proposal 1 and use the specialized overloads. |
Make sure to add tests with NaNs for the floating point overloads. |
…/github.com/dotnet/corefx/issues/467 Perftests: comments on AttributeUsage
I know the
System.Math
library hasn't been pushed to the .Net Core library but since discussing api additions could take some time I thought it was a good idea to propose it now.Rationale and Usage
Math.Clamp
is an easy to implement method and makes it easier for developers. Currently developers would have to implement their own methods in an extension class or useMath.Max
andMath.Min
together. The proposed API will shorten the amount of code needed and increase readability.Two examples on how we currently can clamp in .NET:
The proposed API would turn that into:
Proposed API 1 - Specialized
This proposed version mimics the way current
System.Math
methods do it.Proposed API 2 - IComparable
Because this is a new API addition it can also be possible to have smaller implementation using generics that should cover all the cases.
Open Questions
The text was updated successfully, but these errors were encountered: