Skip to content
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

Feat/robot fixes #1075

Open
wants to merge 25 commits into
base: og-develop
Choose a base branch
from
Open

Feat/robot fixes #1075

wants to merge 25 commits into from

Conversation

cremebrule
Copy link
Contributor

No description provided.

@cremebrule cremebrule requested a review from ChengshuLi January 8, 2025 02:14
pre-commit-ci bot and others added 20 commits January 8, 2025 02:14
# Conflicts:
#	omnigibson/examples/robots/curobo_example.py
#	omnigibson/robots/manipulation_robot.py
#	tests/test_curobo.py
# Conflicts:
#	omnigibson/action_primitives/starter_semantic_action_primitives.py
#	omnigibson/configs/tiago_primitives.yaml
#	omnigibson/envs/data_wrapper.py
#	omnigibson/examples/robots/curobo_example.py
#	omnigibson/robots/fetch.py
#	tests/test_curobo.py
Copy link
Member

@ChengshuLi ChengshuLi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a few comments, looks great overall! Thanks!

Comment on lines +178 to +179
def eef_to_fingertip_lengths(self):
return {arm: {name: 0.087 for name in names} for arm, names in self.finger_link_names.items()}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm is this 0.087 actually correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmmm you're right -- i had just directly converted finger_lengths to eef_to_fingertip_length, which is not correct.

Maybe I can remove this for now? It isn't used anywhere in the code currently, so perhaps it should be (re-)tuned later once there's an actual use case for A1

Comment on lines +241 to +242
def eef_to_fingertip_lengths(self):
return {arm: {name: 0.1 for name in names} for arm, names in self.finger_link_names.items()}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should also dynamically compute this if parallel gripper is used?

@@ -276,11 +286,11 @@ def teleop_rotation_offset(self):
return {self.default_arm: self._teleop_rotation_offset}

@property
def assisted_grasp_start_points(self):
def _assisted_grasp_start_points(self):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here

Comment on lines +27 to +28
def eef_to_fingertip_lengths(self):
return {arm: {name: 0.15 for name in names} for arm, names in self.finger_link_names.items()}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here

@@ -40,15 +40,15 @@ def curobo_path(self):
return os.path.join(gm.ASSET_PATH, "models/franka/franka_mounted_description_curobo.yaml")

@property
def assisted_grasp_start_points(self):
def _assisted_grasp_start_points(self):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here

Comment on lines +58 to +59
def eef_to_fingertip_lengths(self):
return {arm: {name: 0.04 for name in names} for arm, names in self.finger_link_names.items()}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same? this one is parallel gripper as well?

Comment on lines -187 to -189
# Reset the environment
env.scene.reset()

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm why do we need to remove this?

Comment on lines +268 to +270
if not is_parallel_jaw:
self._default_ag_start_points[arm] = None
self._default_ag_end_points[arm] = None
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm should we assert is_parallel_jaw here?


# Now, only keep points that are above the parent max z by 20% for inferring y values
finger_range = finger_max_z - finger_parent_max_z
valid_idxs = th.where(finger_pts[:, 2] > (finger_parent_max_z + finger_range * 0.2))[0]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe make 0.2 and 0.8 into macros?

Comment on lines +351 to +354
grasp_pts = (grasp_pts @ finger_to_eef_tf.T)[:, :3]
grasping_points = [
GraspingPoint(link_name=finger_link.body_name, position=grasp_pt) for grasp_pt in grasp_pts
]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we add a comment here saying "we convert the grasp_pts from the eef frame to the finger frame?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants