Adds config option for including static objects during navmesh construction #2104
Conversation
facebook-github-bot
added
the
CLA Signed
|
@aclegg3, I have made the suggested changes and have also opened a corresponding hab-lab PR here: |
| @@ -54,6 +54,10 @@ void initSimBindings(py::module& m) { | |||
| .def_readwrite( | |||
| "allow_sliding", &SimulatorConfiguration::allowSliding, | |||
| R"(Whether or not the agent can slide on NavMesh collisions.)") | |||
| .def_readwrite( | |||
| "include_static_objects_in_navmesh", | |||
There was a problem hiding this comment.
I am sure there is a good reason, but why isn't this a navmesh setting? It requires a reconstruction of the Navmesh which we check by seeing if the new navmesh settings equals the old. Feels more naturally to live in there, no?
There was a problem hiding this comment.
Good point.
Currently, NavMeshSettings only includes recast-level parameters. The mesh joining triggered by this flag happens before the nav module is ever invoked, so it is really a Simaultor flag at the scene level.
I think there could be a valid argument to add this to the NavMeshSettings and then pivot off of that in recompute_navmesh directly instead of treating it separately. That would be a breaking change across the stack.
| self.recompute_navmesh(self.pathfinder, needed_settings) | ||
| self.recompute_navmesh( | ||
| self.pathfinder, | ||
| needed_settings, |
There was a problem hiding this comment.
So weird that this isn't in the navmesh setting struct...
There was a problem hiding this comment.
Actually, don't we need to change the if statement above this to always recompute the navmesh if include_static_objects=True, since we can't guarantee that the navmesh setting caching will work in that case? This code will not be run currently if a navmesh computed with include_static_objects=False is cached as part of the dataset, right?
|
Thoughts @aclegg3 on having this bool be part of the navmesh settings struct instead? Any reason for or against it? |
|
Ah, nvm I see this is part of some legacy code that it is not part of the navmesh setting struct for some reason. @aclegg3 Do you rememebr what the historical reason for not putting it as part of the navmesh settings was? |
Currently, NavMeshSettings only includes recast-level parameters. The mesh joining triggered by this flag happens before the nav module is ever invoked, so it is really a Simaultor flag at the scene level. I think there could be a valid argument to add this to the NavMeshSettings and then pivot off of that in recompute_navmesh directly instead of treating it separately. That would be a breaking change across the stack. Let's discuss this further before merging this change as-is. |
|
Superseded by #2111 |
Motivation and Context
HSSD scenes have all scene objects initialized as static ones. Anyone who needs to use these scenes for training agents (using hab-lab) needs to either manually turn on this inclusion of static objects for navmesh construction (and rebuild) or make an explicit
recompute_navmeshcall with this flag turned on.This PR adds a config option to toggle this inclusion. Corresponding hab-lab PR coming soon.
How Has This Been Tested
By visualizing topdown maps with this flag ON and OFF.
Types of changes
Checklist