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

Added a convenient method to clamp the vector's components. #7369

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

GadWissberg
Copy link
Contributor

Hello,
I came across several times where I used the Vector3 to specify locations in my game's world and many times wanted to clamp the position to the map's size. So I thought instead of repeating it, it might be useful for other devs.
Thanks

Copy link
Member

@tommyettinger tommyettinger left a comment

Choose a reason for hiding this comment

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

This is definitely useful, though it would be best if it worked with Vector2 (and maybe Vector4) as well. I had to look at the sources for Vector3.clamp to notice that the existing clamp() does not actually clamp, it scales to stay within a limit rather than truncating numbers that go above or below a max or min. Since the new method is here, maybe a different name is warranted for clampComponents(), but it's pretty descriptive as-is. If you think of a better name, go for it, but I'm fine approving once Vector2 gets this (and ideally Vector4), all of which should be extremely similar to this code.

@Frosty-J
Copy link
Contributor

In that case it could be added to the Vector interface and implemented in each respective class.

I'm unsure how robust this solution is, as you may not want your map size to be perfectly cubic forever and am generally struggling to come up with uses for clamping each component to the same range.

@GadWissberg
Copy link
Contributor Author

GadWissberg commented Mar 23, 2024

Taken your feedback and applied it by adding a generic unified method to the interface and also added a more flexible version to each Vector class. Feel free to suggest a better name

@tommyettinger
Copy link
Member

In that case it could be added to the Vector interface and implemented in each respective class.

I'm unsure how robust this solution is, as you may not want your map size to be perfectly cubic forever and am generally struggling to come up with uses for clamping each component to the same range.

Mainly, clamping to the -1 to 1 or the 0 to 1 range come to mind, since those can then be scaled out to any non-cubic range you want. I'm approving this now.

@obigu
Copy link
Contributor

obigu commented Apr 1, 2024

I like the added clampComponents(float min, float max) added to the Vector interface but I don't like the implementation specific overloaded methods.

This signature hurts my eyes public Vector4 clampComponents (float xMin, float xMax, float yMin, float yMax, float zMin, float zMax, float wMin, float wMax). I'd personally stick to the generic one and remove the others. I'm not dying on this hill though if others think it's worth having them all.

@GadWissberg
Copy link
Contributor Author

I don't mind removing that method for Vector4. Also don't mind removing that method for the other implementations and keep only the one with the same clamp values for each component

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

Successfully merging this pull request may close these issues.

None yet

5 participants