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

Fix Double PathFinder #1483

Merged
merged 3 commits into from
Sep 15, 2021
Merged

Fix Double PathFinder #1483

merged 3 commits into from
Sep 15, 2021

Conversation

erikwijmans
Copy link
Contributor

Motivation and Context

Due to legacy reasons, we create a pathfinder object in the python sim after the cpp sim has been initialized. This means that the navmesh loaded via the scene dataset get's unloaded!

How Has This Been Tested

Via tests. We have a lot of tests that require the navmesh to be loaded correctly and I do not see python saying it loaded the navmesh, so cpp must be doing it correctly!

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 Sep 15, 2021
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.

The only time this will fail is if Sim::reconfigure is called with the same SimulatorConfiguration a it already has. If close was called before this, the Pathfinder will have been destroyed, but this short circuit will bypass the code to recreate it.

Otherwise, the Pathfinder is always recreated as part of reconfigure

Probably, the Pathfinder init should be moved to be before the configuration similarity check, conditional on it being nullptr, like we do with sceneManager_

@erikwijmans
Copy link
Contributor Author

Close also resets the config to its default value so that'll be pretty edge-casey, but yeah, I will do the same thing as sceneManager_ to avoid a potential segfault.

@jturner65 jturner65 self-requested a review September 15, 2021 18:36
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

@Skylion007 Skylion007 left a comment

Choose a reason for hiding this comment

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

This also has the side effect of simplifying the python code. Nice!

@erikwijmans erikwijmans merged commit ea0da67 into master Sep 15, 2021
@erikwijmans erikwijmans deleted the fix-double-pathfinder branch September 15, 2021 21:24
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

5 participants