Add vector normalization to Vector trait #114
Conversation
src/vector.rs
Outdated
| /// Compute the squared magnitude of a vector | ||
| fn magnitude_sq(self) -> C | ||
| where | ||
| C: core::iter::Sum, | ||
| { | ||
| self.iter().map(|n| n * n).sum() | ||
| } | ||
|
|
||
| /// Compute the magnitude of a vector | ||
| fn magnitude(self) -> f32 |
There was a problem hiding this comment.
It might make sense for magnitude to be composed in terms of magnitude_sq. This change also seems irrelevant to the addition of normalized and could possibly use its own PR. We might also consider changing magnitude to similarly return C, and possibly other methods as well, though that seems like a breaking change.
There was a problem hiding this comment.
I had the same thoughts, just didn't want to fiddle with the existing implementations too much. Expressing magnitude in terms of magnitude_sq requires the core::iter::Sum trait bound as well.
I agree that magnitude_sq can be removed here as it doesn't add to normalize. I'll pull it into a separate PR.
There was a problem hiding this comment.
Went a different way now and added the map function that transforms elements individually. Let me know what you think about it; if it doesn't fit, I'll remove it.
sunsided
force-pushed
the
feature/magnitude-sq
branch
from
a2ef9cd
to
c0e2cde
Compare
sunsided
force-pushed
the
feature/magnitude-sq
branch
from
c0e2cde
to
ddaccc6
Compare
Signed-off-by: Markus Mayer <[email protected]>
sunsided
force-pushed
the
feature/magnitude-sq
branch
from
91f1b3d
to
a7481bd
Compare
This addresses #99 and performs vector normalization by reusing the existing
magnitudeimplementation, i.e. it goes throughf32, normalizes inf32and then converts back toCby usingFromIterator.I attempted pre-calculating the inverse of the magnitude and then multiplying by that, but it brought the results too far off in terms of error.
I thought about factoring out
invsqrtinto a trait and then implement that trait forF32andf32alike. This way themagnitudefunction could be more generic (C: InvSqrtinstead ofC: Into<f32>) and wouldn't require the use off32explicitly.