[Sensor] upgrade draw_observation in python Sensor class by bigbike · Pull Request #1159 · facebookresearch/habitat-sim

bigbike · 2021-04-04T04:18:22Z

Motivation and Context

This diff is trying to address the following issues:

Address the code duplication. The old draw_observation in Sensor (python) has the same code logic as the drawObservation() in the CameraSensor class (C++). It is a type of code duplication.
Such code logic does not work for the FisheyeSensor introduced recently. It works ONLY for the "Pinhole" or "Ortho" Camera.
The old dependency is wrong. Before the actual rendering logic was implemented in the Renderer class (C++). And Sensor class (both in C++ and python) depended on it. The correct dependency should be: Each sensor type implements the actually rendering logic (because different sensors (Pinhole, Fisheye etc.) have different, specific logic), and Renderer depends on the sensors.
Upgrade Fisheye sensor and Camera sensor accordingly.

How Has This Been Tested

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.

bigbike · 2021-04-04T04:23:07Z

+  // TODO: deprecate the following function: draw(visualSensor, sceneGraph,
+  // flags);
  // draw the scene graph with the visual sensor provided by user
  void draw(sensor::VisualSensor& visualSensor,


My opinion: This function is really obsolete and thus should be removed.
Use the new one directly.
Note that the RenderCamera flags (such as FrustumCulling) are not needed since such info is included in the sim.

bigbike · 2021-04-04T04:30:20Z

Tested it with the example.py and it gave the correct result. See the attached picture:

bigbike · 2021-04-04T04:24:46Z

@@ -38,11 +40,22 @@ class Renderer {
            scene::SceneGraph& sceneGraph,


This one can be kept since it provides a way to render the scene graph without the sensor.

bigbike · 2021-04-04T06:49:45Z

-    renderer->draw(*this, sim.getActiveSemanticSceneGraph(), flags);
+    draw(sim.getActiveSemanticSceneGraph(), flags);
    if (&sim.getActiveSemanticSceneGraph() != &sim.getActiveSceneGraph()) {
      flags |= gfx::RenderCamera::Flag::ObjectsOnly;
-      renderer->draw(*this, sim.getActiveSceneGraph(), flags);


I believe this is a bug in our current master. Just no one hit it so far.
this CameraSensor cannot be in 2 different SG at the same time.

Update:
Oh, I believe it became a bug recently when we deprecated the defaultRenderCamera in each SG. Before we used it to render the corresponding SG. So using the sensor from another SG to render was not a problem since it set its absolute transformation to the defaultRenderCamera of the current SG...

bigbike · 2021-04-04T18:46:10Z

+    if (twoSceneGraphs) {
+      // *assume* the sensor is always in the regular scene graph.
+      // backup first
+      Mn::Matrix4 relativeTransform = node().transformation();
+      Mn::Matrix4 absTransform = node().absoluteTransformation();
+      scene::SceneNode* p = static_cast<scene::SceneNode*>(node().parent());
+      // take the sensor from the current regular scene graph and connect it to
+      // the root node of semantic scene graph, set the *correct* transformation
+      node().setParent(&sim.getActiveSemanticSceneGraph().getRootNode());
+      node().setTransformation(absTransform);
+      draw(sim.getActiveSemanticSceneGraph(), flags);
+      // after drawing, put the node back to the regular scene graph.
+      // MUST use the setParent function! Otherwise the sensorSuite in the node
+      // will be wrong!!!
+      node().setParent(p);
+      node().setTransformation(relativeTransform);
+    } else {
+      draw(sim.getActiveSemanticSceneGraph(), flags);
+    }


@Skylion007 : I just updated the code as I replicated the logic here, and did it in the correct way with setParent().

Skylion007 · 2021-04-04T19:26:05Z

+    }
+
+    if (twoSceneGraphs) {
      flags |= gfx::RenderCamera::Flag::ObjectsOnly;


BTW, this cludge we are doing is going to hurt our FPS a lot in scenes without the semantic mesh, but only added objects (such as synthetic datasets.) One thing on the agenda is to find a better way to resolve this long term so we don't have to do this double pass all the time. @aclegg3 I think you mentioned something about this during our last meeting?

This can of course be saved for a different PR, just wanted to bring it up.

🤔 I'm not sure that this is the case. Here if we only have a single scene graph we'll just render it with semantic ids, if we have 2 scene-graphs (e.g. a specific separate semantic asset) then we render that asset and then a pass over dynamic objects. I don't see the overhead you are worried about. Am I missing something?

@aclegg3 : It means the absolute transformation for all nodes will be computed twice (one per each SG) if I remember it correctly.

bigbike · 2021-04-05T00:35:48Z

          [](Renderer& self, sensor::VisualSensor& visualSensor,
-             scene::SceneGraph& sceneGraph, RenderCamera::Flag flags) {
-            self.draw(visualSensor, sceneGraph, RenderCamera::Flags{flags});
+             scene::SceneGraph& sceneGraph, RenderCamera::Flags flags) {


@aclegg3 :
Would you mind taking a close look at it?
The original code was introduced in #679 . I believe it is a bug. Can you help me to double verify it?
Thanks.

bigbike · 2021-04-05T00:36:46Z


 public:
-  ESP_SMART_POINTERS(ManagedContainer<T, Access>);
+  ESP_SMART_POINTERS(ManagedContainer<T, Access>)


this ";" gives us a lot of compiling warnings. Get rid of it in this PR as well.

Skylion007 · 2021-04-05T15:35:00Z

+    if (twoSceneGraphs) {
+      // *assume* the sensor is always in the regular scene graph.
+      // backup first
+      Mn::Matrix4 relativeTransform = node().transformation();


Perhaps this should be abstracted out if possible. Disappointing that this hack is necessary, but it is required for every VisualSensor, right?

That can also be saved for a later PR though.

Yes, a bit code duplication here. I did not figure out a good way to extract it so far. Like I said in the description, different visual sensors has it is own rendering logic. Read this two pieces of code. They are a bit different.

Allow me to think about it.
This is problem that we need to solve.

Otherwise it will become a big problem as more sensors are coming in the future.

@mosra @aclegg3 @erikwijmans any ideas here?

I am thinking the ways to address this. It must be solved before committing this diff in.

bigbike · 2021-04-06T01:58:13Z

+      moveSemanticSensorToSemanticSceneGraph(sim);
+      cubeMap_->renderToTexture(*cubeMapCamera_,
+                                sim.getActiveSemanticSceneGraph(), flags);
+      moveSemanticSensorBackToRegularSceneGraph(sim);


@Skylion007 : I reduced the code duplication by extracting 2 helper functions applied here.

The challenge is that the code looks like a "sandwich". The "bread" is the same but the "food" in between varies.

Skylion007 · 2021-04-06T02:33:15Z

Similar to render_enter / exit semantics though, right? It would need to be split between two functions. Also could be done with a functional programming lambda interface, although not as cleanly.

…

On Mon, Apr 5, 2021 at 9:58 PM bigbike ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/esp/sensor/FisheyeSensor.cpp <#1159 (comment)> : > // generate the cubemap texture - cubeMap_->renderToTexture(*cubeMapCamera_, sim.getActiveSceneGraph(), flags); + if (fisheyeSensorSpec_->sensorType == SensorType::Semantic) { + bool twoSceneGraphs = + (&sim.getActiveSemanticSceneGraph() != &sim.getActiveSceneGraph()); + + if (twoSceneGraphs) { + moveSemanticSensorToSemanticSceneGraph(sim); + cubeMap_->renderToTexture(*cubeMapCamera_, + sim.getActiveSemanticSceneGraph(), flags); + moveSemanticSensorBackToRegularSceneGraph(sim); @Skylion007 <https://github.com/Skylion007> : I reduced the code duplication by extracting 2 helper functions applied here. The challenge is that the code looks like a "sandwich". The "bread" is the same but the "food" in between varies. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1159 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAPVMXYTQYRXR2OM7GVJHULTHJTEDANCNFSM42K66TIA> .

erikwijmans · 2021-04-06T15:49:18Z

 target_link_libraries(
  sensor
  PUBLIC core gfx scene
+  PRIVATE sim


Why private?

It was set (in different ways) to help me better understand why the CI fails.
It is a sad story. I was taking PTO on Mon. but I spent whole day trying to solve the CI failure, which is still there.

I found the ResourceMangerTest cannot be compiled. It complains libsensor.a is missing some definitions from here and there, even though it does not need anything from sensor.

erikwijmans · 2021-04-06T15:50:09Z

+void CameraSensor::draw(scene::SceneGraph& sceneGraph,
+                        gfx::RenderCamera::Flags flags) {
+  for (auto& it : sceneGraph.getDrawableGroups()) {
+    // TODO: remove || true


This TODO is getting carried around more and more places, are we ever going to actually do it?

I believe the purpose is to build a filter function on drawables, which can be useful in the material editing.

Yeah, the it.second.prepareForDraw(*renderCamera_) part of the if statement seems fine and I can see a use for that returning false and something not getting drawn, my concern is that we are simply ignoring the return value of that function via the || true. If we ever do want to do something with that function, we have a bunch of || true's that will make that harder.

I don't know the history of the || true (I think it's been there forever and thus likely a debugging thing that never got removed), but it currently does nothing since the only implementation for prepareForDraw just returns true.

erikwijmans · 2021-04-06T15:55:25Z

    return semanticScene_;
  }

+  bool semanticSceneExists() { return (semanticScene_ != nullptr); }


Suggested change

bool semanticSceneExists() { return (semanticScene_ != nullptr); }

bool semanticSceneExists() const { return (semanticScene_ != nullptr); }

erikwijmans · 2021-04-06T15:57:20Z

+    return;
+  }
+
+  node().setParent(semanticSensorParentNodeBackup_);


Can you add an assert that the pointer is not null and the optional is not empty and then null/empty them after?

OK. Will do.

erikwijmans · 2021-04-06T15:58:25Z

+        (&sim.getActiveSemanticSceneGraph() != &sim.getActiveSceneGraph());
+
+    if (twoSceneGraphs) {
+      moveSemanticSensorToSemanticSceneGraph(sim);


This seems a bit risky as it's easy to forget to call the moveSemanticSensorBackToRegularSceneGraph function. Worth considering RAII here.

Can you elaborate a bit how you are going to do RAII here?

if people forgets to call this function moveSemanticSensorBackToRegularSceneGraph, she will know immediately as in the following (L145), she needs it to draw the "objects". If the above function is not called, the program will hit an assertion in Magnum (camera is not in the same SG) and quit.

Just a helper class that calls moveSemanticSensorToSemanticSceneGraph in its constructor and moveSemanticSensorBackToRegularSceneGraph in its destructor.

Skylion007 · 2021-04-07T16:35:19Z

@bigbike Merging from master should solve the CI issues caused by doc building

Skylion007 · 2021-04-07T21:53:34Z

Okay weird, only 1 test is running.

bigbike · 2021-04-07T21:50:03Z

+# the codebase has a deep architectural problem that needs solving -- lib dependency cycles !!!!
+# the following is just a "crutch", a temporary remedy here.
+# https://cmake.org/cmake/help/latest/prop_tgt/LINK_INTERFACE_MULTIPLICITY.html
+set_target_properties(sensor PROPERTIES LINK_INTERFACE_MULTIPLICITY 3)


Previously the CI tests kept failing. Tried many ways to fix it but failed.
With the help from @mosra, we found the root cause -- lib dependency cycles. It is really a big problem in our current system. Here we applied a short-term solution to temporarily mitigate it.

bigbike · 2021-04-07T21:55:49Z

  VisualSensorSpec::ptr visualSensorSpec_ =
      std::dynamic_pointer_cast<VisualSensorSpec>(spec_);
+
+  class MoveSemanticSensorNodeHelper {


@erikwijmans :
The RAII proposed by you is actually a good idea. Thank you.
Take a look at the implementation. See if it is what you expected.

Yeah, this looks nice. Left one question about implementation

erikwijmans · 2021-04-08T15:23:15Z

+      "sensor is attached to the root node, and thus cannot be moved.", );
+
+  // check if the sensor is already in this semantic scene graph
+  if (node.scene() == sim.getActiveSemanticSceneGraph().getRootNode().scene()) {


I think this case will always cause a crash? Since the destructor will always hit CORRADE_INTERNAL_ASSERT(semanticSensorParentNodeBackup_);

Well, the basic assumption is that at the very beginning, the semantic sensor is attached to the regular scene graph, not the semantic scene graph. Your comment makes me think if it is always true.

If this is true, then you do not need to worry about it here.

If there are NO two scene graphs, e.g., only the semantic scene graph, then user should never ever use MoveSemanticSensorNodeHelper at all.

Should this be an assert then? Right now it looks like a feature where this won't move the sensor if it doesn't need to. However, the destructor assumes that if the sensor isn't part of the active scene graph, then the constructor moved it to the semantic scene graph and will try to move it back. This will cause a crash however.

bigbike · 2021-04-08T18:52:15Z

-    // SensorType is Depth or any other type
-    renderer->draw(*this, sim.getActiveSceneGraph(), flags);
+    // SensorType is Color, Depth or any other type
+    draw(sim.getActiveSceneGraph(), flags);


@erikwijmans :
If there is only one scene graph, we assume it is the regular scene graph.
Is there a chance that we will only have semantic scene graph?
also cc: @aclegg3

If we only have a one scene graph we'll use it for both RGB and SEMANTIC and all object will be on the correct scene graph for both renders.

I see. Thanks @aclegg3. Very clear. In this case, getActiveSceneGraph and getActiveSemanticSceneGraph will return the same SG. So we are good here.

bigbike · 2021-04-09T17:48:01Z

Dear reviewers,

If you do not have more concerns, I am going to merge it in this afternoon. Thanks.
@erikwijmans : hope the latest version addressed all your concerns.

mosra

Just a random comment that doesn't affect the approval status.

mosra · 2021-04-09T17:51:10Z

 )

+# ATTENTION developers !!!!!!!!!!!!!!!!!!!!
+# the codebase has a deep architectural problem that needs solving -- lib dependency cycles !!!!


I like the amount of exclamation marks here. 👍

mosra · 2021-04-09T17:52:34Z

+  CORRADE_INTERNAL_ASSERT(activeSceneID_ >= 0 &&
+                          activeSceneID_ < sceneID_.size());


Code golf, if you want:

Suggested change

CORRADE_INTERNAL_ASSERT(activeSceneID_ >= 0 &&

activeSceneID_ < sceneID_.size());

CORRADE_INTERNAL_ASSERT(std::size_t(activeSceneID_) < sceneID_.size());

Same below. Not sure about the type, also you might probably have different coding style rules for the cast.

erikwijmans

🚢

Skylion007 · 2021-04-09T19:13:07Z

Looks good. Let's merge this in.

bigbike added 2 commits April 3, 2021 20:09

minor

27c28de

minor

7e892c1

bigbike requested a review from aclegg3 April 4, 2021 04:18

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

bigbike requested review from Skylion007, erikwijmans and mosra April 4, 2021 04:18

bigbike commented Apr 4, 2021

View reviewed changes

bigbike requested a review from eundersander April 4, 2021 04:28

Skylion007 reviewed Apr 4, 2021

View reviewed changes

Comment thread habitat_sim/simulator.py

fix the CI.

ccdbe2c

bigbike commented Apr 4, 2021

View reviewed changes

Skylion007 reviewed Apr 4, 2021

View reviewed changes

Skylion007 approved these changes Apr 4, 2021

View reviewed changes

Skylion007 reviewed Apr 4, 2021

View reviewed changes

Comment thread habitat_sim/simulator.py

bigbike added 3 commits April 4, 2021 16:52

minor

9748e23

minor

44527c9

bugs??!!

6ea5782

bigbike commented Apr 5, 2021

View reviewed changes

bigbike added 2 commits April 4, 2021 18:15

minor

84f47e2

minor

02c4157

bigbike commented Apr 5, 2021

View reviewed changes

Comment thread src/esp/gfx/Renderer.cpp

bigbike added 2 commits April 4, 2021 19:03

deprecate a draw function in Renderer

95ee080

did the same trick for fisheye sensor

32ce26b

Skylion007 reviewed Apr 5, 2021

View reviewed changes

bigbike added the do not merge Not ready to merge. This label should block merging. label Apr 5, 2021

reduce code duplication

50fc51a

bigbike removed the do not merge Not ready to merge. This label should block merging. label Apr 6, 2021

minor

91dcb78

bigbike commented Apr 6, 2021

View reviewed changes

bigbike added 4 commits April 5, 2021 21:44

minor

ff50829

minor

5a2b316

test

8560670

minor

85747f0

erikwijmans reviewed Apr 6, 2021

View reviewed changes

bigbike added 6 commits April 7, 2021 11:13

Merge branch 'master' into py_sensor

5a3c955

address reviewer's concern.

227eb11

minor

a984bd6

add RAII. address reviewers' concerns.

2d16459

minor

f5313c2

minor

fc692d0

minor

ddbe9b5

bigbike commented Apr 7, 2021

View reviewed changes

erikwijmans reviewed Apr 8, 2021

View reviewed changes

bigbike commented Apr 8, 2021

View reviewed changes

bigbike added 2 commits April 8, 2021 16:40

minor

4b5ebf1

Merge branch 'master' into py_sensor

f500643

mosra approved these changes Apr 9, 2021

View reviewed changes

minor

e4245fb

erikwijmans approved these changes Apr 9, 2021

View reviewed changes

bigbike merged commit f90d7ea into master Apr 9, 2021

bigbike deleted the py_sensor branch April 9, 2021 23:49

aclegg3 mentioned this pull request May 11, 2021

[AO migration] async rendering #1229

Merged

11 tasks

		@@ -38,11 +40,22 @@ class Renderer {
		scene::SceneGraph& sceneGraph,

	bool semanticSceneExists() { return (semanticScene_ != nullptr); }
	bool semanticSceneExists() const { return (semanticScene_ != nullptr); }

		CORRADE_INTERNAL_ASSERT(activeSceneID_ >= 0 &&
		activeSceneID_ < sceneID_.size());

	CORRADE_INTERNAL_ASSERT(activeSceneID_ >= 0 &&
	activeSceneID_ < sceneID_.size());
	CORRADE_INTERNAL_ASSERT(std::size_t(activeSceneID_) < sceneID_.size());

Conversation

bigbike commented Apr 4, 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!

bigbike Apr 4, 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!

bigbike commented Apr 4, 2021

Uh oh!

Uh oh!

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!

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 Apr 9, 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!

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!

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!

Skylion007 commented Apr 6, 2021 via email

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!

Choose a reason for hiding this comment

Uh oh!

Skylion007 commented Apr 7, 2021

Uh oh!

Skylion007 commented Apr 7, 2021

bigbike commented Apr 4, 2021 •

edited

Loading

bigbike Apr 4, 2021 •

edited

Loading

bigbike Apr 9, 2021 •

edited

Loading