Integrate KRM with RearrangeSim #1927
Conversation
facebook-github-bot
added
the
CLA Signed
|
CI was failing because the rearrange v1 dataset was mapping object placements to Receptacle.name instead of Receptacle.unique_name. |
| self._sim.kinematic_relationship_manager.relationship_graph.remove_obj_relations( | ||
| snap_obj_id, parents_only=True | ||
| ) | ||
| # update root parent transforms so new parent state is registered |
There was a problem hiding this comment.
Why do we need this? I understand that the line above removes the relationship with the parent. Won't the root parent transforms stay the same when we pick an object? Why do we need to store them in prev_root_obj_state?
There was a problem hiding this comment.
When we disconnect an object from its parents, the object becomes a root parent. We need to cache the new root parent's global transform so it can be applied to update any children.
This is the essential logic which allows, for example, picking up a tray with cups on it and moving the cups along with the tray. The tray is a new root parent and its transform must be tracked.
| @@ -109,6 +112,7 @@ def __init__(self, config: "DictConfig"): | |||
| self.viz_ids: Dict[Any, Any] = defaultdict(lambda: None) | |||
| self._handle_to_object_id: Dict[str, int] = {} | |||
| self._markers: Dict[str, MarkerInfo] = {} | |||
| self._targets: Dict[str, mn.Matrix4] = {} | |||
There was a problem hiding this comment.
What are those _targets? How are they relevant to KRM?
There was a problem hiding this comment.
This was a clean-up refactor, not related to KRM. IMO, self. variables should all be declared in init(), even if they are initialized in an internal function. This allows any comments and typing to be co-located. More straightforward paradigm than trying to hunt around for the first declaration of the variable in some internal function.
| ) | ||
| if done: | ||
| break | ||
| for _ in range(2): |
There was a problem hiding this comment.
Couldn't removing the with env cause segfaults like in the llm codebase?
There was a problem hiding this comment.
This is a test file, so it won't get pulled in elsewhere. I also added an explicit close() afterward. I don't think this change really does anything, but I thought it was more clear what was happening.
targets variable: I answered that here. TL;DR: unrelated clean-up refactor v2 dataset: details on the changes are here. TL;DR: the v1 dataset did not comply to the required spec for |
* integrate KRM with RearrangeSim * replace all references to reaplica_cad/rearrange/v1/ datasets with v2/ paths instead
* integrate KRM with RearrangeSim * replace all references to reaplica_cad/rearrange/v1/ datasets with v2/ paths instead
* integrate KRM with RearrangeSim * replace all references to reaplica_cad/rearrange/v1/ datasets with v2/ paths instead
Motivation and Context
This PR integrates the
KinematicRelationshipManagerfrom #1909 into theRearrangeSimforkinematic_mode.When
kinematic_modeis set:updateloopRearrangeGraspManagersevers kinematic relationships when a grasp is triggered.Also adds some typing and docstrings style refactors.
Also updates the test rearrange episode datasets across the stack to "v2" following facebookresearch/habitat-sim#2381
TODO:
How Has This Been Tested
Not explicitly tested yet. Existing integration tests for RearrangeSim only.
Types of changes
Checklist