[Sensor] Decouple sensor initialization from Agent by vauduong · Pull Request #1064 · facebookresearch/habitat-sim

vauduong · 2021-01-29T17:39:37Z

Motivation and Context

Currently, agents are constructed using a valid scene node reference and an agent configuration in the c++ habitat-sim function Simulator::addAgent. The agent configuration specifies sensor specifications, and the sensors are initialized in the agent constructor using the provided scene node.

This PR just focuses on the C++ side, but another PR can be made in the future to bring the functionality to python.

Sensors can be constructed using any valid scene node, so we wish to decouple sensor initialization from the agent. In this PR, we:

create a factory class SensorFactory, which contains a static method to return a SensorSuite given a reference to a SceneNode and reference to a vector of SensorSpec::ptrs called SensorSetup.
Add method SensorSuite::merge to merge the sensors from another SensorSuite into its existing map of sensors
SensorFactory is called in Simulator::AddAgent to provide sensors.
Add method Simulator::addSensorToObject to add a sensor at a node, given the node's objectId and the specification of the sensor
Add test in SimTest to test adding a sensor to an object
Add test in SensorTest to test SensorFactory methods
Methods in viewer to demo adding sensors to objects has been moved to https://github.com/vauduong/habitat-sim/tree/add-sensors-to-object-viewer-demo

To demo this functionality, methods were added to Simulator to keep track of Sensors and get Sensors by object ID; and methods were added to Viewer to add CameraSensors to objects and toggle between agent and object cameras. You can see a video of a banana being pushed here:

BananaDemo.mov

How Has This Been Tested

Build and run with demo in viewer
Add test to add object in SimTest
Add test to test SensorFactory in SensorTest

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.
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.

… decouple-sensors

…sensors in Simulator::addAgent()

… decouple-sensors

… decouple-sensors formatting

…to decouple-sensors

Skylion007 · 2021-01-29T19:31:06Z

+    scene::SceneNode& node,
+    const sensor::SensorSetup& sensorSetup) {
+  sensor::SensorSuite sensorSuite = sensor::SensorSuite();
+  for (sensor::SensorSpec::ptr spec : sensorSetup) {


This is already decoupled on the Python side FYI. It's even factored out on the Python side to a sensor can be added without reconfiguring the sim. Seems like this should be refactored to also have a function to add a single sensor like we have on the Python side agent.add_sensor()...

Hi @Skylion007
If possible, I sincerely hope you can help us to take a look at the internal task (I will send you the task number via slack), where we gave concrete implementation steps. Check if something is contradictory to what has been done in the Python side. Thanks!

jturner65

Some suggestions.

jturner65

Please remember to document your added functions.

jturner65

Fix for clang-tidy

Skylion007 · 2021-02-05T02:19:35Z

      .def_property_readonly(
          "gfx_replay_manager", &Simulator::getGfxReplayManager,
          R"(Use gfx_replay_manager for replay recording and playback.)")
+      .def_property_read_only("sensor_suite", &Simulator::getSensorSuite)


This is going to conflict with the Simulator python class. Furthermore, the Python sensorsuite is entirely seperate from the C++ one, we need to do more work before merging them.

Thanks for the feedback Aaron, I see now that the python sensor suite is a Dict[str, hsim.Sensor], while the C++ SensorSuite is std::map<std::string, Sensor::ptr>. In this PR, we are making changes that we'd like to see on the C++ side, but I don't know enough about the python side to know if we'd like to see the same changes. It seems like the functionality to just add sensors already exists, right? Do you think I should modify how python sensors work to add them using this SensorFactory method, or just leave it as is?

Oh I think I understand now, we should implement a similar SensorFactory method on the python side to create sensors.

Skylion007 · 2021-02-05T02:21:05Z

      .def("get", &SensorSuite::get, R"(get the sensor by id)");

+  // ==== SensorFactory ====
+  py::class_<SensorFactory>(m, "SensorFactory")


This should be a factory method on the Sensorsuite, there is no reason for this particular class to be bound in Python like so. You should add python tests to see the divergent behavior.

bigbike · 2021-02-05T19:20:12Z

@Skylion007 : when you have time, can you take a look at the slack channel? Thanks!

bigbike · 2021-02-09T20:12:06Z

+   * @param resolution vec2i of resolution to set spec to
+   *
+   */
+  void addSensorToObject(const int objectId,


When designing this API, please take a look at the definitions of the other similar "add" functions, such as addAgent.

The return can be a pointer to that new sensor. So you can memorize this sensor in your demo. You do not need that global sensorSuite_ in the simulator.
The input should be object Id or a scene::SceneNode of that object (You can create an overload function, and use one to call the other.) + a sensorSpec (instead of the pos + orientation + resolution).

bigbike · 2021-02-09T20:16:21Z


  std::vector<agent::Agent::ptr> agents_;
+  // SensorSuite of all sensors available to the simulator
+  esp::sensor::SensorSuite sensorSuite_;


I do not think you need this sensorSuite_. Maybe you can have one defined in the viewer.cpp for your demo, but not in the simulator.
@aclegg3 : How do you think? I remembered that when we discuss this project, you mentioned once that we might need such a global sensorSuite to collect all the sensors? Can you think of any usage case where such a global sensorSuite is a must-have?

Python uses its own for backwards compatability, but there is no reason if you can quickly and easily get all the sensors in the scene.

Thanks for the comments - I included a global sensorSuite_ into Simulator to keep track of all the sensors. I thought about just keeping a pointers to the last added sensor, but later reasoned that it would be useful to be able to get all the sensors, not just the agent sensor and the last added sensor.

I'm happy to change this, but right now it seems like it's not so easy to get all the sensors in the scene.

Let me know any thoughts or if there is a better way to keep track of all the sensors! :)

The question is that why you need to get all the sensors in the scene?
When you add a sensor to an object, the "add" function should give a pointer to this sensor, and you keep it somewhere so that you can use it later.

I am not against to have such a sensorSuite in the viewer.cpp for example, but not here, in the simulator class.

bigbike · 2021-02-09T20:27:07Z


  const int defaultAgentId_ = 0;
  esp::agent::Agent::ptr defaultAgent_ = nullptr;
+  std::string lastAddedObjectName_ = "";


All the new code in the viewer.cpp is for your demo only. Perhaps you do not have to commit it in with this PR.
So this PR should include probably everything expect the viewer.cpp.
Then you create a new branch on top of this branch, vauduong:decouple-sensors (careful, not on top of the master) and put the modified viewer.cpp in that new branch (no PR is needed.)

vauduong · 2021-02-09T21:23:02Z

EDIT: Can hold off on review, going to make some changes - getting rid of global SensorSuite
EDIT 2: Changes done
Thanks @bigbike for the feedback! If anyone else could give me another review and feedback on things to fix that would be greatly appreciated as well!

@aclegg3 @eundersander @jturner65 @Skylion007 @mosra @erikwijmans

…rn pointer to sensor

bigbike

The PR is improved a lot. We are almost there. :)

vauduong · 2021-02-12T15:11:32Z

@aclegg3 @eundersander @jturner65 @Skylion007 @mosra @erikwijmans @bigbike
Thanks everyone for the suggestions! viewer code for demo has been moved to https://github.com/vauduong/habitat-sim/tree/add-sensors-to-object-viewer-demo, and I'm looking for any final reviews for this PR before merging.

aclegg3

Looks good. 👍

…1064) - Create a factory class SensorFactory, which contains a static method to return a SensorSuite given a reference to a - - SceneNode and reference to a vector of SensorSpec::ptrs called SensorSetup. - Add method SensorSuite::merge to merge the sensors from another SensorSuite into its existing map of sensors - SensorFactory is called in Simulator::AddAgent to provide sensors. - Add method Simulator::addSensorToObject to add a sensor at a node, given the node's objectId and the specification of the sensor - Add tests in SimTest and SensorTest

bigbike · 2021-02-13T05:23:12Z

+
+#include "esp/scene/SceneNode.h"
+#include "esp/sensor/CameraSensor.h"
+#include "esp/sensor/Sensor.h"


@vauduong : This is duplicated header file.

vauduong added 9 commits January 28, 2021 12:19

Preliminary

5f6fc89

Merge branch 'master' of github.com:facebookresearch/habitat-sim into…

a32ae25

… decouple-sensors

First draft: Create factory class SensorFactory and use it to create …

186e286

…sensors in Simulator::addAgent()

Merge branch 'master' of github.com:facebookresearch/habitat-sim into…

290f93c

… decouple-sensors

formatting

64a2b9b

Merge branch 'master' of github.com:facebookresearch/habitat-sim into…

b0e3b41

… decouple-sensors formatting

Formatting

7999fec

Merge branch 'decouple-sensors' of github.com:vauduong/habitat-sim in…

deae893

…to decouple-sensors

Unused import

a48f270

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

vauduong requested a review from jturner65 January 29, 2021 17:39

Skylion007 reviewed Jan 29, 2021

View reviewed changes

bigbike requested review from aclegg3, bigbike and eundersander January 29, 2021 19:55

jturner65 reviewed Feb 1, 2021

View reviewed changes

Comment thread src/esp/sensor/SensorFactory.h Outdated

Comment thread src/esp/sensor/SensorFactory.h Outdated

Comment thread src/esp/agent/Agent.h Outdated

vauduong added 2 commits February 2, 2021 00:57

Make SensorFactory a utility class

a94440a

Merge remote-tracking branch 'upstream/master' into decouple-sensors

bc9c9ee

jturner65 reviewed Feb 2, 2021

View reviewed changes

Comment thread src/esp/sensor/SensorFactory.cpp Outdated

vauduong added 3 commits February 4, 2021 12:15

Preliminary demo

2c5a1c2

Add camera object file name to imgui text

b53dc36

Add bindings, use move to set SensorSuite

5d78b49

Skylion007 reviewed Feb 5, 2021

View reviewed changes

vauduong added 3 commits February 5, 2021 12:26

Fix bindings

b64551b

Update viewer logging info, fix move(agentSensors)

4f940f1

Fix

08d5c8b

Merge remote-tracking branch 'upstream/master' into decouple-sensors

def32a7

vauduong marked this pull request as ready for review February 9, 2021 10:22

jturner65 reviewed Feb 9, 2021

View reviewed changes

Comment thread src/esp/sim/Simulator.cpp Outdated

jturner65 reviewed Feb 9, 2021

View reviewed changes

Comment thread src/esp/sim/Simulator.cpp Outdated

Fix destruct object

4e241bd

bigbike suggested changes Feb 9, 2021

View reviewed changes

vauduong added 2 commits February 9, 2021 15:17

Add SensorTest

27c24be

Fix accidental clang-format CMakeLists

68be5a5

vauduong requested a review from mosra February 9, 2021 21:21

Quick updates from feedback

dc6bfef

jturner65 reviewed Feb 10, 2021

View reviewed changes

Comment thread src/utils/viewer/viewer.cpp Outdated

vauduong added 4 commits February 10, 2021 18:05

Move SensorSuite to viewer, update tests, make addSensorToObject retu…

f537430

…rn pointer to sensor

Use corrade utility for lastAddedObjectName

ac7f312

fix clang-tidy

cdd708e

Fix bindings so only correct ones get added

4fae668

bigbike reviewed Feb 12, 2021

View reviewed changes

Comment thread src/esp/sim/Simulator.cpp Outdated

Comment thread src/tests/SensorTest.cpp Outdated

Comment thread src/tests/SimTest.cpp Outdated

vauduong added 5 commits February 11, 2021 19:07

Merge remote-tracking branch 'upstream/master' into decouple-sensors

33e80c3

Make suggested changes

37998be

Remove viewer code

95d3f12

Fix getPreviousNumVisibleDrawings

970796e

Merge remote-tracking branch 'upstream/master' into decouple-sensors

ad5bd24

aclegg3 approved these changes Feb 12, 2021

View reviewed changes

vauduong merged commit 96f5fa1 into facebookresearch:master Feb 12, 2021

bigbike reviewed Feb 13, 2021

View reviewed changes

Comment thread src/esp/sensor/SensorFactory.h

bigbike reviewed Feb 13, 2021

View reviewed changes

vauduong mentioned this pull request Mar 1, 2021

[Sensor] SensorSuite in each SceneNode #1118

Merged

11 tasks

Conversation

vauduong commented Jan 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation and Context

How Has This Been Tested

Types of changes

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jturner65 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jturner65 left a comment

Choose a reason for hiding this comment

Uh oh!

jturner65 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!

bigbike commented Feb 5, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

Uh oh!

bigbike left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vauduong commented Feb 12, 2021

Uh oh!

aclegg3 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

vauduong commented Jan 29, 2021 •

edited

Loading

vauduong commented Feb 9, 2021 •

edited

Loading