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.

Sign up for GitHub

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

Adds config option for including static objects during navmesh construction #2104

Closed
wants to merge 4 commits into from
Closed

Adds config option for including static objects during navmesh construction #2104

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3c403d9
Adds cfg flag for including static objects in navmeshes
mukulkhanna May 11, 2023
9e8c18b
Pre-commit run
mukulkhanna May 12, 2023
ae28ee3
Adds bool comparator
mukulkhanna May 12, 2023
ac4b18a
Pre-commit run
mukulkhanna May 12, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions src/esp/bindings/SimBindings.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

&SimulatorConfiguration::includeStaticObjectsInNavmesh,
R"(Whether or not static objects will be included in the constructed NavMesh.)")
.def_readwrite(
"create_renderer", &SimulatorConfiguration::createRenderer,
R"(Optimisation for non-visual simulation. If false, no renderer will be created and no materials or textures loaded.)")
Expand Down
1 change: 1 addition & 0 deletions src/esp/sim/SimulatorConfiguration.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ bool operator==(const SimulatorConfiguration& a,
a.physicsConfigFile == b.physicsConfigFile &&
a.overrideSceneLightDefaults == b.overrideSceneLightDefaults &&
a.sceneLightSetupKey == b.sceneLightSetupKey;
a.includeStaticObjectsInNavmesh == b.includeStaticObjectsInNavmesh;
}

bool operator!=(const SimulatorConfiguration& a,
Expand Down
2 changes: 2 additions & 0 deletions src/esp/sim/SimulatorConfiguration.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@ struct SimulatorConfiguration {
bool createRenderer = true;
//! Whether or not the agent can slide on NavMesh collisions.
bool allowSliding = true;
//! Whether or not static objects will be included in the constructed NavMesh.
bool includeStaticObjectsInNavmesh = false;
//! Enable or disable the frustum culling optimisation
bool frustumCulling = true;
/**
Expand Down
6 changes: 5 additions & 1 deletion src_python/habitat_sim/simulator.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -248,7 +248,11 @@ def _config_pathfinder(self, config: Configuration) -> None:
f"Recomputing navmesh for agent's height {default_agent_config.height} and radius"
f" {default_agent_config.radius}."
)
self.recompute_navmesh(self.pathfinder, needed_settings)
self.recompute_navmesh(
self.pathfinder,
needed_settings,
Copy link
Contributor

Choose a reason for hiding this comment

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

So weird that this isn't in the navmesh setting struct...

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

include_static_objects=self.config.sim_cfg.include_static_objects_in_navmesh,
)

self.pathfinder.seed(config.sim_cfg.random_seed)

Expand Down