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

New function to generate humanoid walking motion #2067

Merged
merged 1 commit into from
Sep 6, 2024

Conversation

xavierpuigf
Copy link
Contributor

Motivation and Context

New method to select humanoid walking motion without changing humanoid pace. This change allows to better control humanoid using speed, thus providing better support for HITL and a controller more similar to the robot one.

How Has This Been Tested

Ran Base Velocity with it.

Types of changes

  • [Development] A pull request that add new features to the habitat-lab task and environment codebase. Development Pull Requests must be small (less that 500 lines of code change), have unit testing, very extensive documentation and examples. These are typically new tasks, environments, sensors, etc... The review process for these Pull Request is longer because these changes will be maintained by our core team of developers, so make sure your changes are easy to understand!

Checklist

  • [ x] My code follows the code style of this project.
  • [ x] I have updated the documentation if required.
  • [ x] I have read the CONTRIBUTING document.
  • [ x] I have completed my CLA (see CONTRIBUTING)
  • [ x] I have added tests to cover my changes if required.

@xavierpuigf xavierpuigf requested a review from 0mdc September 5, 2024 20:50
@facebook-github-bot facebook-github-bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label Sep 5, 2024
@xavierpuigf xavierpuigf requested a review from aclegg3 September 5, 2024 20:50
@@ -32,6 +32,23 @@
from typing import List, Tuple


def bin_search(v, a, b, target_value):
Copy link
Contributor

Choose a reason for hiding this comment

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

docstring?

@0mdc 0mdc force-pushed the xavierpuig/correct_human_speed branch from a351f98 to 1b8b462 Compare September 6, 2024 20:40
@@ -279,6 +296,80 @@ def calculate_walk_pose(
self.obj_transform_base = obj_transform_base @ rot_offset
self.joint_pose = joint_pose

def translate_and_rotate_with_gait(
Copy link
Contributor

Choose a reason for hiding this comment

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

This function is not being called anywhere. It probably should be the default from now on.

It would be worth replacing all calls to calculate_walk_pose_directional and deprecating it.

Copy link
Contributor

@0mdc 0mdc left a comment

Choose a reason for hiding this comment

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

Some more comments

@@ -279,6 +296,80 @@ def calculate_walk_pose(
self.obj_transform_base = obj_transform_base @ rot_offset
self.joint_pose = joint_pose

def translate_and_rotate_with_gait(
self, forward_delta: float, rot_delta: float
Copy link
Contributor

@0mdc 0mdc Sep 6, 2024

Choose a reason for hiding this comment

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

Because agent velocity is driven from config, we should consider having forward_delta a value between -1.0 and 1.0, and having this function multiply it with the agent's lin_speed.

If we do this, there would be a single source of truth to determine the humanoid agent's velocity.
This would eliminate any discrepancy caused by the implementer failing to read the right config field, and would ease agent config refactoring.

While we're at it, we could use the same velocity variable as the other agents.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This multiplication happens in the BaseVelocityAction, this way we have parity with how the robot moves around.
Here is where we calculate how much to move: https://github.com/facebookresearch/habitat-llm/blob/main_improve_planner/habitat_llm/agent/env/actions.py#L116

This is similar to the robot action, which computes delta and then transforms in here:
https://github.com/facebookresearch/habitat-lab/blob/main/habitat-lab/habitat/tasks/rearrange/actions/actions.py#L489

else:
right = mid - 1

return min(left, len(v) - 1)
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be worthwhile unit testing this one.

We could also use bisect to solve this.

Copy link
Contributor

@0mdc 0mdc left a comment

Choose a reason for hiding this comment

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

I think that this is good to go, because there is no side-effect.

Let's get this merged to get you unblocked, and revisit my concerns as we deploy the code across the codebase.

@xavierpuigf xavierpuigf merged commit c270a1f into main Sep 6, 2024
3 of 4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Do not delete this pull request or issue due to inactivity.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants