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

"Ontop" utility #1835

Merged
merged 5 commits into from
Mar 8, 2024
Merged

"Ontop" utility #1835

merged 5 commits into from
Mar 8, 2024

Conversation

aclegg3
Copy link
Contributor

@aclegg3 aclegg3 commented Mar 6, 2024

Motivation and Context

This PR adds a utility for querying which objects are "ontop" of a subject object. Initial prototype in #1770.

Here, "ontop" means the objects have contacting collision points with a close to vertical contact normal. The epsilon threshold for the dot product is a controllable variable, defaulting to 0.75.

The function inputs can be ManagedObjects or object_id integers. If the object_id provided refers to an ArticulatedLink, only that link is checked. Conversely, if a ManagedArticulatedObject is provided, all links are checked.

How Has This Been Tested

A pytest is added using all input variant in a ReplicaCAD scene.

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

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

@facebook-github-bot facebook-github-bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label Mar 6, 2024
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.

Left some comments, but otherwise LGTM!

link_id is None or link_id == cp.link_id_b
):
contacting_obj_id = cp.object_id_a
obj_is_b = True
Copy link
Contributor

@0mdc 0mdc Mar 6, 2024

Choose a reason for hiding this comment

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

I'm probably misunderstanding what's going on here.

Wouldn't we want to exclude the object itself from the set of objects on top of it? Would a continue be better suited here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, possibly misunderstanding. For clarity, we don't know if our object is the first or second in the contact pair. This statement just figures out which one it is if either and prepares to invert the normal if necessary.

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 other thoughts.

objectA = subject_object

if do_collision_detection:
sim.perform_discrete_collision_detection()
Copy link
Contributor

@0mdc 0mdc Mar 6, 2024

Choose a reason for hiding this comment

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

Edit: wrong line.

sim.get_physics_contact_points() is expensive. We should probably cache it at sim-level (and invalidate the cache when a fresh collision detection pass is done).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this really so expensive to warrant the effort and complexity? I haven't profiled, but it's just iterating over the internal structures and collecting the already computed data into a new structure to return. 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If this does end up being too costly we'd likely add the cache in sim, so I'll defer this to a future PR.

:param do_collision_detection: If True, a fresh discrete collision detection is run before the contact point query. Pass False to skip if a recent sim step or pre-process has run a collision detection pass on the current state.
:param vertical_normal_error_threshold: The allowed error in normal alignment for a contact point to be considered "vertical" for this check. Functionally, if dot(contact normal, Y) <= threshold, the contact is ignored.

:return: a list of integer object_ids for the set of objects "ontop" of objectA.
Copy link
Contributor

Choose a reason for hiding this comment

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

Will users ever need to check whether a specific object_b is on top of object_a, and get a simple bool as a result? A variant function may be cheaper if they need to do this comparison often.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It could be a bit cheaper, but you'd still need to look through every contact point, so I don't think it would be much faster. I guess the advantage would the option for early abort.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll leave this to a future PR. Early abort and a check against provided object_id should be easy enough to add in a small refactor if we want it.

Copy link
Contributor

@0mdc 0mdc Mar 8, 2024

Choose a reason for hiding this comment

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

Sounds good.
My worry is that users would also have to find the desired object within the results - the search itself could be called in a nested loop, making this overall process inefficient.
I'm fine with doing this later - this is already a good increment.

@aclegg3
Copy link
Contributor Author

aclegg3 commented Mar 8, 2024

Left some comments, but otherwise LGTM!

Thanks for the review. Good points. I've answered questions and pushed the changes you suggested. Some items deferred for now. I'll merge this once the tests pass.

@aclegg3 aclegg3 merged commit 8e2d957 into main Mar 8, 2024
4 checks passed
@aclegg3 aclegg3 deleted the alex-03_05-ontop branch March 8, 2024 20:11
dannymcy pushed a commit to dannymcy/habitat-lab that referenced this pull request Jul 8, 2024
* add ontop function and test

* refactor utils to use python case
HHYHRHY pushed a commit to SgtVincent/habitat-lab that referenced this pull request Aug 31, 2024
* add ontop function and test

* refactor utils to use python case
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.

3 participants