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.

Sign up for GitHub

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

Jump to bottom

Fixes outdated sensor data after reset #1276

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
+138 −6
Open

Fixes outdated sensor data after reset #1276

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
9b4816a
Fixes outdated sensor data after reset
kellyguo11 Oct 20, 2024
4bc79ea
Update extension.toml
kellyguo11 Oct 21, 2024
995b8da
Update CHANGELOG.rst
kellyguo11 Oct 21, 2024
345ffd1
Update CHANGELOG.rst
kellyguo11 Oct 21, 2024
5ab1f0b
Update extension.toml
kellyguo11 Oct 21, 2024
5c0c725
Merge branch 'main' of github.com:isaac-sim/IsaacLab into sensor_reset
kellyguo11 Nov 17, 2024
5e3ea01
rename API to forward and add unit test
kellyguo11 Nov 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion source/extensions/omni.isaac.lab/config/extension.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
[package]

# Note: Semantic Versioning is used: https://semver.org/
version = "0.26.0"
version = "0.26.1"

# Description
title = "Isaac Lab framework for Robot Learning"
Expand Down
9 changes: 9 additions & 0 deletions source/extensions/omni.isaac.lab/docs/CHANGELOG.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,15 @@
Changelog
---------

0.26.1 (2024-10-20)
~~~~~~~~~~~~~~~~~~~

Changed
^^^^^^^

* Added call to update articulation kinematics after reset to ensure updated states in non-rendering sensors.
Copy link
Contributor

Choose a reason for hiding this comment

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

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 :)



0.26.0 (2024-10-16)
~~~~~~~~~~~~~~~~~~~

Expand Down
2 changes: 2 additions & 0 deletions source/extensions/omni.isaac.lab/omni/isaac/lab/envs/direct_rl_env.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -344,6 +344,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
self.sim.update_fabric_and_kinematics()
# if sensors are added to the scene, make sure we render to reflect changes in reset
if self.sim.has_rtx_sensors() and self.cfg.rerender_on_reset:
self.sim.render()
Expand Down
2 changes: 2 additions & 0 deletions source/extensions/omni.isaac.lab/omni/isaac/lab/envs/manager_based_env.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should be called after the event manager is called to reset transforms (so all the other managers also get this change)

IsaacLab/source/extensions/omni.isaac.lab/omni/isaac/lab/envs/manager_based_env.py

Line 363 in 5ab1f0b

self.event_manager.apply(mode="reset", env_ids=env_ids, global_env_step_count=env_step_count)

self.sim.update_fabric_and_kinematics()
# if sensors are added to the scene, make sure we render to reflect changes in reset
if self.sim.has_rtx_sensors() and self.cfg.rerender_on_reset:
self.sim.render()
Expand Down
2 changes: 2 additions & 0 deletions source/extensions/omni.isaac.lab/omni/isaac/lab/envs/manager_based_rl_env.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment as the manager based env.

self.sim.update_fabric_and_kinematics()
# if sensors are added to the scene, make sure we render to reflect changes in reset
if self.sim.has_rtx_sensors() and self.cfg.rerender_on_reset:
self.sim.render()
Expand Down
14 changes: 9 additions & 5 deletions source/extensions/omni.isaac.lab/omni/isaac/lab/sim/simulation_context.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -394,6 +394,14 @@ def get_setting(self, name: str) -> Any:
"""
return self._settings.get(name)

def update_fabric_and_kinematics(self) -> None:
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Suggested change
def update_fabric_and_kinematics(self) -> None:
def forward(self) -> None:

"""Updates articulation kinematics and fabric for rendering."""
if self._fabric_iface is not None:
if self.physics_sim_view is not None and self.is_playing():
# Update the articulations' link's poses before rendering
self.physics_sim_view.update_articulations_kinematic()
self._update_fabric(0.0, 0.0)

"""
Operations - Override (standalone)
"""
Expand Down Expand Up @@ -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?

if self.physics_sim_view is not None and self.is_playing():
# Update the articulations' link's poses before rendering
self.physics_sim_view.update_articulations_kinematic()
self._update_fabric(0.0, 0.0)
self.update_fabric_and_kinematics()
# render the simulation
# note: we don't call super().render() anymore because they do above operation inside
# and we don't want to do it twice. We may remove it once we drop support for Isaac Sim 2022.2.
Expand Down
Loading