fu5ha / ultraviolet

A wide linear algebra crate for games and graphics.
https://crates.io/crates/ultraviolet
750 stars 81 forks source link

Generify MulAdd for Vec types #122

Closed joeftiger closed 3 years ago

joeftiger commented 3 years ago

I was kinda missing Vec3::mul_add(&self, a: f32, b: Self) so I implemented it for all vec types by implementing the trait MulAdd for all vec types with a: Self and b: $t.

I am unsure where to move the trait (was in int.rs before), so I put it inside vec/mod.rs for now.

fu5ha commented 3 years ago

Hmmm... I'm not sure that I'm a fan of this change tbh. Think I would rather just keep it simple with direct methods. Is there a use case where the trait is more convenient for you?

joeftiger commented 3 years ago

I am using smth. along the lines of

let a = Vec3::new(1.0, 2.0);
let b = 3.0;
let c = Vec3::new(4.0, 5.0);

let v = a.mul_add(b, c);

As it is right now, one either does v = a * b + c or uses broadcasting:

let a = Vec3::new(1.0, 2.0);
let b = Vec3::broadcast(3.0);
let c = Vec3::new(4.0, 5.0);

let v = a.mul_add(b, c)

I disliked having to broadcast the float to a vector every single time. I am not sure the compiler would optimize this away, as it is only one single float we multiply for each entry.

This PR would make it a bit more generic that one can use either a Vec or a f32 type. One could introduce another method like Vec3::fmul_add(&self, b: f32, c: Vec3), but I thought itd be nice to have the same name, just for the base type $t of Vec.

joeftiger commented 3 years ago

It could well be that I just fell too much in love with traits to give the same methods to "same" things which just have more entries like Vec2, Vec3, Vec4.

Ofc one could implement it handwritten each time.

I think your concern is the import of that trait every time one calls a function from it, right?

In that case I'd propose Vec::fmul_add() as a rename to implement for the vectors.

joeftiger commented 3 years ago

@termhn Should I change it to an fmul_add(&self, mul: f32, add: Self)?

joeftiger commented 3 years ago

Overlaps with #123 due to my incompetence :')

fu5ha commented 3 years ago

So my issue is basically just philosophical with the way ultraviolet is structured... I'm not sure I like fmul either though... I think I'd rather add something like From<f32> for Vec which would just call broadcast... Then you could do 10.0.into() to shorten it a bit but still would be explicit.

On Sat, Mar 27, 2021, 4:47 PM joeftiger @.***> wrote:

Overlaps with #123 https://github.com/termhn/ultraviolet/pull/123 due to my incompetence :')

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/termhn/ultraviolet/pull/122#issuecomment-808818749, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAGYXH2HATVXXPXIFKQN4I3TFZVBJANCNFSM4ZWXXYUQ .