[Sensor] Fix hab-lab examples to work with Hierarchical SensorSpec changes by vauduong · Pull Request #598 · facebookresearch/habitat-lab

vauduong · 2021-02-22T06:55:15Z

Motivation and Context

See: facebookresearch/habitat-sim#1090
PR#1090 in habitat-sim introduces a Hierarchical SensorSpec to match the new sensor framework for VisualSensor.

SensorSpec::create() is no longer used, use correct SensorSpec initialization to create correct sensor, such as CameraSensorSpec::create().

This PR fixes examples to use CameraSensorSpec() to initialize sensorSpecs

How Has This Been Tested

Built and tested locally with version of hab-sim in PR#1090, checked that tests passed

Types of changes

Docs change / refactoring / dependency upgrade
Bug fix (non-breaking change which fixes an issue)

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-02-22T07:39:00Z

        sensor_specifications = []
        for sensor in _sensor_suite.sensors.values():
-            sim_sensor_cfg = habitat_sim.SensorSpec()
+            sim_sensor_cfg = habitat_sim.CameraSensorSpec()


Hi @Skylion007 ! I noticed that in hab-lab sensors we don't have SensorSubTypes, and was wondering what the reasoning for that was - would it be a good idea to add this enum?

For context, I check that SensorSubTypes are correct before initializing a CameraSensorSpec() here:
https://github.com/facebookresearch/habitat-sim/pull/1090/files#diff-1c82f32ca8e8fe4738a140dd923ac12f5d482300e28446788e41eafa248a62a1R66-R103

I'm also not sure how BumpSensor is initialized, since this isn't something I was able to find in hab-sim or hab-lab. I know we can't initialize it with CameraSensorSpec as we do the other sensors though. Any insight to this would be appreciated! @Skylion007 @bigbike @mathfac @erikwijmans @abhiskk

We don't actually have implementations of BumpSensor, I don't believe.

Yes, we have only CollisionMeasure, but that would be great to have BumpSensor, but it looks like it can be HabitatLab Task sensor, not Simulator one.

Skylion007 · 2021-02-22T18:33:10Z

I started doing that here: #533

…or-spec

Skylion007 · 2021-02-23T19:02:38Z

-            sensor_specifications.append(sim_sensor_cfg)
+
+            # Check if type VisualSensorSpec, we know that Sensor is one of HabitatSimRGBSensor, HabitatSimDepthSensor, HabitatSimSemanticSensor
+            if (


Not all sensors are VisualSensor in Habitat.

Not all sensors are VisualSensors but VisualSensorSpecs are the only kinds of specs that can be initialized at the moment.

Skylion007

This is fine as a cludge, but there are some issues with support multiple sensor types n the future. Also, some issues with the current error handling / indentation

Skylion007 · 2021-02-23T19:05:54Z

 @registry.register_sensor
 class HabitatSimSemanticSensor(SemanticSensor):
    sim_sensor_type: habitat_sim.SensorType
+    sim_sensor_subtype: habitat_sim.SensorSubType


I suppose, we could tie the SensorSubtype to the Sensor Class like this, but I was thinking about having it be a SensorConfig string that determined it (similar to how we do for UUIDs now). Any reason why not do it that way? It would give the user more flexability down the road (unless you expect SensorSubtypes to have entirely different config types).

Doing it like this though causes a quadratic explosion in the number of Sensor modalities -> sensor_subtypes though.

In other words, is there a good reason this cannot/should not be read from the config?

This is all fine for now since we only support PINHOLE, but will be annoying later when we supports RGBFisheye/DepthFisheye etc...

Hi Aaron! I did this because this file goes through some defined configs and initializes the right sensorSpec.

So far, only CameraSensorSpec exists, but I'm adding the structure for checks so that when Fisheye/Equirectangular sensors are implemented, we can follow the logic to initialize the right sensorSpec.

Does Subtype infer how observation space will be defined? Does different Subtypes of sensors require separate classes wrappers in HabLab?

Hi @mathfac , I think I don't understand enough about how observation space is defined in hablab vs habsim. What is the difference there and what should I be aware of?

Exactly, I don't think they should require separate wrappers, they should be set by the config.

Skylion007 · 2021-02-23T19:07:18Z

        )

        sensor_specifications = []
+        VisualSensorTypeSet = {


We can programatically get the values from a enum by iterating through them with .value()

Also wouldn't this be better defined by some inheritance structure?

Are you saying that instead of checking if it's in a set we should get the value it and compare it with the enum numbers? This is just a way to check which type of SensorSpec to initialize based on the values in the config.

…or-spec

…e statements

mathfac · 2021-02-27T00:48:41Z

Looking forward for this change.

Skylion007 · 2021-03-01T18:01:03Z

Want to let @erikwijmans to take a look at this before merging. Otherwise, looks good to me. We may want to add an error to upgrade habitat_sim though.

erikwijmans

This looks fine to me!

* --Rework PhysicsManager instantiation; PhysicsManagerAttributes has had often-used getter/setters added in the manner of PhysicsObjectAttributes; PhysicsManagers are now passed their defining attributes upon construction; ResourceManager now has separate method to create PhysicsManagers; * --Groundwork for instancing primitive objects; Have rigid body's retain copy of Attributes object at the time of their creation. * Update src/utils/viewer/viewer.cpp Co-Authored-By: Vladimír Vondruš <mosra@centrum.cz> * Update viewer.cpp * --Undo incorrect spelling changes. Should actually be "reseat" when discussing a pointer. * --Address review suggestion * --Minor fixes and updates * --Primitive physical object code; Set up primitive-type specific attributes to organize prim-required constructor info as specified in Magnum implementations; Build enum to describe supported primitives; * --add default constructors to attributes * --Attempt to get bindings to work with intermediate Attributes class; * --more groundwork for instancing primitive objects * -- stub message * --Propagate fix to address double rotation; * --Address reviewer comments * --Modify Primitive Attributes to inherit directly from configuration; * --Address further reviewer comments; Co-authored-by: Vladimír Vondruš <mosra@centrum.cz>

…nsor-spec [Sensor] Fix hab-lab examples to work with Hierarchical SensorSpec changes

Fix examples with hab-sim changes

3d09e35

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

vauduong mentioned this pull request Feb 22, 2021

[Sensor] Hierarchical Sensor spec facebookresearch/habitat-sim#1090

Merged

11 tasks

Small format fix now that .keys() deprecated

93c6216

vauduong commented Feb 22, 2021

View reviewed changes

vauduong added 2 commits February 23, 2021 12:56

Add checks for SensorType and SubType

ef340ec

Merge remote-tracking branch 'upstream/master' into modify-tests-sens…

f71327a

…or-spec

Skylion007 reviewed Feb 23, 2021

View reviewed changes

Skylion007 suggested changes Feb 23, 2021

View reviewed changes

vauduong added 2 commits February 26, 2021 16:59

Merge remote-tracking branch 'upstream/master' into modify-tests-sens…

1bbbd04

…or-spec

Remove uses of sim_utils and initialize SensorSpecs directly, fix els…

80ef51e

…e statements

Add comment

703f78a

Skylion007 approved these changes Mar 1, 2021

View reviewed changes

erikwijmans approved these changes Mar 1, 2021

View reviewed changes

jturner65 merged commit 64b5bee into facebookresearch:master Mar 1, 2021

vauduong deleted the modify-tests-sensor-spec branch March 1, 2021 19:13

Skylion007 mentioned this pull request Apr 16, 2021

[BUG] FOV Is not properly set #627

Closed

dannymcy pushed a commit to dannymcy/habitat-lab that referenced this pull request Jul 8, 2024

Merge pull request facebookresearch#598 from vauduong/modify-tests-se…

a068416

…nsor-spec [Sensor] Fix hab-lab examples to work with Hierarchical SensorSpec changes

HHYHRHY pushed a commit to SgtVincent/EMOS that referenced this pull request Aug 31, 2024

Merge pull request facebookresearch#598 from vauduong/modify-tests-se…

25212b2

…nsor-spec [Sensor] Fix hab-lab examples to work with Hierarchical SensorSpec changes

Conversation

vauduong commented Feb 22, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Skylion007 commented Feb 22, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

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!

Uh oh!

mathfac commented Feb 27, 2021

Uh oh!

Skylion007 commented Mar 1, 2021

Uh oh!

erikwijmans left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

vauduong commented Feb 22, 2021 •

edited

Loading