yoanlcq / vek

Generic 2D-3D math swiss army knife for game engines, with SIMD support and focus on convenience.
https://docs.rs/vek
Apache License 2.0
282 stars 32 forks source link

Consider exposing a way to normalize and get the previous magnitude of a vector in a single function #81

Closed NiklasJonsson closed 2 years ago

NiklasJonsson commented 2 years ago

If I want to get the magnitude of a vector and then normalize it, I call magnitude and then normalize/normalized but that means the magnitude is computed twice when only once is actually needed. It would be convenient with a single function if I know I need both and also save some perf 😄 .

It doesn't really matter to me if this is done by adding a return value to normalize that is the previous magnitude or if a new function is introduced (but what to call it?). I wouldn't mind submitting a PR for this if you think it would be valuable but I'd like to know what you prefer.

yoanlcq commented 2 years ago

This is a good idea. I don't mind doing it and would suggest the following :

impl<T> VecN<T> {
    pub fn normalize_and_get_magnitude(&mut self) -> T {
        let magnitude = self.magnitude();
        self /= magnitude;
        magnitude
    }
   pub fn normalized_and_get_magnitude(&self) -> (Self, T) {
        let magnitude = self.magnitude();
        (self / magnitude, magnitude)
    }
}

I'm hesitating about the _get in normalized_and_get_magnitude(), maybe should be named normalized_and_magnitude(), but maybe it's better to stay consistent in the naming.

So I'll do it now, but not publishing a new version yet, in case you have other comments or remarks. If you're fine with the suggested code, then just tell me and I'll publish ASAP.

Btw: adding a return value to normalize (the one with &mut self) is technically a breaking change, so we shouldn't do that, unfortunately.

fn foo(v: Vec3<f32>) {
    v.normalize() // Notice the lack of semicolon; this compiles now, but if normalize() returned a value, that would break...
}
NiklasJonsson commented 2 years ago

I think that looks great! I didn't think about doing the component-wise division, that is a lot simpler and would have saved me a length computation. Still, nice to have it in-crate!

As for the backward-compatibility, yeah, I didn't consider that case. I guess it could be managed by semver but this kind of change hardly seems worth it.

Thanks for the quick response!

yoanlcq commented 2 years ago

(FYI, I've published the new version just now)