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

Sum, Product implementations for Iterator<Vector*> #666

Merged
merged 1 commit into from
Apr 22, 2024

Conversation

jamesmcm
Copy link
Contributor

Add implementations for std::iter::Sum and std::iter::Product for the Vector (and IVector) types.

Note both operations are element-wise (this comes from glam).

Due to there being multiple Add and Mul implementations (ultimately in glam), I noticed having to specify types explicitly in some circumstances. I imagine this can probably be improved but I am not an expert on generic programming.

@lilizoey lilizoey added feature Adds functionality to the library c: core Core components labels Apr 19, 2024
@GodotRust
Copy link

API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-666

@jamesmcm jamesmcm force-pushed the vector_iter_impl_sum_product branch from f872e32 to 6dcddd3 Compare April 19, 2024 23:33
@fpdotmonkey
Copy link
Contributor

There also should be a docstring explaining this is all element-wise since that's not obvious for Product. Not that dot- or cross-product would make any sense for this, but those are what I'd think of first as being Vector * Vector multiplication.

Presently, there is no impl Mul<Vector> for Vector. I think that should be added since it would be incongruous to have element-wise multiplication for iterators, but not values.

Also no need to qualify that glam does element-wise multiplication; Godot does as well.

@jamesmcm jamesmcm force-pushed the vector_iter_impl_sum_product branch from 6dcddd3 to 521291b Compare April 22, 2024 17:59
@jamesmcm
Copy link
Contributor Author

Thanks, I've made these changes.

impl Mul<Vector> for Vector exists already - I think it was confusing due to the existing vector_vector macro still accepting the $Scalar value before even though it was unused, so I've removed that here too.

Add implementations for `std::iter::Sum` and `std::iter::Product` for
the Vector (and IVector) types.

Note both operations are element-wise (this comes from glam).

Due to there being multiple Add and Mul implementations (ultimately in
glam), I noticed having to specify types explicitly in some
circumstances. I imagine this can probably be improved but I am not an
expert on generic programming.
@Bromeon Bromeon force-pushed the vector_iter_impl_sum_product branch from 521291b to 13307df Compare April 22, 2024 20:03
@Bromeon Bromeon enabled auto-merge April 22, 2024 20:10
@Bromeon Bromeon added this pull request to the merge queue Apr 22, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Apr 22, 2024
@Bromeon Bromeon added this pull request to the merge queue Apr 22, 2024
Merged via the queue into godot-rust:master with commit ca58618 Apr 22, 2024
16 checks passed
@Bromeon
Copy link
Member

Bromeon commented Apr 22, 2024

Thank you, and congrats to your first merged PR! 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: core Core components feature Adds functionality to the library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants