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

Remove Rotation and Transform traits #302

Closed
wants to merge 4 commits into from
Closed

Conversation

brendanzab
Copy link
Collaborator

Closes #143, #194

Given the interactions on IRC, they cause more confusion than perhaps they are worth. Perhaps we can re-add them in the future with a better API, but for now I would like to focus more on simplicity.

The following traits and types have been removed:

  • Rotation types and structs: Rotation, Rotation2, Rotation3, Basis3, Basis3
  • Transform types and structs: Transform, Transform2, Transform3, Decomposed, AffineMatrix3

@kvark
Copy link
Collaborator

kvark commented Mar 24, 2016

I'm obviously not too happy about the Transform stuff removal.

@brendanzab
Copy link
Collaborator Author

I'll think about this some more. I think some common traits would be useful, but I definitely don't want to leave it in its current state. Have you been leaning on these heavily?

@kvark
Copy link
Collaborator

kvark commented Mar 24, 2016

I've been using Decomposed and Transform for the "claymore" engine/game, and part of it ended up going to gfx_scene: https://github.com/gfx-rs/gfx_scene/blob/master/src/scene/cull.rs#L59

Of course, gfx_phase and gfx_scene need to be heavily refactored (if that's even possible without total redesign) to work with the latest gfx, but I didn't plan to change the way it interacts with cgmath.

@brendanzab
Copy link
Collaborator Author

Cheers, thanks for showing me!

@mhintz
Copy link
Collaborator

mhintz commented Apr 8, 2016

I would support a redesign here - my experiences using cgmath have been positive, except for the fact that it feels like I often need to import several traits from the library just to get the expected basic functionality from a math library. I've started just including use cgmath::* wherever I need it. Removing some traits would help, and it would also make the documentation substantially clearer and easier to navigate. The overall comprehensibility of the library would be improved, as well.

I support this also because I recently ran into a big rat's nest when I needed to combine a rotation (Basis3) and a translation (Vector3) into a single 4x4 matrix. After spending a long time looking through the documentation of Basis3 for a way to convert it to a 4x4 matrix, I settled on this rather clunky formulation:

let translation = Matrix4::from_translation(object_position);
// Matrix3::from_rotation(x: Basis3) would be useful here:
let rotation: Matrix3<T> = object_rotation.into();
// Matrix4::from(object_rotation.into()) would fail here:
translation * Matrix4::from(rotation) // -> final matrix

The downside of removing the Basis structs is that I do like the guarantee of orthonormality...

@brendanzab
Copy link
Collaborator Author

Note that I've also added cgmath::prelude::* in the 0.8.0 release, which cuts down individual importing of traits.

@nbennet
Copy link

nbennet commented Apr 18, 2016

Is this still being considered? I'd rather see it refactored rather than removed. Transform, Rotation and Decomposed are definitely useful.

What about only removing the traits and renaming/refactoring the structs? Perhaps rename Decomposed to Transform{2|3} and move the trait functions from both Transform and Rotation.

Decomposed<Vector3, R> to struct Transform3<Vector3, Quaternion>
Decomposed<Vector2, R> to struct Transform2<Vector2, Quaternion>

@brendanzab
Copy link
Collaborator Author

I'm actually thinking of going with Isometry like nalgebra.

@brendanzab
Copy link
Collaborator Author

One tricky thing is Matrices are can be treated interpreted in different ways. A Matrix3 could either be interpreted as a 2D homogeneous transformation matrix, or a 3D rotation matrix. So implementing transformation constructors on matrices can be confusing. That was the idea behind the Basis and AffineMatrix wrapper structs.

@kvark
Copy link
Collaborator

kvark commented Apr 18, 2016

@nbennet if you move the transform functions into the struct that comes from Decomposed, you'd not be able to use it in a generic interface where either Decomposed or Matrix4 would work. So the whole point of this abstraction will be ruined.

@bjz is this a problem though? Just implement both Transform2 and Transform3 for Matrix3 - problem solved.

@nbennet
Copy link

nbennet commented Apr 18, 2016

@kvark I don't disagree about the abstraction. I was only suggesting an alternative in order to prevent them from being removed entirely.

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

Successfully merging this pull request may close these issues.

None yet

4 participants