-
Notifications
You must be signed in to change notification settings - Fork 505
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
Embodied Unoccluded Navmesh Snap util #1949
Embodied Unoccluded Navmesh Snap util #1949
Conversation
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.
LGTM! Would be great to have a unit test at some point. Thanks, Alex!
start_rotation = agent_embodiment.base_rot | ||
|
||
agent_embodiment.base_pos = batch_sample[0] | ||
agent_embodiment.base_rot = desired_angle |
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 am not sure I understand orientation noise in these functions. Should it apply to desired_angle here/agent orientation basically?
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.
Yeah, basically this is an option to sample random noisy orientations from a gaussian centered on the forward facing direction (robot facing toward the target).
The final, valid orientation is part of the return Tuple.
We have to set the agent orientation during the sampling process such that the collision checking can validate it.
This is helpful because it may be the case that some parts of the scene are tight enough that the robot will only fit into the space within a limited range of orientations (e.g. walking between sofa and coffee table). Sampling accurately in these kinds of poses requires many iterations and a high degree of noise.
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.
LGTM!
… add navmesh_offsets to params
… BaseVelNonCylinderAction
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.
Looks good from my context! Thanks for improving typing and adding detailed usage documentation.
orientation_noise: float = 0, | ||
max_orientation_samples: int = 5, | ||
data_out: Dict[Any, Any] = None, | ||
) -> Tuple[mn.Vector3, float, bool]: |
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.
Nit: It's possible to return null values for the first two parameters.
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.
LGTM! Thanks for adding the test!
@@ -597,6 +608,7 @@ def collision_check( | |||
""" | |||
# Get the offset positions | |||
num_check_cylinder = len(self._navmesh_offset) | |||
# TODO: height 0 is not a good assumption in general. This must be changed to query current navmesh height. |
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.
any reason to not do this right now?
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 would be a breaking change in behavior for the action. So far everything in this PR is transparent. I didn't want to potentially change downstream behaviors.
…noccluded_navmesh_snap_util
* add new method embodied_unoccluded_navmesh_snap and test * update APIs with ignore_object_ids and fix some doc typos * refactor to unify MobileManipulatorParams and SpotParams, add navmesh_offsets to params * remove navmesh_offset from config in favor of params and refactor the BaseVelNonCylinderAction
* add new method embodied_unoccluded_navmesh_snap and test * update APIs with ignore_object_ids and fix some doc typos * refactor to unify MobileManipulatorParams and SpotParams, add navmesh_offsets to params * remove navmesh_offset from config in favor of params and refactor the BaseVelNonCylinderAction
* add new method embodied_unoccluded_navmesh_snap and test * update APIs with ignore_object_ids and fix some doc typos * refactor to unify MobileManipulatorParams and SpotParams, add navmesh_offsets to params * remove navmesh_offset from config in favor of params and refactor the BaseVelNonCylinderAction
* add new method embodied_unoccluded_navmesh_snap and test * update APIs with ignore_object_ids and fix some doc typos * refactor to unify MobileManipulatorParams and SpotParams, add navmesh_offsets to params * remove navmesh_offset from config in favor of params and refactor the BaseVelNonCylinderAction
* add new method embodied_unoccluded_navmesh_snap and test * update APIs with ignore_object_ids and fix some doc typos * refactor to unify MobileManipulatorParams and SpotParams, add navmesh_offsets to params * remove navmesh_offset from config in favor of params and refactor the BaseVelNonCylinderAction
Motivation and Context
This PR adds the
embodied_unoccluded_navmesh_snap
function from #1778 without the proposed full-stack refactor. See that PR for video and context. The implementation remains largely unchanged. The goal is to leverage this function downstream and retroactively refactor existing code in later changes.TL;DR: we want to able to place embodied agents in the world nearby a particular object or furniture asset via the navmesh while considering the constraints of their embodiment and also preventing naive placement errors such as locations on the other side of a thin wall.
Accompanying refactors:
SpotParams
intoMobileManipulatorParams
and addednavmesh_offsets
parameter.navmesh_offset
from config in favor of paramsHow Has This Been Tested
Added a unit test.
Types of changes
Checklist