-
Notifications
You must be signed in to change notification settings - Fork 508
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
Add UUID config to simulator visual sensors #472
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you document how and why to use this somewhere in the code? Even just linking to #471 in the code would help.
Otherwise LGTM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, Aaron! That's longstanding issue for defining several sensors of the same type.
Maybe worth to change Sensor class to fix it once:
habitat-lab/habitat/core/simulator.py
Line 66 in 8cc6cf7
self.uuid = self._get_uuid(*args, **kwargs) |
To something like this:
if self.config and hasattr(self.config, "UUID"):
self.uuid = config.UUID
else:
self.uuid = self._get_uuid(*args, **kwargs)
habitat/core/simulator.py
Outdated
|
||
def _get_uuid(self, *args: Any, **kwargs: Any) -> str: | ||
return "rgb" | ||
return self.uuid |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This all can be done with:
return self.uuid | |
return self.uuid | |
if hasattr(self.config, "UUID"): | |
return config.UUID | |
else: | |
return "rgb" |
@mathfac Maybe we should just deprecate _get_uuid and have it be a required part of the YAML sensor config? |
if hasattr(self.config, "UUID"): | ||
# We allow any sensor config to override the UUID | ||
self.uuid = self.config.UUID | ||
else: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mathfac Actually, I am not sure I like this method because calling _get_uuid will return the wrong UUID. We really should remove the internal method INMO
* Add UUID config to simulator visual sensors * Add comments about UUID and add to BumpSensor * Incorporate suggestions * Remove unused code * Make base sensor have overidable UUID
* Add UUID config to simulator visual sensors * Add comments about UUID and add to BumpSensor * Incorporate suggestions * Remove unused code * Make base sensor have overidable UUID
Motivation and Context
Fixes #471
How Has This Been Tested
With existing sensors tests as well as by overriding the UUID
Types of changes
Checklist