[BE] - fix inconsistent base_rot property #2085
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…sts. Activate and fix skipped humanoid unit tests.
aclegg3
requested review from
xavierpuigf,
zephirefaith,
jimmytyyang and
0mdc
facebook-github-bot
added
the
CLA Signed
0mdc
approved these changes
Oct 7, 2024
| assert kin_humanoid.get_robot_sim_id() == 1 # 0 is the ground plane | ||
| assert ( | ||
| kin_humanoid.get_robot_sim_id() == 2 | ||
| ) # 0 is the stage and 1 is the ground plane |
There was a problem hiding this comment.
We should use constants here for clarity (enum?).
There was a problem hiding this comment.
This isn't a general thing. The stage_id is a constant from sim, but the others are just situational here.
| produce_debug_video = False | ||
| observations = [] | ||
| cfg_settings = default_sim_settings.copy() | ||
| cfg_settings["scene"] = "NONE" |
| "color", | ||
| "test_base_rot", | ||
| open_vid=True, | ||
| ) |
| angle += mn.math.pi * 2 | ||
| # NOTE: This final fmod ensures that large angles are mapped back into the -2pi, 2pi range. | ||
| angle = mn.math.fmod(angle, 2 * mn.math.pi) | ||
| return angle |
There was a problem hiding this comment.
In general, it would be more robust to force usage of forward vectors or rotation quaternions.
There was a problem hiding this comment.
I agree that we are baking in lots of assumptions using a scalar angle about Y axis with implicitly defined origin.
Happy to re-design this part of the system, but pretty deeply embedded, so just trying to get it to behave consistently for now.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation and Context
This PR addresses a long-standing bug in the
articulated_agent.base_rotproperty API.The bug: 🐛
Rotations are stored internally as
Magnum.Quaternionobjects. Thebase_rotproperty operates in scalar space, namely the angle of rotation about the Y axis. This means that every call to the setter creates a Quaternion from a rotation angle and Y axis, while the getter deconstructs a Quaternion into an angle using 2arccos.While the result is correct in some sense, it does not account for:
This results in the potential for several nasty bugs. The most obvious being that getting an angle after settings does not return the same scalar value (it could be a correct, but different angle) and could return an erroneous value (e.g. if the quaternion axis is inverted during normalization).
The solution:
This PR modifies the getter to:
Together, these changes result in a few nice guarantees from the function:
-pi ==pi. This means, e.g.base_rot = pi/2guaranteesbase_rot == pi/2.base_rot = base_rot + 0.1with the expected outcome of continual rotation.How Has This Been Tested
Added unit tests for robot wrapper and humanoid wrapper.
This includes changes from #2084. Note that CI was failing there and is now passing.
test_base_rot.mp4
Types of changes
Checklist