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

Updated clamp method #2335

Closed
wants to merge 1 commit into from
Closed

Updated clamp method #2335

wants to merge 1 commit into from

Conversation

@jacobrec
Copy link

jacobrec commented Sep 14, 2014

Works properly now when min > max.

Works properly now when min > max.
@NathanSweet
Copy link
Member

NathanSweet commented Sep 14, 2014

I think I would prefer if min > max then we either do nothing or the input is invalid and we throw an exception. If you are clamping a value to a range, it doesn't make sense to pass a min > max.

@MobiDevelop
Copy link
Member

MobiDevelop commented Sep 14, 2014

I'd agree that passing a min which is greater than max is invalid input.

@NathanSweet
Copy link
Member

NathanSweet commented Sep 14, 2014

Shall we check it and throw or leave it silent?

@guillaume-alvarez
Copy link
Contributor

guillaume-alvarez commented Sep 14, 2014

I'd vote to check it and throw: earlier you fail, earlier you fix.

@NathanSweet
Copy link
Member

NathanSweet commented Sep 14, 2014

My worry is someone relies on the behavior. If you pass a min > max then the function correctly returns false. It would actually be annoying to need to check for min > max if you wanted false back in that case. I guess my vote is that it isn't necessarily an error if min > max so we can leave the method as is.

@xoppa
Copy link
Member

xoppa commented Sep 14, 2014

Fwiw, I'd say that clamp is a shortcut for min(max(x, minVal), maxVal) (which is how opengl defines the method). I don't think that it should throw an exception, nor swap arguments (which is what this PR does).

@NathanSweet
Copy link
Member

NathanSweet commented Sep 14, 2014

Agreed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.