search

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

Use `MulAdd` from num-traits #73

Closed zakarumych closed 3 years ago

zakarumych commented 3 years ago

vek uses traits from num-traits but rolls its own MulAdd trait for some reason. It could be that there was no MulAdd in num-traits when it was added to vek. Right now this seems confusing.

yoanlcq commented 3 years ago

Thanks for bringing this to my attention!

It could be that there was no MulAdd in num-traits when it was added to vek.

That is true IIRC. When I wrote vek I made sure to use num-traits extensively, and I would have preferred using num-traits's MulAdd rather than rolling my own.

So I agree and I'd be in favor of using num-trait's MulAdd. This is technically a breaking change but I think it'll be fine. I'll try to take some time to look into this soon.

zakarumych commented 3 years ago

I think that if vek would re-export MulAdd it won't be breaking change then.

yoanlcq commented 3 years ago

Re-exporting MulAdd appeared to do the trick with no issue.

Interestingly enough though, it is still a (slightly) breaking change because now MulAdd is not implemented for std::num::Wrapping<T>, and regardless of that, the actual operation's implementation may have changed.
No big deal, it just means I'll update the minor version instead of the patch version, and call it a day.

yoanlcq commented 3 years ago

I've published the 0.15.0 version with the fix just now, so I'll close this. Thanks again.