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

Sim settings update (taking over for Nakura Mino) #1958

Draft
wants to merge 45 commits into
base: main
Choose a base branch
from

Conversation

jrreyna
Copy link
Contributor

@jrreyna jrreyna commented Dec 2, 2022

Motivation and Context

Taking over this PR from NakuraMino. Here was the original description:

The current habitat_sim.utils.settings.make_cfg() fn is limited to single first person sensors with the uuid color_sensor, semantic_sensor etc.

e.g.

if settings["color_sensor"]:
        color_sensor_spec = create_camera_spec(
            uuid="color_sensor",
            hfov=settings["hfov"],
            sensor_type=habitat_sim.SensorType.COLOR,
            sensor_subtype=habitat_sim.SensorSubType.PINHOLE,
        )

where camera_sensor_spec.position = [0, settings["sensor_height"], 0].

Since many of the examples in examples/colabs/ use a 1st person sensor AND a 3rd person sensor (e.g. https://github.com/facebookresearch/habitat-sim/blob/main/examples/tutorials/nb_python/ECCV_2020_Interactivity.py#L178), it would be best if you could flexibly add multiple cameras through sim_settings.

How Has This Been Tested

This passes CI/CD, and these changes are also tested through pytest tests/*.py. They all pass.

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.

@jrreyna jrreyna self-assigned this Dec 2, 2022
@facebook-github-bot facebook-github-bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label Dec 2, 2022
@jrreyna jrreyna changed the title Sim settings update (john reyna takeover) Sim settings update (taking over for Nakura Mino) Dec 2, 2022
…_to_settings' and made a new function called 'update_sensor_settings' for clarity. Also made a function in settings.py called 'clear_sensor_settings' so that users don't have to manually clear sensor settings. Lastly, I updated all the scripts and pytests that use the renamed function
…into sim_settings_update_john_reyna_takeover
…lues in settings.py, also added comments to it
…ome reason, it went from passing to failing inexplicably
…into sim_settings_update_john_reyna_takeover
… as a depth sensor was leftover from a previous function
…or_type is depth_sensor, so I simplified that section. Also fixed a bug where I was trying to access agent_id from a sensor spec, even though that isn't a property of sensor specs
…sor_settings to check if a given key in 'kw_args' is valid. Reworked test_sensors.py to use the 'update_or_add_sensor_settings' function, and restructured some of the code accordingly
…laced that function call with its body: sensor_settings = {**default_sensor_settings, **sensor_settings}
…posed to do. Also added lines to remove sensor settings in the test_sensors pytest in certain places that I had removed earlier. Converted those sections of test_sensors on main to the remove/clear functions I wrote in settings.py
…ly'. Also changed code to always add an initial color sensor by default if we are adding a sensor dynamically, as if we are adding a color sensor dynamically, we need to already have one in the scene instantiated when configuring the simulator.
…ors function. Probably not necessary. Also added comments
…est_sensors function. It failed pytest without it
…into sim_settings_update_john_reyna_takeover
…into sim_settings_update_john_reyna_takeover
…ngs} as it is kind of unclear what it does. Instead, I made a function in settings.py that does the same thing
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.

None yet

2 participants