--Support stand alone Metadata Mediator, and building Simulator from MM by jturner65 · Pull Request #1099 · facebookresearch/habitat-sim

jturner65 · 2021-02-17T15:24:25Z

Motivation and Context

This PR adds the ability to construct a Metadata Mediator in python, either from an existing Simulator Configuration or using a default, without requiring a Simulator to exist. It also adds the ability to pass an existing Metadata Mediator into Simulator as a constructor default, as well as to retrieve a reference to the Metadata Mediator currently in use from Simulator.

The purpose of this is that Metadata Mediator maintains all the configuration files that have been loaded into the system, which can be expensive for large datasets. By enabling the MM to be created and accessed independent of a Simulator instance, this loading process can be performed once, and then maintained through multiple simulator lifetimes.

How Has This Been Tested

Existing C++ and python tests. Simtest.cpp has been modified to test Simulators constructed using MM as well as those constructed from SimulatorConfiguration. TODO : python test is pending.

Types of changes

Docs change / refactoring / dependency upgrade
Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
[ x I have read the CONTRIBUTING document.
I have completed my CLA (see CONTRIBUTING)
I have added tests to cover my changes.
All new and existing tests passed.

mosra

Just commenting on the test-related things.

mosra · 2021-02-17T15:48:50Z

  Corrade::Utility::Debug()
      << "Starting Test : getCustomLightingRGBAObservation ";


Why this? :)

(I'm aware that if you get a crash, it won't print what test case it started, but that shouldn't mean you have to suffer like this ;) On my list of things to fix.)

mosra · 2021-02-17T15:52:00Z

+
+    auto stateFinal = AgentState::create();
+    agent->getState(stateFinal);
+    CORRADE_VERIFY(stateOrig->position == stateFinal->position);


Suggested change

CORRADE_VERIFY(stateOrig->position == stateFinal->position);

CORRADE_COMPARE(stateOrig->position, stateFinal->position);

assuming those are Magnum vectors and not Eigen vectors (which don't have a debug output), same below

Oh yeah. Ty

oops, those are eigen vecs

CORRADE_COMPARE(Mn::Vector3{stateOrig->position}, Mn::Vector3{stateFinal->position}); ? 😅

mosra · 2021-02-17T15:53:24Z

 }

 void SimTest::reset() {
+  auto testReset = [&](Simulator& simulator) {


Because all test macros (which take care of extracting the function name) are inside the lambda, the function won't be marked correctly in the output. I'd do

void SimTest::reset() { CORRADE_VERIFY(true); auto testReset = [&](Simulator& simulator) {

to work around that :)

Skylion007 · 2021-02-17T16:12:47Z

      /* --- Template Manager accessors --- */
+      .def_property(
+          "metadata_mediator", &Simulator::getMetadataMediator,
+          &Simulator::setMetadataMediator, py::return_value_policy::copy,


Interesting, you really want to return a copy?

copy of smart pointer. want to increment holder count

Please document in a comment.

That's what happens by default for smart pointers with pybind

That's what I thought but I wasn't sure. Should I remove the return value policy specifier? Serves as a nice doc -shrug-

Skylion007 · 2021-02-17T16:15:27Z

  // ==== Simulator ====
  py::class_<Simulator, Simulator::ptr>(m, "Simulator")
+      // add constructor to build Simulator from existing MetadataMediator
+      .def(py::init<esp::metadata::MetadataMediator::ptr>())


Mind adding a pytest test case somewhere. It will serve as documentation too.

working on it now

Skylion007 · 2021-02-17T18:00:30Z

          R"(The currently active dataset being used.  Will attempt to load
            configuration files specified if does not already exist.)")
      /* --- Template Manager accessors --- */
+      // We wish a copy of the metadata mediator smart poitner so that we


Skylion007 · 2021-02-17T18:03:42Z

    sim_cfg: SimulatorConfiguration
    agents: List[AgentConfiguration]
+    # An existing Metadata Mediator can also be used to construct a SimulatorBackend
+    metadata_mediator: MetadataMediator = attr.ib(default=None)


Suggested change

metadata_mediator: MetadataMediator = attr.ib(default=None)

metadata_mediator: Optional[MetadataMediator] = None

Can sim_cfg and metadata_mediator override each other? Do we print a warning if that's the case? Or assert that one is None? We may want to add a validator to the effect.

Can sim_cfg and metadata_mediator override each other? Do we print a warning if that's the case? Or assert that one is None? We may want to add a validator to the effect.

Yes, let's discuss this further in slack.

Skylion007 · 2021-02-17T18:06:00Z

    sim_cfg: SimulatorConfiguration
    agents: List[AgentConfiguration]
+    # An existing Metadata Mediator can also be used to construct a SimulatorBackend
+    metadata_mediator: MetadataMediator = attr.ib(default=None)


Can sim_cfg and metadata_mediator override each other? Do we print a warning if that's the case? Or assert that one is None? We may want to add a validator to the effect.

aclegg3

Looks good. 👍

Skylion007 · 2021-02-18T21:25:34Z

    def _config_backend(self, config: Configuration) -> None:
        if not self._initialized:
-            super().__init__(config.sim_cfg)
+            super().__init__(config.sim_cfg, config.metadata_mediator)


We should still give a warning / info via the logger that the MetadataMediator is overriding stuff though. Is this handled on the C++ side?

Nothing is being overridden. The config.sim_cfg is the primary datasource. The existing MM is passed in, and then config.sim_cfg is queried for the dataset and scene, just as if it wasn't passed at all. The MM will get its dataset changed to match the dataset specified in the config.sim_cfg. This is non-destructive, and any loaded dataset information the existing MM will be retained, and can be specified separately.

facebook-github-bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label Feb 17, 2021

jturner65 requested review from Skylion007, aclegg3 and mosra February 17, 2021 15:24

mosra reviewed Feb 17, 2021

View reviewed changes

Skylion007 reviewed Feb 17, 2021

View reviewed changes

Skylion007 suggested changes Feb 17, 2021

View reviewed changes

Skylion007 requested a review from erikwijmans February 17, 2021 18:10

jturner65 added 5 commits February 18, 2021 08:29

--Add Simulator Constructor consuming MM; Support MM access from Sim

e99e544

--Create instanced tests for both Simulator constructor mechanisms

1d3a37b

--Python support of MM as arg for SimulatorBackend; python tests

86b17e3

--Reviewer suggestion

72ee67d

--Rebuild to take MM as optional ctor parameter.

aa7d743

jturner65 force-pushed the MM_BuildSimFromMM branch from a6a6f87 to aa7d743 Compare February 18, 2021 13:29

--typo

d728dfd

aclegg3 approved these changes Feb 18, 2021

View reviewed changes

Skylion007 reviewed Feb 18, 2021

View reviewed changes

jturner65 merged commit 61138e6 into facebookresearch:master Feb 19, 2021

jturner65 deleted the MM_BuildSimFromMM branch February 19, 2021 19:02

		Corrade::Utility::Debug()
		<< "Starting Test : getCustomLightingRGBAObservation ";

	CORRADE_VERIFY(stateOrig->position == stateFinal->position);
	CORRADE_COMPARE(stateOrig->position, stateFinal->position);

	metadata_mediator: MetadataMediator = attr.ib(default=None)
	metadata_mediator: Optional[MetadataMediator] = None

Conversation

jturner65 commented Feb 17, 2021

Motivation and Context

How Has This Been Tested

Types of changes

Checklist

Uh oh!

mosra left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jturner65 Feb 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aclegg3 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

jturner65 Feb 18, 2021 •

edited

Loading