Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Disable sim sensors when using the batch renderer. #2031

Merged
merged 3 commits into from
Mar 16, 2023

Conversation

0mdc
Copy link
Contributor

@0mdc 0mdc commented Mar 14, 2023

Motivation and Context

This change prevents the simulators to allocate sensor memory and render them when using the batch renderer.

How Has This Been Tested

Tested locally from habitat-lab.

Types of changes

  • 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.

@facebook-github-bot facebook-github-bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label Mar 14, 2023
Copy link
Contributor

@eundersander eundersander left a comment

Choose a reason for hiding this comment

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

I left some comments. Have a look and try to address, but basically LGTM.

for _sensor_uuid, sensor in agent_sensorsuite.items():
sensor.draw_observation()

# Get observations.
for agent_id in agent_ids:
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you explain what's going on here? How can we safely call get_observation if we haven't rendered here? Is this relying on some external code to have already triggered rendering?

Copy link
Contributor Author

@0mdc 0mdc Mar 15, 2023

Choose a reason for hiding this comment

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

When batch rendering, get_observation() returns None, creating an empty "placeholder" for this sensor uuid. The batch renderer replaces this value by the rendered data in post_step().

This is a bit of a hack as Habitat-Lab has checks to make sure that all sensors are present in observations between reset()/step() and post_step().

An alternative would be to skip those checks for visual sensors when batch renderer is activated, but I'm afraid that this is more confusing.

I admit that the behavior could be clearer here.

src_python/habitat_sim/simulator.py Outdated Show resolved Hide resolved
src_python/habitat_sim/simulator.py Show resolved Hide resolved
@0mdc 0mdc force-pushed the batch-renderer-disable-sim-sensors branch from c9f25a9 to 295825c Compare March 15, 2023 21:25
@0mdc 0mdc marked this pull request as ready for review March 15, 2023 21:26
@0mdc 0mdc merged commit 3df607c into main Mar 16, 2023
jturner65 added a commit that referenced this pull request Mar 22, 2023
jturner65 added a commit that referenced this pull request Mar 22, 2023
jturner65 added a commit that referenced this pull request Mar 30, 2023
jturner65 added a commit that referenced this pull request Apr 3, 2023
jturner65 added a commit that referenced this pull request Apr 3, 2023
jturner65 added a commit that referenced this pull request Apr 3, 2023
@0mdc 0mdc deleted the batch-renderer-disable-sim-sensors branch May 29, 2023 14:27
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