-
Notifications
You must be signed in to change notification settings - Fork 273
refactor: remove habitat_sim dependency in motor_policies.py #79
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
Conversation
| # TODO could vectorize this | ||
| rotated_locs = [ | ||
| hab_utils.quat_rotate_vector(inverse_quaternion_rotation, point) | ||
| qt.rotate_vectors(inverse_quaternion_rotation, point) |
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.
note: I think this could become qt.rotate_vectors(inverse_quaternion_rotation, adjusted_prev_locs) based on qt.rotate_vectors documentation: https://quaternion.readthedocs.io/en/latest/quaternion/#quaternion.rotate_vectors
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 removed the extraneous list comprehension, which is now qt.rotate_vectors. This function is intended to be applied to multiple vectors and will return the correct shape.
vkakerbeck
left a comment
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.
Really nice, thanks for the update! Just one optional comment that could be useful for future reference (since we've been tripped up by this several times in the past. Could also add the note here https://thousandbrainsproject.readme.io/docs/common-issues-and-how-to-fix-them#quaternions)
| ) | ||
| return inverse_quaternion_rotation | ||
| [w, x, y, z] = qt.as_float_array(self.state[self.agent_id]["rotation"]) | ||
| [x, y, z, w] = rot.from_quat([x, y, z, w]).inv().as_quat() |
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.
Maybe add a comment here clarifying that magnum and scipy rotation use different quaternion conventions.
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.
Yeah as in quaternion and scipy right (not magnum)?
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.
yes, sorry thats what I meant
nielsleadholm
left a comment
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.
Awesome, thanks for sorting this Tristan.
| ) | ||
| return inverse_quaternion_rotation | ||
| [w, x, y, z] = qt.as_float_array(self.state[self.agent_id]["rotation"]) | ||
| [x, y, z, w] = rot.from_quat([x, y, z, w]).inv().as_quat() |
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.
Yeah as in quaternion and scipy right (not magnum)?
This pull request makes progress on decoupling Monty from HabitatSim (identified in #52). Specifically, the
habitat_simdependency withinmotor_policies.pyis removed.These temporary tests (excluded from the pull request) guided the changes in addition to the standard test suite. Notably,
Rotationexpects anx, y, z, wquaternion format.