Suggest: Directly set visibility=0(visual body) instead of modifying the actor's pose.#1447
Suggest: Directly set visibility=0(visual body) instead of modifying the actor's pose.#1447fffffq00 wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Simplifies actor visual hiding/showing by removing GPU-specific pose-move workaround and always toggling render-body visibility.
Changes:
- Replace GPU/CPU branching in
hide_visual()with a single visibility toggle loop. - Replace GPU/CPU branching in
show_visual()with a single visibility toggle loop. - Update docstring to reflect simplified behavior.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def hide_visual(self): | ||
| """ | ||
| Hides this actor from view. In CPU simulation the visual body is simply set to visibility 0 | ||
|
|
||
| For GPU simulation, currently this is implemented by moving the actor very far away as visiblity cannot be changed on the fly. | ||
| As a result we do not permit hiding and showing visuals of objects with collision shapes as this affects the actual simulation. | ||
| Note that this operation can also be fairly slow as we need to run px.gpu_apply_rigid_dynamic_data and px.gpu_fetch_rigid_dynamic_data. | ||
| Hides this actor from view. Simply set to visibility 0. | ||
| """ | ||
| assert not self.has_collision_shapes |
| # set hidden *after* setting/getting so not applied to self.before_hide_pose erroenously | ||
| for obj in self._objs: | ||
| obj.find_component_by_type( | ||
| sapien.render.RenderBodyComponent | ||
| ).visibility = 0 |
| # set hidden *before* setting/getting so not applied to self.before_hide_pose erroenously | ||
| self.hidden = False | ||
| if self.scene.gpu_sim_enabled: | ||
| if hasattr(self, "before_hide_pose"): | ||
| self.pose = self.before_hide_pose | ||
| self.px.gpu_apply_rigid_dynamic_data() | ||
| self.px.gpu_fetch_rigid_dynamic_data() | ||
| else: | ||
| for obj in self._objs: | ||
| obj.find_component_by_type( | ||
| sapien.render.RenderBodyComponent | ||
| ).visibility = 1 | ||
| for obj in self._objs: | ||
| obj.find_component_by_type( | ||
| sapien.render.RenderBodyComponent | ||
| ).visibility = 1 |
| for obj in self._objs: | ||
| obj.find_component_by_type( | ||
| sapien.render.RenderBodyComponent | ||
| ).visibility = 0 |
| for obj in self._objs: | ||
| obj.find_component_by_type( | ||
| sapien.render.RenderBodyComponent | ||
| ).visibility = 1 |
|
One issue is that this will slow down the sim step due to the for loop required to modify each parallelized instance of an object. I am not sure we can merge this in. Where are you getting an error at the moment? I don't recall this being a bug, otherwise some of our tasks like Pickcube-v1 should not work at all since they use a hidden goal visual object for observations. |
|
First, this issue came up in my own environment implementation. Like I described, I get the pose of hidden objects at each timestep because I don't want to store an extra copy and waste GPU memory. About the overhead — this function won't be called at every timestep anyway, unless I need to hide an object to get observations and then show it again for rendering. Otherwise, it'll just exit immediately each time because the condition Also, the original code called: That's way more expensive than the current loop that just assigns attributes, since it has to apply and sync rigid body data for all objects in the environment, not just the hidden ones. Quick side note — if we want to optimize further, I can submit a new change that stores references to the RenderBodyComponent during initialization, so we don't have to call |
|
The hide and show functions for objects are called each time when rendering anything (whether you want RGB observations, or render for human reviewing), so it should slow things down somewhat. Moreover, after running example code in ManiSkill where objects are hidden I do not get the issue where pose data is wrong. When visual only hidden objects are teleported far away, if you retrieve pose data you are given data with the 9999 subtracted eg self.goal_site.pose.p gives you the right data in pick cube |
|
I found that the issue occurs with merged Actors. Since each environment uses different objects, they need to be added separately to But I recommend switching to the new version anyway, because when object meshes become complex and non-convex, from mani_skill.utils.building.ground import build_ground
from mani_skill.utils.building import actors
from mani_skill.utils.structs.actor import Actor
import gymnasium as gym
import numpy as np
import torch
import sapien.core as sapien
from typing import Any, Dict, Union
from mani_skill.agents.robots import Panda, Fetch
from mani_skill.envs.sapien_env import BaseEnv
from mani_skill.utils.registration import register_env
@register_env("test", max_episode_steps=100)
class TestEnv(BaseEnv):
SUPPORTED_ROBOTS = ["panda", "fetch"]
agent: Union[Panda, Fetch]
def __init__(self, *args, robot_uids="panda", **kwargs):
super().__init__(*args, robot_uids=robot_uids, **kwargs)
def _load_agent(self, options: dict):
super()._load_agent(options, sapien.Pose(p=[0, 0, 0]))
def _load_scene(self, options: dict):
self.ground = build_ground(self.scene, altitude=0.0)
self.obj = actors.build_cube(self.scene, 0.05, [1, 0, 0, 1], "cube", initial_pose=sapien.Pose(p=[0.5, 0, 0.1]))
target_objs = []
for i in range(self.num_envs):
target_obj = actors.build_cube(self.scene, 0.05, [0, 1, 0, 1], f"target_cube_{i}", initial_pose=sapien.Pose(p=[0.0, 0.0, 0.025]), body_type="kinematic", add_collision=False)
target_objs.append(target_obj)
self._hidden_objects.append(target_obj)
self.remove_from_state_dict_registry(target_obj)
self.target_obj = Actor.merge(target_objs, name="target_objs")
def _initialize_episode(self, env_idx: torch.Tensor, options: dict):
pass
def _get_obs_extra(self, info: Dict):
return {}
def compute_dense_reward(self, obs: Any, action: torch.Tensor, info: Dict):
pass
def compute_normalized_dense_reward(self, obs: Any, action: torch.Tensor, info: Dict):
return 0
if __name__ == "__main__":
env = gym.make("test", num_envs=2, robot_uids="panda", render_mode="human")
obs, info = env.reset()
for _ in range(1000):
action = env.action_space.sample()
obs, reward, terminated, truncated, info = env.step(action)
print(env.unwrapped.target_obj.pose.p)
env.render() |
|
Ah i understand the bug now. Didn't expect there to be merged hidden actors. I'll take a look and benchmark a bit and make a decision later, thank you! |
In the current implementation, hiding objects is achieved by setting their positions to
9999. However, this causes an issue when the target object has a hidden property and its position is retrieved insideevaluate().During the following execution loop:
The original
hide_visual()changes the object's position. One possible solution would be to callshow_visual()immediately afterhide_visual()and collecting sensor data, but this introduces unnecessary overhead for eachhide_visual()call.To avoid breaking the current design (where
show_visual()is only called when display is needed), I propose hiding only the visual body of the object instead of changing its position. This achieves the same visual hiding effect without altering the object's actual position.