Conversation
| # 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() |
There was a problem hiding this comment.
this may be a potentially big change and affect existing users. @Abhiram824 can you make it a flag and use self.env.sim.model.get_xml() by default? we should toggle the flag to use self.env.model.get_xml() only for robocasa for now. others can follow suit it they'd like
| self.env.hard_reset = True | ||
| self.reset() | ||
| self.env.hard_reset = reset_mode | ||
| if self.env.hard_reset is not True: |
There was a problem hiding this comment.
can you quickly investigate why we have this change?
There was a problem hiding this comment.
Im not 100% sure (I think you might've made this change), but it looks like logic is that we dont need the extra reset in the VisualizationWrapper because we already call reset right before we start collecting demonstrations.
I personally think we should undo this change though
There was a problem hiding this comment.
Agreed that if we can remove it, that would be best. Could you test that undoing this will still work for us?
There was a problem hiding this comment.
it still works, I reverted the code
| forward_jnt = None | ||
| forward_jnt_axis = None | ||
| for jnt in self.joint_names: | ||
| if "joint_mobile_forward" in jnt: | ||
| forward_jnt = jnt | ||
| forward_jnt_axis = self.sim.model.jnt_axis[self.sim.model.joint_name2id(jnt)] | ||
| break |
There was a problem hiding this comment.
@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_axis change at some point?
There was a problem hiding this comment.
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.
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 be action = np.copy([action[i] for i in [1, 0, 2]]) instead. This will be cleaner!
There was a problem hiding this comment.
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
snasiriany
left a comment
snasiriany
left a comment
There was a problem hiding this comment.
leaving clarification questions and comments
|
Code looks good, left some minor comments. Let's also get a review from @kevin-thankyou-lin |
Ill address your comments within 1-2 hours and ping him to review as well! |
3 participants
What this does
Changes to make robosuite compatible with latest updates to robocasa features