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

Update multi goal path implementation #465

Merged
merged 4 commits into from
Jan 31, 2020
Merged

Conversation

erikwijmans
Copy link
Contributor

Motivation and Context

The multigoal path implementation was relying on being able to compare path lengths on the navigation mesh before optimization, this turns out to be not correct a little too often. This PR changes it to be simply min(A* paths).

This also has the benefit of removing on our deviations from recast main!

How Has This Been Tested

With the test! Also added a benchmark because Corrade's test suite is fun.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)

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

codecov-io commented Jan 31, 2020

Codecov Report

Merging #465 into master will increase coverage by 0.27%.
The diff coverage is 98.71%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #465      +/-   ##
==========================================
+ Coverage   57.81%   58.09%   +0.27%     
==========================================
  Files         161      161              
  Lines        7067     7106      +39     
  Branches       84       84              
==========================================
+ Hits         4086     4128      +42     
+ Misses       2981     2978       -3
Flag Coverage Δ
#CPP 53.09% <98.71%> (+0.52%) ⬆️
#JavaScript 10% <ø> (ø) ⬆️
#Python 79.2% <ø> (ø) ⬆️
Impacted Files Coverage Δ
src/esp/nav/test/PathFinderTest.cpp 100% <100%> (ø) ⬆️
src/esp/nav/PathFinder.cpp 77.28% <97.22%> (+0.51%) ⬆️

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 a23722f...381b751. Read the comment docs.

if (testCaseInstanceId() == 0) {
setTestCaseDescription("Path to single point");
esp::nav::ShortestPath path;
path.requestedStart = pathFinder.getRandomNavigablePoint();
Copy link
Contributor

Choose a reason for hiding this comment

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

Does that make sense to calculate distance to the same 100 points using single shortest path search?

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.

Thank you for improving this, checked the code flow. Look's valid and tests are passing.

path.requestedStart.data(), closestRequestedEnd.data(), polys, numPolys,
path.points[0].data(), 0, 0, &numPoints, MAX_POLYS);
for (int i = 0; i < path.requestedEnds.size(); ++i) {
if ((path.requestedStart - path.requestedEnds[i]).norm() >
Copy link
Contributor

Choose a reason for hiding this comment

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

As I understand this check is selecting prospect end points that can be closer that path.geodesicDistance

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, exactly.

@mathfac
Copy link
Contributor

mathfac commented Jan 31, 2020

Merging to unblock facebookresearch/habitat-lab#290.

@mathfac mathfac merged commit 74d8501 into master Jan 31, 2020
@mathfac mathfac deleted the update-multi-goal-path branch January 31, 2020 08:40
Ram81 pushed a commit to Ram81/habitat-web-sim that referenced this pull request Dec 10, 2020
The multigoal path implementation was relying on being able to compare path lengths on the navigation mesh before optimization, this turns out to be not correct a little too often. This PR changes it to be simply min(A* paths).
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