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

[Unity plugin] Remove sensor noise properties #1646

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Balint-H
Copy link
Contributor

Remove references in MjGlobalOptions and sensor base class to the deprecated sensornoise attribute.

Copy link
Collaborator

@yuvaltassa yuvaltassa left a comment

Choose a reason for hiding this comment

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

Removing the enable flag: Great!

Removing the sensor attribute: Why? It's still there for the user to write into, if they like... Maybe just update the tooltip?

@Balint-H
Copy link
Contributor Author

I think it could be easily misleading, e.g. they have learned some RL controller but want to make it more robust with sensor noise, they see the option and just put a value there without it actually having the intended effect. In any component they use to collect sensor readings with, they can inject noise there just as well.

@yuvaltassa yuvaltassa requested a review from erez-tom May 13, 2024 00:55
@erez-tom
Copy link
Collaborator

Could you please run the tests? Both MjBaseSensorTests and MjcfImporterTests make references to this now-absent field.

@Balint-H
Copy link
Contributor Author

Balint-H commented May 13, 2024

I forgot about the test framework implementation in the package! From now on I'll make sure to run them before a PR, and I'll amend this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants