[Sensor] SensorSuite in each SceneNode by vauduong · Pull Request #1118 · facebookresearch/habitat-sim

vauduong · 2021-03-01T21:30:29Z

Motivation and Context

Due to new functionality from #1064, there needs to be more functionality to keep track of sensors.

In this PR:

Change the sensors_ in SensorSuite from std::map<std::string, Sensor::ptr> to std::map<std::string, reference_wrapper>
Make SensorSuite inherited from Magnum::SceneGraph::AbstractFeature3D
Add raw pointers nodeSensorSuite and subtreeSensorSuite in SceneNode. These features will be released upon SceneNode's deletion.
In the Sensor constructor, add this sensor to its node's nodeSensorSuite and subtreeSensorSuite. Traverse all the way up to the root node, add this sensor to the subtreeSensorSuite of every ancestor node. Remove the Sensor from subtreeSensorSuite of every ancestor node in the destructor.
Overload setParent so that ancestor subtrees are updated
Write bindings for getter functions and SensorFactory methods
Write robust unit tests for creating sensors, deleting sensors, and setting parents

How Has This Been Tested

Build and test, write additional unit tests

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.

vauduong · 2021-03-01T21:45:04Z

  for (const SensorSpec::ptr& spec : sensorSetup) {
    scene::SceneNode& sensorNode = node.createChild();
    // VisualSensor Setup
    if (spec->isVisualSensorSpec()) {
      if (spec->sensorSubType == SensorSubType::Orthographic ||
          spec->sensorSubType == SensorSubType::Pinhole) {
-        sensorSuite.add(CameraSensor::create(
-            sensorNode, std::dynamic_pointer_cast<CameraSensorSpec>(spec)));
+        sensor::CameraSensor::create(
+            sensorNode,
+            std::dynamic_pointer_cast<sensor::CameraSensorSpec>(spec));
      }


I'm having some trouble with initializing sensors here. Currently, the sensor is initialized and a reference is added to all the relevant node and subtree sensorSuites, as expected.

However, as soon as it is done initializing and the for loop moves onto the next SensorSpec, the sensor is destructed immediately.

I'm not sure why this is happening because the reference to the sensor should still exist as it was added to a map, right? Why is this happening and how can I get the Sensor to persist?

the for loop moves onto the next SensorSpec

It means the local smart pointer will be deleted.

I'm not sure why this is happening because the reference to the sensor should still exist as it was added to a map, right? Why is this happening and how can I get the Sensor to persist?

No it is not right. When the smart pointer (a local variable) gets destroyed, the dtor of Sensor class is called, which means the sensor you just added to the map gets removed (you just coded it up by yourself in this PR).

vauduong · 2021-03-16T08:42:33Z

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

Thank you everyone for the feedback! I've addressed all the feedback so far, and am looking forwards to a final pass on this PR. There are some comments about the python implementation that have been left for reference, but they will be addressed in a separate PR.

bigbike

I did not even go through all the PR. Too many problems need to be addressed already.
Stop reviewing now.

bigbike · 2021-03-17T18:42:00Z

      .function("setLocalTransform", &Sensor_setLocalTransform)
      .function("specification", &Sensor::specification);

+  em::class_<SensorSuite>("SensorSuite")


You only fixed the bindings, not the actual web utility who would call such bindings. I am worried if this will cause failure in the web version. Can you figure out the web utility and double confirm this is not the case?

Took a deeper look and none of the demos except the vr demo will call getSensorSuite, but there are some issues that I posted about in slack and am still investigating.

emscripten can't use reference types to interact with js, only pointers and primitives https://stackoverflow.com/questions/55905148/emscripten-pass-stl-c-map-parameter

It seems there's a workaround to write a policy to get pointers from references, but I'm not sure if this is what we want to do here. emscripten-core/emscripten#3480

Can anyone provide some guidance?

How about have JS getSensors return pointers instead of references? And then instead of binding it directly to SensorSuite::getSensors, bind it to some new C++ helper function that constructs the map of pointers.

Thank you for the suggestion eundersander! vr demo is fixed and working

bigbike · 2021-03-17T20:52:28Z

+  std::map<std::string, std::reference_wrapper<sensor::Sensor>>::iterator it =
+      nodeSensorSuite_->getSensors().begin();
+  if (it != nodeSensorSuite_->getSensors().end()) {
+    CORRADE_ASSERT(static_cast<scene::SceneNode*>(this->parent()) != nullptr,


Clearly you do not understand what is the difference between the static_cast and the dynamic_cast.
If the item is not convertible, static_cast will NOT return nullptr. So basically what you wrote here will not work.
I remembered I clearly told you to use dynamic_cast during our 1:1.

bigbike · 2021-03-17T21:00:17Z

+  std::map<std::string, std::reference_wrapper<sensor::Sensor>>::iterator it =
+      nodeSensorSuite_->getSensors().begin();
+  if (it != nodeSensorSuite_->getSensors().end()) {
+    CORRADE_ASSERT(static_cast<scene::SceneNode*>(this->parent()) != nullptr,


Here, the same thing. static_cast is the problem.
And you just simply copy and paste whatever you have in the addSensorToParentNodeSensorSuite.
But clearly the logic is different in an "add" and a "remove" functions.
Here, if you find you cannot dynamic_cast to its parent, one case is that the parent (SceneNode) is already deconstructed. So the conversion fails., which means in this case, you do not need to do anything. (The logic you have here is to let the program exit due to ASSERTION failure. It is totally wrong.)

Ok, thanks for the clarification!

spring4175 · 2021-03-17T21:25:16Z

Hi, I've been repeatedly pinged in this thread in review comments. I believe you're meaning to ping someone else. Just letting you know.

bigbike · 2021-03-17T21:37:46Z

@vivian: Sorry! I sincerely apologize! It was my mistake. I am pretty sure this would be the last time to ping you in this PR.

bigbike · 2021-03-19T18:38:24Z

@@ -12,26 +14,173 @@ namespace scene {

 SceneNode::SceneNode(SceneNode& parent)


Hold on here:
You have code duplication in the constructors.
You need constructor delegation.

bigbike · 2021-03-19T18:42:38Z

+}
+
+SceneNode& SceneNode::createChild(SceneNodeTags childNodeTags) {
+  CORRADE_ASSERT(


Are these functions (createChild, setParent etc.) exposed in the python side?
If yes, let use ESP_CHECK in the esp/core/Check.h in these functions as it is more friendly for the py users.

If you already have ESP_CHECK, you do not need the CORRADE_ASSERT any more. You should read the comments about the ESP_CHECK before you use it.

bigbike · 2021-03-19T18:43:00Z

+  // Perform same internal checks as magnum to ensure newParent is a valid new
+  // parent before updating any SensorSuites
+  // Parent can not be leaf node
+  CORRADE_ASSERT(!(newParent->getSceneNodeTags() & SceneNodeTag::Leaf),


same here, use ESP_CHECK

Again, CORRADE_ASSERT is not needed any more.

bigbike · 2021-03-19T19:14:33Z

+void SceneNode::removeSubtreeSensorsFromAncestors() {
+  for (const auto& sensor : subtreeSensorSuite_->getSensors()) {
+    SceneNode* currentNode = dynamic_cast<SceneNode*>(this->parent());
+    while (currentNode && !SceneGraph::isRootNode(*currentNode)) {


if currentNode is the root node, then this piece of code will not remove the sensors from the currentNode.

Go back to the slack channel, read the code I gave you very carefully, and try to understand the difference between my code and the code you wrote here.

In your tests, DO check if the number of sensors is correct in the root node!!

Thank you, this is really important. Here is the code from slack:

SceneNode* currentNode = this; do { currentNode = dynamic_cast<SceneNode*>(currentNode->parent()); // no need to worry currentNode could be nullptr, no chance currentNode->getSubtreeSensorSuite().getSensors().erase(sensor.first); } while (!SceneGraph::isRootNode(*currentNode));

We had discussed previously that currentNode could never be nullptr, which made sense because the currentNode would never be nullptr before reaching rootNode's parent, which it would not reach due to the while condition. However, this currentNode was actually coming back nullptr sometimes, and this was causing the segfault when running examples.py. My update to check that currentNode was not nullptr fixed the segfault, but overlooked adding/removing sensors from the root node's SensorSuites.

I have updated the logic to check that currentNode is not nullptr and to also update rootNode. I have also added checks for rootNode's SensorSuites in SensorTest.

Thanks and let me know if you have any questions.

bigbike · 2021-03-19T19:17:31Z

+void SceneNode::addSubtreeSensorsToAncestors() {
+  for (const auto& sensor : subtreeSensorSuite_->getSensors()) {
+    SceneNode* currentNode = dynamic_cast<SceneNode*>(this->parent());
+    while (currentNode && !SceneGraph::isRootNode(*currentNode)) {


So it means the root node will never ever update its sensorSuite.

eundersander · 2021-03-19T23:43:37Z

+   * this Agent's SceneNode and its children values.
+   * NOTE: This is only called in JS, as emscripten needs pointers
+   */
+  std::map<std::string, sensor::Sensor::ptr> jsGetSubtreeSensors() {


Let's avoid enlarging this class's public interface unnecessarily. I had in mind a global static function defined in bindings_js.cpp and not exposed to other parts of the codebase. We don't want to litter the C++ codebase with js-specific code.

eundersander · 2021-03-19T23:45:00Z

      .function("hasAction", &Agent::hasAction)
      .function("act", &Agent::act)
-      .function("getSubtreeSensors", &Agent::getSubtreeSensors);
+      .function("getSubtreeSensors", &Agent::jsGetSubtreeSensors);


You can bind to a free function (a static global function defined in this file) instead of a Agent member method.

Thanks for the clarification Eric! That's been addressed and is tested and working.

bigbike · 2021-03-20T01:43:17Z

+SceneNode::SceneNode() : Mn::SceneGraph::AbstractFeature3D{*this} {
  setCachedTransformations(Mn::SceneGraph::CachedTransformation::Absolute);
  absoluteTransformation_ = absoluteTransformation();
+  // nodeSensorSuite_ and subtreeSensorSuite_ will be released upon SceneNode's


// Once created, nodeSensorSuite_ and subtreeSensorSuite_ are features owned by the scene graph node. No need to release them in the destructor since magnum scene graph will handle it.

Thank you! Let me know if there are any other changes you think I should make.

bigbike · 2021-03-20T01:47:33Z

+}
+
+SceneNode& SceneNode::createChild(SceneNodeTags childNodeTags) {
+  CORRADE_ASSERT(


If you already have ESP_CHECK, you do not need the CORRADE_ASSERT any more. You should read the comments about the ESP_CHECK before you use it.

bigbike · 2021-03-20T01:48:06Z

+  // Perform same internal checks as magnum to ensure newParent is a valid new
+  // parent before updating any SensorSuites
+  // Parent can not be leaf node
+  CORRADE_ASSERT(!(newParent->getSceneNodeTags() & SceneNodeTag::Leaf),


Again, CORRADE_ASSERT is not needed any more.

bigbike · 2021-03-23T17:56:33Z

-  static sensor::SensorSuite createSensors(
-      scene::SceneNode& node,
-      const sensor::SensorSetup& sensorSetup);
+  static void deleteSensor(scene::SceneNode& node, const std::string& uuid);


Just read the comments. So consider call it deleteSubtreeSensor to avoid confusion.

vauduong added 7 commits February 24, 2021 14:59

prelim

71deeee

Merge remote-tracking branch 'upstream/master' into node-sensor-suite

20c1ef7

First draft

92871f1

Fix log error message

aac0352

Add pointer to sensor in scene node

4a12397

Merge remote-tracking branch 'upstream/master' into node-sensor-suite

d1d2af8

Remove pointer in scene node

8cbe2de

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

Skylion007 reviewed Mar 1, 2021

View reviewed changes

Comment thread src/esp/scene/SceneNode.cpp

vauduong commented Mar 1, 2021

View reviewed changes