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

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

Closed
NiklasJonsson opened this issue Jan 15, 2022 · 3 comments

Comments

@NiklasJonsson
Copy link

NiklasJonsson commented Jan 15, 2022

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
Copy link
Owner

yoanlcq commented Jan 15, 2022

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
Copy link
Author

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
Copy link
Owner

yoanlcq commented Jan 15, 2022

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

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

No branches or pull requests

2 participants