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

Local planner #546

Merged
merged 20 commits into from
Apr 30, 2020
Merged

Local planner #546

merged 20 commits into from
Apr 30, 2020

Conversation

erikwijmans
Copy link
Contributor

Motivation and Context

The current greedy follower wasn't designed in a world were sliding was disabled and action noise existed, so it isn't great in those situations. This PR switches it out for a local motion primitive planner that does a considerably better job and also sets things up for action noise (by allowing the user to specify what key should be emitted for each action to take as the planner needs to plan on noiseless actions).

Compared to the old one, there is a bump of ~0.02 SPL when sliding is enabled. When sliding is disabled however, SPL is increased from ~0.85 (with a ~12% failure rate) to ~0.95 (with a ~2% failure rate), which is much better.

How Has This Been Tested

Via the tests!

Types of changes

  • New feature (non-breaking change which adds functionality)

@facebook-github-bot facebook-github-bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label Mar 26, 2020
@codecov
Copy link

codecov bot commented Mar 26, 2020

Codecov Report

Merging #546 into master will increase coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #546      +/-   ##
==========================================
+ Coverage   57.32%   57.35%   +0.02%     
==========================================
  Files         129      130       +1     
  Lines        5739     5750      +11     
  Branches       84       84              
==========================================
+ Hits         3290     3298       +8     
- Misses       2449     2452       +3     
Flag Coverage Δ
#CPP 52.27% <100.00%> (ø)
#JavaScript 10.00% <ø> (ø)
#Python 76.33% <100.00%> (-0.02%) ⬇️
Impacted Files Coverage Δ
habitat_sim/nav/greedy_geodesic_follower.py 98.43% <100.00%> (+2.21%) ⬆️
habitat_sim/simulator.py 96.53% <100.00%> (ø)
src/esp/core/RigidState.h 100.00% <100.00%> (ø)
src/esp/physics/RigidObject.h 27.02% <100.00%> (-1.93%) ⬇️
habitat_sim/utils/common.py 60.81% <0.00%> (-5.41%) ⬇️

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 eed76b2...9271925. Read the comment docs.

@erikwijmans erikwijmans mentioned this pull request Mar 28, 2020
4 tasks
Copy link
Contributor

@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.

Minor comments. I'll wait for others to comment also. Nice docs. 👍

Curious about thrashing. Thrashing occurs in noisy action space when no combination of LEFT or RIGHT with FORWARD results in a better reward than constant collision? Looks like to fix this we just execute the entire pile of LEFT/RIGHT actions that build up and hope that the next iteration of search is better afterward?

habitat_sim/nav/greedy_geodesic_follower.py Outdated Show resolved Hide resolved
src/esp/nav/GreedyFollower.h Outdated Show resolved Hide resolved
@erikwijmans
Copy link
Contributor Author

For thrashing, it occurs when you enter a loop of left->right->left->right->left->right.... the way to get out of that is to take the best primitive and execute it fully instead of executing the first action and re-planning.

For the situation you described (no action better than just colliding), that results in an error (the cpp code returns an ERROR token and python then throws an error).

@erikwijmans
Copy link
Contributor Author

As a side note, I haven't seen a case where this thing actually thrashes. I am not sure if you actually need that logic since we have access to the ground truth navmesh to plan on, but its simple enough to have it :)

@bigbike bigbike removed their request for review April 29, 2020 23:35
@erikwijmans
Copy link
Contributor Author

@aclegg3 Can you double check esp::core::RigidState

@aclegg3
Copy link
Contributor

aclegg3 commented Apr 30, 2020

@aclegg3 Can you double check esp::core::RigidState

Looks good. Thanks!

Copy link
Contributor

@mathfac mathfac left a comment

Choose a reason for hiding this comment

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

Tested the PR from Habitat-API level. Works great with noisy actions as well!
Example of navigation:
Screenshot 2020-04-29 20 43 50
!

@mathfac mathfac merged commit d3c7468 into master Apr 30, 2020
@mathfac mathfac deleted the local-planner branch April 30, 2020 03:48
Ram81 pushed a commit to Ram81/habitat-web-sim that referenced this pull request Dec 10, 2020
The current greedy follower wasn't designed in a world were sliding was disabled and action noise existed, so it isn't great in those situations. This PR switches it out for a local motion primitive planner that does a considerably better job and also sets things up for action noise (by allowing the user to specify what key should be emitted for each action to take as the planner needs to plan on noiseless actions).

Compared to the old one, there is a bump of ~0.02 SPL when sliding is enabled. When sliding is disabled however, SPL is increased from ~0.85 (with a ~12% failure rate) to ~0.95 (with a ~2% failure rate), which is much better.
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