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

Overload basic operators #22

Closed
kvark opened this issue Jan 10, 2014 · 13 comments
Closed

Overload basic operators #22

kvark opened this issue Jan 10, 2014 · 13 comments

Comments

@kvark
Copy link
Collaborator

kvark commented Jan 10, 2014

Vector +- Vector
Point +- Vector
Matrix * Vector
Quat * Vector
...
probably a lot more, but we need to keep the behavior strictly obvious (i.e. not overloading Vector*Vector as a dot product)

@ghost
Copy link

ghost commented Jan 22, 2014

This would be pretty useful. With this change would we want to remove mul_v ect?

The way the trait is designed we should be able do some nice operator overloading.

impl<S: Float> Mul<Mat4<S>, Mat4<S>> for Mat4<S>
{
   fn mul(&self, rhs: &Mat4<S>) -> Mat4<S> {self.mul_m(rhs)}
}

impl<S: Float> Mul<S, Mat4<S>> for Mat4<S>
{
   fn mul(&self, rhs: &S) -> Mat4<S> {self.mul_s(*rhs)}
}

impl<S: Float> Mul<Vec4<S>, Vec4<S>> for Mat4<S>
{
   fn mul(&self, rhs: &Vec4<S>) -> Vec4<S> {self.mul_v(rhs)}
}

Might be a bit ugly because we would likely have to do both directions A * B and B * A to avoid the user running into strange compiler errors.

@brendanzab
Copy link
Collaborator

My plan is for matrices for example to satisfy a Ring as outlined here: https://github.com/bjz/num-rs/blob/master/src/algebra.rs#L300-L348

This would require operator overloading.

@ghost
Copy link

ghost commented Jan 23, 2014

So with the current Operator overloading we cannot implement more then one type. So we can do Mat4 * Mat4 but not Mat4 * Vec4 since it will be considered a conflicting implementation.

@brendanzab brendanzab mentioned this issue Jan 23, 2014
@brendanzab
Copy link
Collaborator

@csherratt yes, unfortunately. I think @pcwalton has plans to relax this restriction though.

@pnkfelix
Copy link

We can use double-dispatch to get overloading right now if we want it.

@pnkfelix
Copy link

To see what I mean by double-dispatch, see how I have ported the C++ tests for GLM's vec2 type over to Rust: glm-rs tests, ported from glm tests.

This workaround/pattern does not solve all cases of interest; in particular, one would need to implement TVec2AddAssignRHS for every scalar type individually; you cannot just do:

impl<T:Num> TVec2AddAssignRHS<T> for T { ... }

because then it would not be able to have that generic impl co-exist with the impl for TVec2<f32> etc. But these sorts of problems do not bother me too much; it is more important for a utility library like this tha one have reasonably nice syntax for the known variations of interest.

Also, there are other drawbacks one can note by reviewing the code. I am not 100% sure whether it is a bug or by design that I cannot write 1.0 alone but instead have to specify 1.0f32 in so many places. That might be fixable.

@brendanzab
Copy link
Collaborator

Cool! glm-rs looks rather neat.

I am still hesitant to do this until @nikomatsakis lands his trait reform. Talking to him last week he says he is almost done laying the groundwork for it. It's a bloody pain in the interim, but I would rather keep the traits clean for now rather than having to do a massive untangling of some complex double-dispatched thing in the future :(

Once trait reform lands, I would love to be able to do:

let vec4 = (vec2a, vec2b).to_vec4();
let vec3 = (1.3, vec2a).to_vec3();
let mat3 = (vec3, (vec2a, 3.0).to_vec3(), vec3).to_mat3();

@emberian
Copy link
Collaborator

I don't think operator overloads are worth pain.

@brendanzab
Copy link
Collaborator

@cmr sadface

@porglezomp
Copy link

@cmr as a user, I think operator overloads are absolutely worth the pain.

@brendanzab
Copy link
Collaborator

I'm not sure of the current status of rust-lang/rust#17657 (this is what we need - we are so close)

@kvark
Copy link
Collaborator Author

kvark commented Feb 12, 2015

I believe the trait reform is done already? Unblocking.

@brendanzab
Copy link
Collaborator

I believe this is done!

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

No branches or pull requests

5 participants