-
Notifications
You must be signed in to change notification settings - Fork 57
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
add rotation around origin for Rotation class #245
base: main
Are you sure you want to change the base?
Conversation
def get_rotation_and_translation(self, origin: Coordinate = Coordinate.zero()): | ||
# rotation matrices for each axis |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking about storing the origin as an attribute on the class itself
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good idea
|
||
return rotation_matrix, translation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we need to return the rotation because otherwise it would break get_absolute_rotation and other functions elsewhere
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm idk how to do this without a 4x4 matrix or a second translation matrix in addition to the rotation matrix
4x4 would just add the translation as a column, but would then have to be implemented in get_absolute_rotation and others?
rotation_matrix_4x4 = [
[rotation_matrix_3x3[0][0], rotation_matrix_3x3[0][1], rotation_matrix_3x3[0][2], translation.x],
[rotation_matrix_3x3[1][0], rotation_matrix_3x3[1][1], rotation_matrix_3x3[1][2], translation.y],
[rotation_matrix_3x3[2][0], rotation_matrix_3x3[2][1], rotation_matrix_3x3[2][2], translation.z],
[0, 0, 0, 1] # Last row for homogeneous transformation
]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think this method should just return the rotation matrix, no translation. translation can be applied when actually using the rotation matrix (in get_absolute_location i think)
not sure rotation.py is the right home for the test
@fderop for reference, i think u were thinking about this too?