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

Add vector.combine #11920

Merged
1 commit merged into from
May 4, 2022
Merged

Add vector.combine #11920

1 commit merged into from
May 4, 2022

Conversation

appgurueu
Copy link
Contributor

@appgurueu appgurueu commented Jan 2, 2022

Adds a rather simple function vector.combine. Useful for determining bounds of a set of points by combining vectors using math.min or math.max. Ready for Review.

@sfan5 sfan5 added the Feature ✨ PRs that add or enhance a feature label Jan 2, 2022
@SmallJoker
Copy link
Member

SmallJoker commented Feb 15, 2022

Why not vector.apply, but with a 3 argument variant? I don't think there's a need for an additional function for this.

@appgurueu
Copy link
Contributor Author

Why not vector.apply, but with a 3 argument variant? I don't think there's a need for an additional function for this.

Matter of taste. Overloading functions through argument checks is not something I consider particularly clean. It would also (very slightly) hurt performance.

@rubenwardy rubenwardy added @ Script API Concept approved Approved by a core dev: PRs welcomed! labels Apr 24, 2022
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Seems fine to me from a quick look. I tried some valid operations, and as expected from the unit test they worked fine. Then I tested some invalid ones, doing things like generating nans and returning non-numerical types... these resulted in the same as you'd get from a normal vector.new call (makes sense when you look at how the code works), so I'd call that passing.

@ghost ghost merged commit ae76645 into minetest:master May 4, 2022
@appgurueu appgurueu deleted the add-vector-combine branch March 31, 2024 18:41
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Concept approved Approved by a core dev: PRs welcomed! Feature ✨ PRs that add or enhance a feature @ Script API >= Two approvals ✅ ✅
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants