[Sensor] Hierarchical Sensor spec by vauduong · Pull Request #1090 · facebookresearch/habitat-sim

vauduong · 2021-02-12T18:53:08Z

Motivation and Context

Currently we only have SensorSpec structure in the simulator. (in Sensor.h). This task is to build a hierarchical sensor spec abstractions to match the new sensor framework. For example, we have “SensorSpec” (base class) containing uuid, sensor type, position and orientation etc., then deriving “VisualSensorSpec”, “NonVisualSensorSpec”, and then “CameraSensorSpec”;

Since we do not have any non visual sensor yet. This task will be focusing on the visual sensor and its subclasses (the left side in the tree structure).

The following PR:

Creates structs SensorSpec, VisualSensorSpec, and CameraSensorSpec with relevant sanityChecks to initialize the default parameters for Sensors
Adds functionality for SensorSpecs to be initialized (Currently only CameraSensorSpec can be initialized) with CameraSensorSpec::create(). After initialization, default parameters can be modified. When a vector of sensorSpecs is passed into SensorFactory::createSensors(), SensorFactory will perform checks and automatically initialize the correct sensor for each SensorSpec and return it in a SensorSuite
Adds SensorSubType::None
Improved == and != operators for SensorSpecs
Deprecates height and width in CameraSensor, deprecates parameters in SensorSpec
SensorSpec::create() is no longer used, use correct SensorSpec initialization to create correct sensor, such as CameraSensorSpec::create()
Fixes python code to use CameraSensorSpec() to initialize sensorSpecs, adds relevant checks for sensor initialization
Breaks hab-lab examples, this will be fixed in [Sensor] Fix hab-lab examples to work with Hierarchical SensorSpec changes habitat-lab#598

How Has This Been Tested

Build and modify 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.

…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

Skylion007 · 2021-02-12T20:28:01Z

+    if (spec->sensorType == SensorType::Color) {
+      if (spec->sensorSubType == SensorSubType::Pinhole ||
+          spec->sensorSubType == SensorSubType::Orthographic) {
+        sensorSuite.add(sensor::CameraSensor::create(sensorNode, spec));


We need appropiate errors here.

…nsor-spec

…yChecks

bigbike · 2021-02-25T06:18:28Z

+                 "SensorSpec::sanityCheck(): position is illegal", );
+  CORRADE_ASSERT((abs(orientation.array()) >= 0).any(),
+                 "SensorSpec::sanityCheck(): orientation is illegal", );
+  CORRADE_ASSERT(!noiseModel.empty(),


Are you sure noise model is a must have?

bigbike · 2021-02-25T06:21:34Z

+      isVisualSensor,
+      "VisualSensorSpec::sanityCheck(): sensorType must be Color, Depth, "
+      "Normal, or Semantic", );
+  if (noiseModel == "Gaussian" || noiseModel == "Poisson" ||


@Skylion007 : is noise model a must have??

I am not sure if noise model is a must have, but I see in hab-lab that there are sensors which use the noise model (seems like only visual sensors though). @Skylion007 what do you think we should do here?

These checks in the VisualSensorSpec mirror checks on they python side.

Yili, I think your question is whether we want noiseModel to be an attribute of SensorSpec or VisualSensorSpec, (and not to get rid of noiseModel in all SensorSpecs) right? Since for nonVisualSensorSpecs noiseModel will always be None. Let me know if this is what you're asking.

Ok, from discussion in hab-sim dev sync we want to keep the attribute in SensorSpec because we anticipate that future non-visual sensor specs will want to add a noiseModel

I think we should keep the checks for noiseModel in SanityCheck because it would be nice to have for C++ and js, but let me know if you would like them removed because of redundancy on the python side.

All the noise models are in Pure Python now, it would be a good idea to move them to C++ if performance warrants it though.

Skylion007

Else issues in python

Skylion007 · 2021-02-25T19:02:45Z

+        "noiseModel is Gaussian, Poisson, SaltAndPepper, or Speckle", );
+  }
+  if (noiseModel == "Redwood") {
+    CORRADE_ASSERT(


Can these noise models even be used from C++?

So far I did not see the noise models were used anywhere in the C++, but maybe I am wrong.

I think we can keep the sanity check like this since it is coded up. It can be easily modified in the future.

But we need to make sure the logic is correct at least. For example, when "redwood" is noise model, the sensor type must be depth sensor.

Ok sounds good! I got all the logic for the noiseModels from doing a global search for the noiseModels

Skylion007

More issues.

vauduong · 2021-02-25T19:42:39Z

Thanks @bigbike and @Skylion007 I've addressed all your feedback except the point regarding noiseModel. I understand it's not used in C++ but we are deciding to keep the attribute in SensorSpec, and anticipate that one day we want to include it in C++. Could you confirm/deny that we want to remove the sanityChecks for noiseModel in this PR?

erikwijmans

This is very nice! Thank you for all the work @vauduong

bigbike · 2021-03-06T07:46:00Z

   */
  void setFar(float _far) {
-    spec_->parameters.at("far") = std::to_string(_far);
+    CORRADE_ASSERT(


@vauduong :
This is something we did not capture.
"near" and "far" should be in the VisualSensor spec! They were in the spec before refactoring.
Then member variables, far_ and near_ should be removed.
Please do fix it in another PR.

@vauduong : Also, please lift up the
bool getObservation(sim::Simulator& sim, Observation& obs) override;
bool getObservationSpace(ObservationSpace& space) override;
from CameraSensor to VisualSensor

move ortho_scale to the CameraSensorSpec.
move channel and observationSpace to VisualSensorSpec. (@Skylion007 : is observationSpace used somewhere in the python side??)

It's replicated entirely on the Python side. The C++ isn't used directly.

deprecate the encoding (I did not see it was used anywhere, please double check).
deprecate the observationSpace.

@vauduong : you can see the branch "cubemap-pr". See what I did there for your reference.
https://github.com/facebookresearch/habitat-sim/pull/1131/files

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

vauduong requested a review from bigbike February 12, 2021 18:53

vauduong added 2 commits February 12, 2021 13:02