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

[Core] Add scalar versions of Vector* min/max/clamp/snap(ped) #89114

Merged
merged 1 commit into from
May 2, 2024

Conversation

AThousandShips
Copy link
Member

@AThousandShips AThousandShips commented Mar 3, 2024

Convenience for a number of cases operating on single values

Added a lot of use cases, which I think should justify this addition, it simply saves a lot of code when you use repeated cases, and also reduces risk of copy errors

Optionally bound it to scripting

See also:

If it gets merged first I will expose the mini/minf/maxi/maxf methods as well, and vice-versa

@AThousandShips AThousandShips added this to the 4.x milestone Mar 3, 2024
@AThousandShips AThousandShips requested review from a team as code owners March 3, 2024 14:45
@AThousandShips AThousandShips changed the title [Core] Add scalar versions of Vector* min/max/clamp [Core] Add scalar versions of Vector* min/max/clamp/snap(ped) Mar 3, 2024
Copy link
Contributor

@Mickeon Mickeon left a comment

Choose a reason for hiding this comment

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

Documentation is completely fine and consistent with the existing methods. I just am not sure if having it exposed is strictly necessary.

@AThousandShips
Copy link
Member Author

I'd say these methods are more generally useful than the vector versions, most of the time you want to clamp or snap to the same value, though in GDScript you do have the benefit of being able to do vec.snapped(Vector2.ONE) which you don't have in C++, but vec.snappedf(1.0) is still way cleaner, and especially vec.snappedf(2.0) is way better than vec.snapped(Vector2.ONE * 2.0)

Copy link
Member

@aaronfranke aaronfranke left a comment

Choose a reason for hiding this comment

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

I think this function is a good idea, it seems very common and useful.

Copy link
Contributor

@Riteo Riteo left a comment

Choose a reason for hiding this comment

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

Finally gave a good look at this thing.

All the changes are very simple and they increase readability a lot, great job!

Convenience for a number of cases operating on single values
@akien-mga akien-mga merged commit 0b6c29f into godotengine:master May 2, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

@AThousandShips AThousandShips deleted the vec_elem_scalar branch May 2, 2024 10:49
@AThousandShips
Copy link
Member Author

Thank you!

@akien-mga
Copy link
Member

For the record, this seems to have broken LTO builds for Windows x86_32 with MinGW-GCC. See #92585 for details.

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

Successfully merging this pull request may close these issues.

8 participants