-
Notifications
You must be signed in to change notification settings - Fork 665
Robocasa dev #792
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
Robocasa dev #792
Changes from 11 commits
593c631
9a17d12
9548a5a
8da298d
b8b5d7f
fabdcb2
de413bf
0dd288a
bd69eea
eba8c31
372266c
89d5c10
f577519
1792eb3
7c15939
e1f5f55
7736d32
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -69,10 +69,13 @@ def _start_new_episode(self): | |
|
|
||
| # save the task instance (will be saved on the first env interaction) | ||
|
|
||
| # NOTE: was previously self.env.model.get_xml(). Was causing the following issue in rare cases: | ||
| # NOTE: was originally set to self.env.model.get_xml() | ||
| # That was causing the following issue in rare cases: | ||
| # ValueError: Error: eigenvalues of mesh inertia violate A + B >= C | ||
| # switching to self.env.sim.model.get_xml() does not create this issue | ||
| self._current_task_instance_xml = self.env.sim.model.get_xml() | ||
| # then, switched to self.env.sim.model.get_xml() which does not create this issue | ||
| # however, that leads to subtle changes in the physics, such as fixture doors being harder to close | ||
| # so, reverting back to self.env.model.get_xml() | ||
| self._current_task_instance_xml = self.env.model.get_xml() | ||
|
||
| self._current_task_instance_state = np.array(self.env.sim.get_state().flatten()) | ||
|
|
||
| # trick for ensuring that we can play MuJoCo demonstrations back | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -75,9 +75,10 @@ def __init__(self, env, indicator_configs=None): | |
|
|
||
| # Conduct a (hard) reset to make sure visualization changes propagate | ||
| reset_mode = self.env.hard_reset | ||
| self.env.hard_reset = True | ||
| self.reset() | ||
| self.env.hard_reset = reset_mode | ||
| if self.env.hard_reset is not True: | ||
|
||
| self.env.hard_reset = True | ||
| self.reset() | ||
| self.env.hard_reset = reset_mode | ||
|
|
||
| def get_indicator_names(self): | ||
| """ | ||
|
|
||
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.
@Abhiram824 can you give some context for what's happening in this code block?
Good to also write some comments in the code to give other users context.
Did
forward_jnt_axischange at some point?Uh oh!
There was an error while loading. Please reload this page.
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.
This is the main change between our branch and master branch. Basically previously in the Omron xml, the forward joint moved along the y axis (left to right). But with Kevins change to master it now moves along the x-axis (forward-backward).
Previously, when the forward joint moved along the y-axis, the code reordered the action, but with the latest changes there is no need to reorder the action.
All this code does is dynamically check which axis the forward joint is moving along, so that we know whether or not to reorder the action.
This is needed for playback because our datasets use the xmls with the old forward axis convention. I cannot simply change the xmls in our datasets because the state arrays (which contain joint qpos) will be corrupted.
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.
It seems like a bit of a hack, because it's explicitly looking for a joined named
joint_mobile_forward. Since this is pretty specific code, I would create a separate function for this, just for the purposes of "post-processing" the action to beaction = np.copy([action[i] for i in [1, 0, 2]])instead. This will be cleaner!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 agree that its very ugly, but for the time being i dont think there is much we can do about it. I will move it to a different function though
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.
updated