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

Recompute NavMesh with STATIC RigidObjects #538

Merged
merged 16 commits into from
Mar 27, 2020

Conversation

aclegg3
Copy link
Contributor

@aclegg3 aclegg3 commented Mar 21, 2020

Motivation and Context

Recast NavMesh is an efficient method for enforcing agent navigation constraints for static environments. However, it must be recomputed when the environment changes to remain valid. This feature enables the Simulator::recomputeNavMesh function to optionally include MotionType::STATIC RigidObjects as scene geometry.

To support this change:

  • RigidObject now stores its initialization template which can be queried from PhysicsManager. This allows object parameters (such as render mesh asset key) to be queried after a template may have been changed.
  • RigidObject::visualSceneNode_ can be queried.
  • added Simulator functions to get object template references.

How Has This Been Tested

New C++ unit test.

Example with viewer.cpp modification:
recompute_navmesh_sro

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.

@aclegg3 aclegg3 requested a review from bigbike March 21, 2020 00:27
@facebook-github-bot facebook-github-bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label Mar 21, 2020
@codecov
Copy link

codecov bot commented Mar 21, 2020

Codecov Report

Merging #538 into master will decrease coverage by 4.29%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #538      +/-   ##
==========================================
- Coverage   68.10%   63.80%   -4.30%     
==========================================
  Files          72      162      +90     
  Lines        3474     7880    +4406     
  Branches       84       84              
==========================================
+ Hits         2366     5028    +2662     
- Misses       1108     2852    +1744     
Flag Coverage Δ
#CPP 60.41% <100.00%> (?)
#JavaScript 10.00% <ø> (ø)
#Python 80.48% <100.00%> (ø)
Impacted Files Coverage Δ
src/esp/physics/PhysicsManager.h 80.00% <ø> (ø)
src/esp/physics/RigidObject.h 12.90% <ø> (ø)
src/esp/sim/Simulator.h 100.00% <ø> (ø)
habitat_sim/simulator.py 95.68% <100.00%> (ø)
src/esp/physics/RigidObject.cpp 44.26% <100.00%> (ø)
src/esp/physics/bullet/BulletRigidObject.cpp 76.39% <100.00%> (ø)
src/tests/SimTest.cpp 100.00% <100.00%> (ø)
src/esp/assets/PTexMeshData.cpp 0.00% <0.00%> (ø)
src/esp/sensor/Sensor.h 85.71% <0.00%> (ø)
src/esp/assets/ResourceManager.h 100.00% <0.00%> (ø)
... and 87 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9df61ab...9206a87. Read the comment docs.

@bigbike bigbike requested a review from mosra March 25, 2020 21:54
Copy link
Contributor

@bigbike bigbike 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 to me.
Hope the other reviewers can also take a look.

src/esp/sim/Simulator.cpp Outdated Show resolved Hide resolved
Copy link
Collaborator

@mosra mosra left a comment

Choose a reason for hiding this comment

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

One minor thing. Somehow I expected much more code to achieve this, positively surprised.

Once #542 is merged, the joining could get simplified quite a bit.

src/esp/assets/ResourceManager.cpp Outdated Show resolved Hide resolved
@aclegg3
Copy link
Contributor Author

aclegg3 commented Mar 27, 2020

Once #542 is merged, the joining could get simplified quite a bit.

Looks like we will want to derive our MeshData object from or replace it with Magnum::Trade::MeshData. Once that is done we can simplify the existing join functions.

I'll merge this for now as is and we can circle back with refactor PRs.

@aclegg3 aclegg3 merged commit bfbe9fc into master Mar 27, 2020
@aclegg3 aclegg3 deleted the recompute_navmesh_static_ros branch March 27, 2020 00:42
@mosra
Copy link
Collaborator

mosra commented Mar 27, 2020

I think it's flexible enough to fully replace current functionality. In case you're going to tackle this, feel free to ping me on Slack for design / usage questions :)

Ram81 pushed a commit to Ram81/habitat-web-sim that referenced this pull request Dec 10, 2020
* Added initialization template storage for RigidObject. 

* Added recomputeNavmesh with STATIC objects.

* Added object template getters to Simulator.
luoj1 pushed a commit to luoj1/habitat-sim that referenced this pull request Aug 16, 2022
…d sampling (facebookresearch#538)

- Collapse PPO and DD-PPO trainers
- Faster RNN code -- it is definitely faster and can make a noticeable impact during early training (~20% faster in some cases), but good luck reading it :-)
- Rename NUM_PROCESSES to NUM_ENVIRONMENTS. The fact that the simulators are in different processes is an implementation detail. A backwards compatibility check has been added tho.
- Support specifying training length both in terms of number of updates and number of frames
- Specify the number of checkpoints as the number of checkpoints instead of a checkpoint interval
- Introduce a TensorDict class for more cleanly interacting with dictionaries of tensors (potentially recursive). This also makes RolloutStorage about 100x cleaner.
- Store RGB observations as their proper dtype in the rollout storage (this can save a lot of memory)
- Some refactoring of PPOTrainer.train to be less of a script wrapped in a function
- Double buffered sampling. This can improve performance when simulation time is equal or larger than policy inference time
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