Fixes outdated sensor data after reset #1276
Conversation
kellyguo11
requested review from
Dhoeller19,
Mayankm96 and
jsmith-bdai
as code owners
Signed-off-by: Kelly Guo <kellyguo123@hotmail.com>
Signed-off-by: Kelly Guo <kellyguo123@hotmail.com>
Signed-off-by: Kelly Guo <kellyguo123@hotmail.com>
Signed-off-by: Kelly Guo <kellyguo123@hotmail.com>
| @@ -251,6 +251,8 @@ def reset(self, seed: int | None = None, options: dict[str, Any] | None = None) | |||
| indices = torch.arange(self.num_envs, dtype=torch.int64, device=self.device) | |||
| self._reset_idx(indices) | |||
|
|
|||
| # update articulation kinematics | |||
There was a problem hiding this comment.
I think this should be called after the event manager is called to reset transforms (so all the other managers also get this change)
| @@ -194,6 +194,8 @@ def step(self, action: torch.Tensor) -> VecEnvStepReturn: | |||
| reset_env_ids = self.reset_buf.nonzero(as_tuple=False).squeeze(-1) | |||
| if len(reset_env_ids) > 0: | |||
| self._reset_idx(reset_env_ids) | |||
| # update articulation kinematics | |||
There was a problem hiding this comment.
Same comment as the manager based env.
| @@ -394,6 +394,14 @@ def get_setting(self, name: str) -> Any: | |||
| """ | |||
| return self._settings.get(name) | |||
|
|
|||
| def update_fabric_and_kinematics(self) -> None: | |||
There was a problem hiding this comment.
I am wondering if we should go for a more canonical terminology. Many simulators distinguish between kinematic and dynamics stepping through different terms. For example, Mujoco calls forward kinematics function as forward and the simulation step as simulate.
Something like this is more user-friendly.
| def update_fabric_and_kinematics(self) -> None: | |
| def forward(self) -> None: |
| Changed | ||
| ^^^^^^^ | ||
|
|
||
| * Added call to update articulation kinematics after reset to ensure updated states in non-rendering sensors. |
There was a problem hiding this comment.
I think it's a good practice to explain what was happening before this change was made. For instance, "Earlier, non-rendering sensors still used older transforms which corrupted the obtained readings."
This way it isn't just a change but also the motivation for the change that gets documented :)
|
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: | |||
There was a problem hiding this comment.
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?
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
Checklist
pre-commitchecks with./isaaclab.sh --formatconfig/extension.tomlfileCONTRIBUTORS.mdor my name already exists there