Skip to content

[Sensor] VisualSensor and CameraSensor updates for Hierarchical SensorSpec#1138

Merged
vauduong merged 2 commits intofacebookresearch:masterfrom
vauduong:sensor-spec
Mar 12, 2021
Merged

[Sensor] VisualSensor and CameraSensor updates for Hierarchical SensorSpec#1138
vauduong merged 2 commits intofacebookresearch:masterfrom
vauduong:sensor-spec

Conversation

@vauduong
Copy link
Copy Markdown
Contributor

@vauduong vauduong commented Mar 10, 2021

Motivation and Context

This PR introduces changes to VisualSensor, CameraSensor, VisualSensorSpec, and CameraSensorSpec in order for
the hierarchical SensorSpec to be correct when new Sensors are introduced.

In this PR:

  • Lift getObservation, getObservationSpace, and readObservation from CameraSensor to Visual Sensor
  • Deprecate observationSpace and encoding in SensorSpec
  • Move channels, near, and far to VisualSensorSpec, move ortho_scale to CameraSensorSpec

See https://github.com/facebookresearch/habitat-sim/pull/1090/files#r588849042 for context

How Has This Been Tested

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

@facebook-github-bot facebook-github-bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label Mar 10, 2021
@vauduong vauduong changed the title [Sensor] VisualSensor and CameraSensor updates [Sensor] VisualSensor and CameraSensor updates for Hierarchical SensorSpec Mar 10, 2021
Comment thread src/esp/sensor/VisualSensor.h Outdated
Comment thread src/esp/sensor/CameraSensor.h Outdated
@vauduong
Copy link
Copy Markdown
Contributor Author

Thanks @eundersander for the feedback and the catch! The PR has been updated now.

@vauduong vauduong merged commit 57cc428 into facebookresearch:master Mar 12, 2021
@bigbike
Copy link
Copy Markdown
Contributor

bigbike commented Mar 12, 2021

@vauduong : I have not seen anyone approved this PR before you merged it in.

@vauduong
Copy link
Copy Markdown
Contributor Author

vauduong commented Mar 12, 2021

@vauduong : I have not seen anyone approved this PR before you merged it in.

Oh no! Sorry I think I saw a lot of green and thought it was approved. Do you have any more suggestions? If so I can create another PR.

Thank you for letting me know and sorry for being too hasty.

Copy link
Copy Markdown
Contributor

@bigbike bigbike left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You do not have to roll back this one. But please be more careful next time.
For the small comments I made in this PR, you can fix them in the ongoing pr (sensorSuite).

bool gpu2gpuTransfer = false; // True for pytorch tensor support
vec2i resolution = {128, 128}; // height x width
int channels =
4; // Number of components in buffer values, eg. 4 channels for RGBA
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you change the comment style to

/**
 * @brief ...
 */

*/
bool displayObservation(sim::Simulator& sim) override;

bool isVisualSensor() const override { return true; }
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

comment...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed Do not delete this pull request or issue due to inactivity.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants