Now there is only one one

[hayashi-stl]

This fixes the multiple one found issue (one function mentioned in #354) with attempting to use Matrix4::one() or some other matrix's one() function when importing the prelude, which includes both Transform and One. The changes:

• Transform::one() is gone
• Transform is now a subtrait of One
• Decomposed implements One.
• EuclideanRotation is a new trait specifically for implementers of Rotation<P> for exactly one P. This allows Decomposed to implement One without running into unconstrained type parameter issues. All current implementers of Rotation in the cgmath crate implement EuclideanRotation.
• Decomposed implements Mul so it can implement One.

I know there was an attempt to do it in #386, but it was abandoned and closed.

There seems to be a few extra changes from running cargo fmt.

[kvark]

What is the problem (in more detail) that happens if you don't add the new trait?

[hayashi-stl]

If I wrote:

impl<P: EuclideanSpace, R: Rotation<P>> One for Decomposed<P::Diff, R>
where
    P::Scalar: BaseFloat,
    P::Diff: VectorSpace,
{
    fn one() -> Self {
        Decomposed {
            scale: P::Scalar::one(),
            rot: R::one(),
            disp: P::Diff::zero(),
        }
    }
}

then the compiler will complain that P is unconstrained.

This doesn't currently happen in cgmath, but imagine that there was some type AnotherPoint2 that implemented EuclideanSpace<Diff = Vector2<f64>>. Also imagine that some type RotationAny implemented both Rotation<Point2<f64>> and Rotation<AnotherPoint2<f64>>. Then Decomposed<Vector2<f64>, RotationAny> implements One twice...

Actually, I just realized that in this case, since R::one() is coming from One and not Rotation<P>, so I can rewrite the body as

Decomposed {
    scale: P::Diff::Scalar::one(),
    rot: <R as One>::one(),
    disp: P::Diff::zero(),
}

and see that there is no dependence on P. I can almost replace P: EuclideanSpace with V: VectorSpace and replace P::Diff with V, but that won't deal with Rotation requiring P as a parameter. So here are some possibilities:

• The current pull request, with its upsides and downsides.
• Write it as impl<V: VectorSpace, R: One> One for Decomposed<V, R>. This eliminates the EuclideanRotation trait, but is also more general on R than I'd like.
• Move the associated type of EuclideanRotation into Rotation and remove the parameter on Rotation. This would make sense unless we want it to be possible for a type to implement Rotation for multiple Euclidean spaces. This eliminates the extra trait and allows me to write impl<P: EuclideanSpace, R: Rotation<Euclidean=P>> One for Decomposed<P::Diff, R>, constraining P. However, it could cause a lot of breakage.
• Write an RFC for relaxing the rules on type parameter constraints. In particular, it would be nice to write something like impl<V: VectorSpace, R: exists<P: EuclideanSpace> Rotation<P>> One for Decomposed<V, R> and simply not be able to make the implementation dependent on P. I don't know the chances of this getting approved.

[kvark]

Thank you for elaborating here! I like the (2) and (3) choices:

Write it as impl<V: VectorSpace, R: One> One for Decomposed<V, R>. This eliminates the EuclideanRotation trait, but is also more general on R than I'd like.

What is the concern here, exactly? Is the generosity going to be a footgun in some situation?

Move the associated type of EuclideanRotation into Rotation and remove the parameter on Rotation. This would make sense unless we want it to be possible for a type to implement Rotation for multiple Euclidean spaces.

cgmath needs to steer away from being generic into being more practical, so this constraint seems reasonable to me.

[hayashi-stl]

It turns out that the second option is blocked by a call to rotate_vector in the implementation of Mul for Decomposed. It would actually require R to implement Rotation. So option 3 is taken. I noticed a few other traits like Rotation2, Rotation3, and Transform that have type parameters that should be associated types instead. It wouldn't make sense for the same type to implement both Rotation2<f32> and Rotation2<f64>, for example.

[kvark]

I noticed a few other traits like Rotation2, Rotation3, and Transform that have type parameters that should be associated types instead. It wouldn't make sense for the same type to implement both Rotation2 and Rotation2, for example.

yes, agreed!

[hayashi-stl]

Should I save fixing those for another pull request or combine it with this one?

[kvark]

I think it would make sense to combine them, if you can. Thank you!

[hayashi-stl]

It turns out that Matrix3<S> implements both Transform<Point2<S>> and Transform<Point3<S>> since one of them allows translations and the other doesn't. However, Rotation2, Rotation3, Transform2, and Transform3 didn't need their bounds.

[kvark]

@aloucks @elrnv would you want to help reviewing these changes? They are pretty serious