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

Fixes outdated sensor data after reset #1276

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

Conversation

kellyguo11
Copy link
Contributor

@kellyguo11 kellyguo11 commented Oct 20, 2024

Description

This change adds a call to update_articulations_kinematic() after performing reset in an environment to ensure that non-render sensors are updated after performing reset.

Type of change

  • Bug fix (non-breaking change which fixes an issue)

Checklist

  • I have run the pre-commit checks with ./isaaclab.sh --format
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • I have updated the changelog and the corresponding version in the extension's config/extension.toml file
  • I have added my name to the CONTRIBUTORS.md or my name already exists there

Signed-off-by: Kelly Guo <[email protected]>
Signed-off-by: Kelly Guo <[email protected]>
Signed-off-by: Kelly Guo <[email protected]>
Signed-off-by: Kelly Guo <[email protected]>
@Mayankm96
Copy link
Contributor

Mayankm96 commented Oct 21, 2024

From an understanding standpoint, if a user sets robot state for articulation, isn't the base pose also updated in non-rendering sensors because the "views" capture the same memory. What does the update fabric do? 👀

Also can we have some sort of test to make sure this behavior is always verified (for sensors)?

@@ -464,11 +472,7 @@ def render(self, mode: RenderMode | None = None):
self.set_setting("/app/player/playSimulations", True)
else:
# manually flush the fabric data to update Hydra textures
if self._fabric_iface is not None:
Copy link
Contributor

Choose a reason for hiding this comment

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

Hm this would break non env workflows where users didn't need to call a forward function. Maybe we add a flag on whether forward was called and in-case it wasn't, we do it implicitly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This shouldn't actually change any logic right? It's just moving a piece of the code into a method.

@kellyguo11
Copy link
Contributor Author

From an understanding standpoint, if a user sets robot state for articulation, isn't the base pose also updated in non-rendering sensors because the "views" capture the same memory. What does the update fabric do? 👀

Also can we have some sort of test to make sure this behavior is always verified (for sensors)?

I think it's needed for capturing updates from the dof states and propagating those to the bodies. Added a test case for it.

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.

3 participants