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

Make NavMesh recomputation for non-default agent params optional #1724

Merged
merged 8 commits into from
Apr 19, 2022

Conversation

aclegg3
Copy link
Contributor

@aclegg3 aclegg3 commented Apr 13, 2022

Motivation and Context

simulator.py reconfigure() automatically recomputes the NavMesh if default_agent has non-default parameters. Generally this makes sense because most of our saved NavMeshes are generated for the default agent. However, users may want to skip this if they've generated their own custom NavMeshes as in ReplicaCAD dataset for Hab2.0 rearrangement tasks.

This PR primarily adds a new config option recompute_navmesh_on_agent_params which defaults to previous behavior, but if toggled off allows skipping automatic recomputation.
Edit: This PR adds cache, query, and serialization of navmesh settings accompanying any programmatically computed or loaded NavMesh. Existing NavMeshes computed before this PR are assumed to use default settings. If agent params are different from cached settings for the currently loaded NavMesh, it will be recomputed to match current agent settings.

In addition, I cleaned up documentation for SimulatorConfiguration and removed two unused legacy fields:

  • defaultCameraUuid
  • compressTextures

How Has This Been Tested

CI passes, new tests for NavMesh settings cache and comparators. Built the docs locally.

Types of changes

  • Docs change / refactoring / dependency upgrade
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have completed my CLA (see CONTRIBUTING)
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@facebook-github-bot facebook-github-bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label Apr 13, 2022
Comment on lines -24 to -25
std::string defaultCameraUuid = "rgba_camera";
bool compressTextures = false;
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 these do have a purpose, I couldn't find it.

@erikwijmans
Copy link
Contributor

IMO if we are adding something, we should just save the navmesh settings into the navmesh itself and then check against those. The navmesh is saved with a version so we should be able to save this with backwards compatibility and we just use the default navmesh settings.

@Skylion007
Copy link
Contributor

I agree with @erikwijmans , we should save the agent size with the navmesh. It seems like an integral property of the navmesh and the only reason we aren't doing it is technical debt.

@erikwijmans
Copy link
Contributor

@aclegg3 @Skylion007 I pushed a change set to save the params into the navmesh.

Copy link
Contributor Author

@aclegg3 aclegg3 left a comment

Choose a reason for hiding this comment

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

Looks good, thanks for the upgrade @erikwijmans.

@aclegg3 aclegg3 merged commit 9a18583 into main Apr 19, 2022
@aclegg3 aclegg3 deleted the recompute_navmesh_on_agent_params branch April 19, 2022 15:50
@aclegg3 aclegg3 mentioned this pull request Apr 21, 2022
11 tasks
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.

None yet

4 participants