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

[breaking] - NavMeshSettings refactor #2111

Merged
merged 8 commits into from
May 31, 2023
Merged

Conversation

aclegg3
Copy link
Contributor

@aclegg3 aclegg3 commented May 17, 2023

Motivation and Context

Several recent needs have exposed the brittleness of current NavMesh defaults for Simulator reconfiguration. This PR aims to provide much more flexibility in configuration and reduce unnecessary compute while remaining simple for default use-cases.

This is a breaking change.

Currently, when a Simulator is initialized, the default agent radius and height are used to trigger a re-compute of the NavMesh if any other settings are modified. This cannot be turned off and other settings cannot be modified without explicitly recomputing again after instantiation which can be costly in aggregate and occasionally block use-ability altogether (e.g. if some property of the stage causes navmesh computation to fail).

After this change:

  1. An optional NavMeshSettings object is added to SimulatorConfiguration. This can be configured by user code before the Simulator is instantiated and will trigger NavMesh re-computation as necessary. If omitted, the NavMesh will not be constructed (however, the scene may still load one from file).
  2. The option to include RigidObjects with MotionType STATIC has been migrated into NavMeshSettings to unify the API and eliminate the need for variable chains to configure this feature.
  3. Backwards compatible default behavior (agent specific default NavMesh re-computation) is now realized through and configurable in utils.settings.py. (NOTE this API documentation is not updated until the PR is merged, see the file changes for details).
  4. (breaking deprication) Any automated search for navmeshes matching a specific raw asset has been removed from Simulator.py. This is outdated and should be done in configuration files or the stage loader rather than shunted into the initialization step.

Habitat-lab will need to be updated to use the new API. facebookresearch/habitat-lab#1351

Should cover the needs of #2104.

How Has This Been Tested

Updated tests should cover this.
I did additional local testing with python viewer to double check.

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 May 17, 2023
Copy link
Contributor

@jturner65 jturner65 left a comment

Choose a reason for hiding this comment

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

LGTM

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.

LGTM! I left some minor remarks.

#include <string>

#include "esp/core/Esp.h"
#include "esp/nav/PathFinder.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

This should probably be forward-declared as this class is widely used.

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, I thought so too. However, the compiler does not like the forward declaration here, so I needed the full the include.

src/esp/sim/SimulatorConfiguration.h Outdated Show resolved Hide resolved
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