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

Allow sliding in habitat-sim to be turned off. #439

Merged
merged 8 commits into from Jan 24, 2020
Merged

Allow sliding in habitat-sim to be turned off. #439

merged 8 commits into from Jan 24, 2020

Conversation

@erikwijmans
Copy link
Contributor

erikwijmans commented Jan 21, 2020

Motivation and Context

In habitat-sim, agents slide when they collide. This was shown to hurt sim2real performance.

The implementation of no sliding here differs considerably from the previous implementation. The previous implementation was pretty hacky (although it did allow for intermediary values between on and off). This implementation changes the core of recast and allows for sliding to be truly disabled. See the commit on the recast side here: erikwijmans/recastnavigation@354591e

I also added method documentation and fixed a few little bugs here and there in the pathfinder (I can break these into separated PRs if needed, but GitHub makes that a pain..)

How Has This Been Tested

Via the unit test!

Types of changes

  • New feature (non-breaking change which adds functionality)
erikwijmans added 2 commits Jan 21, 2020
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Jan 22, 2020

Codecov Report

Merging #439 into master will decrease coverage by 0.44%.
The diff coverage is 92.68%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #439      +/-   ##
==========================================
- Coverage   57.87%   57.42%   -0.45%     
==========================================
  Files         175      160      -15     
  Lines        8282     6972    -1310     
  Branches       84       84              
==========================================
- Hits         4793     4004     -789     
+ Misses       3489     2968     -521
Flag Coverage Δ
#CPP 52.08% <92.26%> (-2.17%) ⬇️
#JavaScript 10% <ø> (ø) ⬆️
#Python 78.94% <95.83%> (+0.14%) ⬆️
Impacted Files Coverage Δ
src/esp/sensor/Sensor.h 81.81% <ø> (+25.15%) ⬆️
src/esp/sensor/Sensor.cpp 84.37% <ø> (+31.74%) ⬆️
src/esp/sim/Simulator.h 100% <ø> (ø)
src/esp/gfx/Renderer.h 100% <ø> (+50%) ⬆️
src/esp/scene/test/GibsonSceneTest.cpp 60.6% <ø> (ø) ⬆️
src/esp/nav/PathFinder.h 100% <ø> (+63.88%) ⬆️
src/esp/scene/SceneGraph.h 100% <ø> (ø) ⬆️
habitat_sim/__init__.py 100% <ø> (ø) ⬆️
src/esp/sensor/PinholeCamera.h 100% <ø> (ø) ⬆️
src/tests/SimTest.cpp 100% <ø> (ø) ⬆️
... and 57 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 c0cfc9e...c58e291. Read the comment docs.

Copy link
Contributor

msbaines left a comment

Overall, LGTM.

Major concern is whether allowSliding shouldn't just be part of PathFinder state instead of a new API call. It seems like it is state (it is state for Simulator) and I don't think we would expect calls to both variants for a given PathFinder instance.

Also, the dataQueryFilter change is good but is a large percentage of the patch. Might be better on its own.

Other than that, just nits.

src/esp/nav/PathFinder.h Outdated Show resolved Hide resolved
protected:
bool initNavQuery();
void removeZeroAreaPolys();
std::vector<vec3f> prevEnds;
template <typename T>
T tryStepImpl(const T& start, const T& end, bool allowSliding);

This comment has been minimized.

Copy link
@msbaines

msbaines Jan 22, 2020

Contributor

Should this be handled via a boolean state flag? Wouldn't allowSliding always be passed as true or false for a give PathFinder instance?

This comment has been minimized.

Copy link
@erikwijmans

erikwijmans Jan 23, 2020

Author Contributor

It seemed logical to just have both methods be implemented on the pathfinder and just choose the one to call. May also be useful/let people play around with turning on/off sliding stochastically.

src/esp/nav/PathFinder.h Outdated Show resolved Hide resolved
src/esp/bindings/GfxBindings.cpp Outdated Show resolved Hide resolved
src/esp/nav/PathFinder.cpp Outdated Show resolved Hide resolved
@erikwijmans erikwijmans merged commit 784d683 into master Jan 24, 2020
4 checks passed
4 checks passed
ci/circleci: cpp_lint Your tests passed on CircleCI!
Details
ci/circleci: install_and_test_ubuntu Your tests passed on CircleCI!
Details
ci/circleci: js_lint Your tests passed on CircleCI!
Details
ci/circleci: python_lint Your tests passed on CircleCI!
Details
@erikwijmans erikwijmans deleted the no-sliding branch Jan 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.