Conversation
…mbodiChain into yueci/adapt-dexsim040
Co-authored-by: liwenfeng <liwenfeng@dexforce.top>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 110 out of 110 changed files in this pull request and generated 8 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| class ContactTest: | ||
| def setup_simulation(self, sim_device, enable_rt): | ||
| def setup_simulation(self, sim_device): | ||
| sim_cfg = SimulationManagerCfg( | ||
| width=1920, | ||
| height=1080, | ||
| num_envs=2, | ||
| headless=True, | ||
| physics_dt=1.0 / 100.0, # Physics timestep (100 Hz) | ||
| sim_device=sim_device, | ||
| enable_rt=enable_rt, # Enable ray tracing for better visuals | ||
| ) |
There was a problem hiding this comment.
The Raster/FastRT subclasses no longer differ (no renderer selection is passed into SimulationManagerCfg), and RenderCfg is imported but unused. Consider wiring render_cfg=RenderCfg(renderer=...) into SimulationManagerCfg so the tests actually exercise the intended backends and the imports/class names remain meaningful.
| def to_dexsim_flags(self): | ||
| if self.renderer == "legacy": | ||
| return Renderer.FILAMENT | ||
| elif self.renderer == "hybrid": | ||
| return Renderer.HYBRID | ||
| elif self.renderer == "fast-rt": | ||
| return Renderer.FASTRT | ||
| elif self.renderer == "rt": | ||
| return Renderer.OFFLINERT | ||
| else: | ||
| logger.log_error( | ||
| f"Invalid renderer type '{self.renderer}' specified. Must be one of 'legacy', 'hybrid', 'fast-rt', or 'rt'." | ||
| ) | ||
|
|
There was a problem hiding this comment.
RenderCfg.to_dexsim_flags() logs an error for an unknown renderer but returns None, which can propagate into world_config.renderer and fail later in hard-to-debug ways. Prefer raising a ValueError (or returning a safe default) to enforce invalid values are handled immediately.
| if device.type == "cuda": | ||
| gpu_index = device.index | ||
| if gpu_index is None: | ||
| gpu_index = torch.cuda.current_device() | ||
| gym_env_cfg.sim_cfg.sim_device = torch.device(f"cuda:{gpu_index}") | ||
| if hasattr(gym_env_cfg.sim_cfg, "gpu_id"): | ||
| gym_env_cfg.sim_cfg.gpu_id = gpu_index | ||
| else: | ||
| gym_env_cfg.sim_cfg.sim_device = torch.device("cpu") | ||
| gym_env_cfg.sim_cfg.headless = headless | ||
| gym_env_cfg.sim_cfg.enable_rt = enable_rt | ||
| gym_env_cfg.sim_cfg.gpu_id = local_rank if distributed else gpu_id | ||
| gym_env_cfg.sim_cfg.render_cfg = RenderCfg(renderer=renderer) | ||
| gym_env_cfg.sim_cfg.gpu_id = gpu_id | ||
|
|
There was a problem hiding this comment.
gym_env_cfg.sim_cfg.gpu_id is first set from the resolved CUDA device index (via gpu_index), but then immediately overwritten with the trainer config's gpu_id. This can break multi-GPU/distributed runs where each rank must target a distinct GPU. Consider only setting gpu_id once (derived from device/local_rank), or make the override conditional so it can’t undo the device-based selection.
| # Global default renderer settings for simulation | ||
| DEFAULT_RENDERER: Literal["legacy", "hybrid", "fast-rt", "rt"] = "hybrid" | ||
|
|
||
|
|
||
| @configclass | ||
| class RenderCfg: | ||
| renderer: Literal["legacy", "hybrid", "fast-rt", "rt"] = field( | ||
| default_factory=lambda: DEFAULT_RENDERER | ||
| ) | ||
| """Renderer backend to use for the simulation. Options are 'legacy', 'hybrid', 'fast-rt', and 'rt'. | ||
|
|
||
| Note: | ||
| - 'legacy' is the traditional rasterization-based renderer and the default for backward compatibility. | ||
| - 'hybrid' uses ray tracing for shadows and reflections while keeping rasterization for primary rendering, | ||
| providing a balance between performance and visual quality. | ||
| - 'fast-rt' is a fully ray-traced renderer for maximum visual fidelity, but may have higher computational cost. | ||
| - 'rt' is an offline ray-traced renderer for maximum visual fidelity, suitable for high-quality rendering tasks. | ||
| """ |
There was a problem hiding this comment.
DEFAULT_RENDERER is set to "hybrid", but the RenderCfg docstring says "legacy" is the default for backward compatibility. Either change DEFAULT_RENDERER to "legacy" or update the docstring (and any other docs) so the stated default matches runtime behavior.
| def add_env_launcher_args_to_parser(parser: argparse.ArgumentParser) -> None: | ||
| """Add common environment launcher arguments to an existing argparse parser. | ||
|
|
||
| This function adds the following arguments to the provided parser: | ||
| --num_envs: Number of environments to run in parallel (default: 1) | ||
| --device: Device to run the environment on (default: 'cpu') | ||
| --headless: Whether to perform the simulation in headless mode (default: False) | ||
| --enable_rt: Whether to use RTX rendering backend for the simulation (default: False) | ||
| --renderer: Renderer backend to use for the simulation. Options are 'legacy', 'hybrid', and 'fast-rt'. (default: 'legacy') | ||
| --gpu_id: The GPU ID to use for the simulation (default: 0) | ||
| --gym_config: Path to gym config file (default: '') | ||
| --action_config: Path to action config file (default: None) | ||
| --preview: Whether to preview the environment after launching (default: False) | ||
| --filter_visual_rand: Whether to filter out visual randomization (default: False) | ||
| --filter_dataset_saving: Whether to filter out dataset saving (default: False) | ||
|
|
||
| Note: | ||
| 1. In preview mode, the environment will be launched and keep running in a loop for user interaction. | ||
|
|
||
| Args: | ||
| parser (argparse.ArgumentParser): The parser to which arguments will be added. | ||
| """ | ||
| parser.add_argument( | ||
| "--num_envs", | ||
| help="The number of environments to run in parallel.", | ||
| default=1, | ||
| type=int, | ||
| ) | ||
| parser.add_argument( | ||
| "--device", | ||
| type=str, | ||
| default="cpu", | ||
| help="Device to run the environment on, e.g., 'cpu' or 'cuda'.", | ||
| ) | ||
| parser.add_argument( | ||
| "--headless", | ||
| help="Whether to perform the simulation in headless mode.", | ||
| default=False, | ||
| action="store_true", | ||
| ) | ||
| parser.add_argument( | ||
| "--renderer", | ||
| type=str, | ||
| choices=["legacy", "hybrid", "fast-rt", "rt"], | ||
| default="hybrid", | ||
| help="Renderer backend to use for the simulation.", | ||
| ) |
There was a problem hiding this comment.
The add_env_launcher_args_to_parser docstring says the default renderer is 'legacy', but the argparse option sets default="hybrid". Please align the docstring with the actual default (or change the default to match the documented behavior).
| def _build_env_cfg( | ||
| gym_config_path: str, | ||
| num_envs: int | None, | ||
| headless: bool, | ||
| enable_rt: bool, | ||
| device: torch.device, | ||
| gpu_id: int, | ||
| ): | ||
| gym_config_data = load_json(gym_config_path) | ||
| gym_env_cfg = config_to_cfg( | ||
| gym_config_data, manager_modules=DEFAULT_MANAGER_MODULES | ||
| ) | ||
| if num_envs is not None: | ||
| gym_env_cfg.num_envs = int(num_envs) | ||
| if gym_env_cfg.sim_cfg is None: | ||
| gym_env_cfg.sim_cfg = SimulationManagerCfg() | ||
| gym_env_cfg.seed = getattr(gym_env_cfg, "seed", None) | ||
| gym_env_cfg.sim_cfg.headless = headless | ||
| gym_env_cfg.sim_cfg.enable_rt = enable_rt | ||
| gym_env_cfg.sim_cfg.gpu_id = gpu_id | ||
| gym_env_cfg.sim_cfg.sim_device = device | ||
| return gym_config_data, gym_env_cfg |
There was a problem hiding this comment.
_build_env_cfg() no longer accepts enable_rt, but it also never sets sim_cfg.render_cfg from config/CLI. This changes behavior vs. previous defaults (often raster/legacy) and makes the renderer impossible to control from the benchmark runtime. Consider threading a renderer argument through and setting gym_env_cfg.sim_cfg.render_cfg = RenderCfg(renderer=renderer) (or reading it from the loaded config).
| def pytest_addoption(parser): | ||
| parser.addoption( | ||
| "--renderer", | ||
| action="store", | ||
| default=None, | ||
| help="Specify the renderer backend: legacy, hybrid, or fast-rt", | ||
| ) | ||
|
|
||
|
|
||
| def pytest_configure(config): | ||
| renderer = config.getoption("--renderer") | ||
| if renderer: | ||
| if renderer not in ["legacy", "hybrid", "fast-rt"]: | ||
| pytest.exit( | ||
| f"Invalid renderer: {renderer}. Must be one of 'legacy', 'hybrid', 'fast-rt'" | ||
| ) |
There was a problem hiding this comment.
Pytest --renderer validation rejects "rt", but the rest of the codebase/CLI accepts it (e.g., add_env_launcher_args_to_parser(... choices=[..., "rt"])). Consider adding "rt" here (and updating the help/error text) so tests can be run against the offline RT backend too.
| # Configure the simulation | ||
| sim_cfg = SimulationManagerCfg( | ||
| width=1920, | ||
| height=1080, | ||
| num_envs=args.num_envs, | ||
| headless=args.headless, | ||
| headless=True, | ||
| physics_dt=1.0 / 100.0, # Physics timestep (100 Hz) | ||
| sim_device=args.device, | ||
| enable_rt=args.enable_rt, # Enable ray tracing for better visuals | ||
| render_cfg=RenderCfg( | ||
| renderer=args.renderer | ||
| ), # Enable ray tracing for better visuals | ||
| ) |
There was a problem hiding this comment.
SimulationManagerCfg is created with headless=True unconditionally, but later logic uses args.headless to decide whether to open a window. This is inconsistent (the manager may never create a window even when --headless is false). Pass headless=args.headless into SimulationManagerCfg so the CLI flag actually takes effect.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 111 out of 111 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
embodichain/lab/gym/utils/gym_utils.py:862
--gym_configis nowrequired=Falsewith default "", butbuild_env_cfg_from_args()unconditionally callsload_json(args.gym_config), which will raiseFileNotFoundErrorwhen the flag is omitted. Either make--gym_configrequired again, or add a real default/guard (e.g., error with a clear message when empty).
parser.add_argument(
"--gym_config",
type=str,
help="Path to gym config file.",
default="",
required=False,
)
parser.add_argument(
"--action_config", type=str, help="Path to action config file.", default=None
)
parser.add_argument(
"--preview",
help="Whether to preview the environment after launching.",
default=False,
action="store_true",
)
parser.add_argument(
"--filter_visual_rand",
help="Whether to filter out visual randomization.",
default=False,
action="store_true",
)
parser.add_argument(
"--filter_dataset_saving",
help="Whether to filter out dataset saving.",
default=False,
action="store_true",
)
def merge_args_with_gym_config(args: argparse.Namespace, gym_config: dict) -> dict:
"""Merge command-line arguments with gym configuration.
Command-line arguments will override the corresponding values in the gym configuration.
Args:
args (argparse.Namespace): The parsed command-line arguments.
gym_config (dict): The original gym configuration dictionary.
Returns:
dict: The merged gym configuration dictionary.
"""
merged_config = deepcopy(gym_config)
merged_config["num_envs"] = args.num_envs
merged_config["device"] = args.device
merged_config["headless"] = args.headless
merged_config["renderer"] = args.renderer
merged_config["gpu_id"] = args.gpu_id
merged_config["arena_space"] = args.arena_space
return merged_config
def build_env_cfg_from_args(
args: argparse.Namespace,
) -> tuple["EmbodiedEnvCfg", dict, dict]:
"""Build environment configuration from command-line arguments.
Args:
args (argparse.Namespace): The parsed command-line arguments.
Returns:
tuple[EmbodiedEnvCfg, dict, dict]: A tuple containing the environment configuration object,
the original gym configuration dictionary, and the action configuration dictionary.
"""
from embodichain.utils.utility import load_json
from embodichain.lab.gym.envs import EmbodiedEnvCfg
from embodichain.lab.sim import SimulationManagerCfg
from embodichain.lab.sim.cfg import RenderCfg
gym_config = load_json(args.gym_config)
gym_config = merge_args_with_gym_config(args, gym_config)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| choices=["dynamic", "kinematic", "static"], | ||
| default="kinematic", | ||
| default="dynamic", | ||
| help="Body type for rigid objects (default: kinematic).", |
There was a problem hiding this comment.
The --body_type argument now defaults to "dynamic", but the help string still says "(default: kinematic)". Update the help text (or revert the default) so CLI usage information is accurate.
| help="Body type for rigid objects (default: kinematic).", | |
| help="Body type for rigid objects (default: dynamic).", |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 110 out of 110 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Global default renderer settings for simulation | ||
| DEFAULT_RENDERER: Literal["legacy", "hybrid", "fast-rt", "rt"] = "hybrid" | ||
|
|
||
|
|
||
| @configclass | ||
| class RenderCfg: | ||
| renderer: Literal["legacy", "hybrid", "fast-rt", "rt"] = field( | ||
| default_factory=lambda: DEFAULT_RENDERER | ||
| ) | ||
| """Renderer backend to use for the simulation. Options are 'legacy', 'hybrid', 'fast-rt', and 'rt'. | ||
|
|
||
| Note: | ||
| - 'legacy' is the traditional rasterization-based renderer and the default for backward compatibility. | ||
| - 'hybrid' uses ray tracing for shadows and reflections while keeping rasterization for primary rendering, |
There was a problem hiding this comment.
DEFAULT_RENDERER is set to "hybrid", but the RenderCfg docstring says "legacy" is the default for backward compatibility. Please align the code and docstring (either change the default constant or update the documentation to match the actual default).
|
|
||
| # Enable ray tracing rendering | ||
| python scripts/tutorials/sim/create_robot.py --enable_rt | ||
| python scripts/tutorials/sim/create_robot.py --renderer |
There was a problem hiding this comment.
The docs show --renderer being used as a flag, but the CLI expects a value (e.g., --renderer hybrid or --renderer fast-rt). As written, this example command will fail argument parsing; please include an explicit renderer argument in the example.
| python scripts/tutorials/sim/create_robot.py --renderer | |
| python scripts/tutorials/sim/create_robot.py --renderer hybrid |
| if sim_config.headless is False: | ||
| self._window = self._world.get_windows() | ||
| self._register_default_window_control() | ||
| # self._register_default_window_control() | ||
|
|
There was a problem hiding this comment.
The call to _register_default_window_control() is now commented out during initialization. Since there are no remaining call sites, default window interactions (selection/raycast helper) appear to be disabled even when a window is created. If this was done to work around an engine issue, consider guarding it behind a config flag or restricting it to the affected renderer mode instead of disabling it globally.
| parser.add_argument( | ||
| "--gym_config", | ||
| type=str, | ||
| help="Path to gym config file.", | ||
| default="", | ||
| required=True, | ||
| required=False, | ||
| ) |
There was a problem hiding this comment.
--gym_config is now marked required=False, but build_env_cfg_from_args() unconditionally calls load_json(args.gym_config). This means run-env/run-agent can now parse successfully without --gym_config and then fail later with a confusing file error. Consider keeping --gym_config required for those CLIs, or add an explicit validation/error message in build_env_cfg_from_args when it is missing/empty.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 111 out of 111 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| class TestContactFastRT(ContactTest): | ||
| def setup_method(self): | ||
| self.setup_simulation("cpu", enable_rt=True) | ||
| self.setup_simulation("cpu") | ||
|
|
There was a problem hiding this comment.
TestContactFastRT* currently configures the simulation exactly the same way as the raster tests (no render_cfg override), so it doesn't actually test the fast-rt renderer path. Either set render_cfg=RenderCfg(renderer="fast-rt") for the FastRT variants, or remove/parameterize these classes and rely on the --renderer pytest option in tests/conftest.py.
| config = SimulationManagerCfg( | ||
| headless=True, | ||
| sim_device=args.device, | ||
| arena_space=3.0, | ||
| enable_rt=args.enable_rt, | ||
| render_cfg=RenderCfg(renderer=args.renderer), |
There was a problem hiding this comment.
SimulationManagerCfg is constructed with headless=True, but later the script checks if not args.headless: sim.open_window(). As written, --headless won't control window creation and the window may not open as intended. Use headless=args.headless when building SimulationManagerCfg (or remove the open_window() branch if the script is meant to always run headless).
| sim_cfg = SimulationManagerCfg( | ||
| width=1920, | ||
| height=1080, | ||
| headless=True, | ||
| physics_dt=1.0 / 100.0, # Physics timestep (100 Hz) |
There was a problem hiding this comment.
SimulationManagerCfg is constructed with headless=True, but later the script checks if not args.headless: sim.open_window(). As written, --headless won't control window creation and the window may not open as intended. Use headless=args.headless when building SimulationManagerCfg (or remove the open_window() branch if the script is meant to always run headless).
| @@ -61,7 +52,7 @@ def main(): | |||
| height=1080, | |||
| physics_dt=1.0 / 100.0, | |||
There was a problem hiding this comment.
This tutorial adds common launcher args (including --headless), but SimulationManagerCfg is constructed without headless=args.headless. As a result, --headless is ignored and the script always opens a window. Please wire args.headless into SimulationManagerCfg and only call open_window() when headless is False.
| config = SimulationManagerCfg( | ||
| headless=True, | ||
| sim_device=args.device, | ||
| arena_space=3.0, | ||
| enable_rt=args.enable_rt, | ||
| render_cfg=RenderCfg(renderer=args.renderer), |
There was a problem hiding this comment.
SimulationManagerCfg is constructed with headless=True, but later the script checks if not args.headless: sim.open_window(). As written, --headless won't control window creation and the window may not open as intended. Use headless=args.headless when building SimulationManagerCfg (or remove the open_window() branch if the script is meant to always run headless).
| height=1080, | ||
| num_envs=args.num_envs, | ||
| headless=args.headless, | ||
| headless=True, | ||
| physics_dt=1.0 / 100.0, # Physics timestep (100 Hz) | ||
| sim_device=args.device, |
There was a problem hiding this comment.
SimulationManagerCfg is created with headless=True, but later the script does if not args.headless: sim.open_window(). With the current config, --headless won't control window creation and the window may never open. Use headless=args.headless when constructing sim_cfg (or remove the window-opening branch if always headless is intended).
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 112 out of 112 changed files in this pull request and generated 8 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Global default renderer settings for simulation | ||
| DEFAULT_RENDERER: Literal["legacy", "hybrid", "fast-rt", "rt"] = "hybrid" | ||
|
|
||
|
|
||
| @configclass | ||
| class RenderCfg: | ||
| renderer: Literal["legacy", "hybrid", "fast-rt", "rt"] = field( | ||
| default_factory=lambda: DEFAULT_RENDERER | ||
| ) | ||
| """Renderer backend to use for the simulation. Options are 'legacy', 'hybrid', 'fast-rt', and 'rt'. | ||
|
|
||
| Note: | ||
| - 'legacy' is the traditional rasterization-based renderer and the default for backward compatibility. | ||
| - 'hybrid' uses ray tracing for shadows and reflections while keeping rasterization for primary rendering, | ||
| providing a balance between performance and visual quality. | ||
| - 'fast-rt' is a fully ray-traced renderer for maximum visual fidelity, but may have higher computational cost. | ||
| - 'rt' is an offline ray-traced renderer for maximum visual fidelity, suitable for high-quality rendering tasks. | ||
| """ |
There was a problem hiding this comment.
DEFAULT_RENDERER is set to "hybrid", but the RenderCfg docstring states 'legacy' is the default for backward compatibility. This mismatch is likely to change behavior for existing callers/tests (CPU headless runs may unexpectedly use RT backends). Align the constant and documentation (e.g., default to legacy unless explicitly overridden).
|
|
||
| # Enable ray tracing rendering | ||
| python scripts/tutorials/sim/create_sensor.py --enable_rt | ||
| python scripts/tutorials/sim/create_sensor.py --renderer | ||
|
|
There was a problem hiding this comment.
This example now documents --renderer as a flag, but the CLI requires a value (e.g. --renderer fast-rt). Update the command example to include a concrete backend value, and consider mentioning available choices.
| def setup_simulation(self, sim_device): | ||
| # Setup SimulationManager | ||
| config = SimulationManagerCfg( | ||
| headless=True, sim_device=sim_device, enable_rt=enable_rt, num_envs=NUM_ENVS | ||
| headless=True, | ||
| sim_device=sim_device, | ||
| num_envs=NUM_ENVS, | ||
| ) |
There was a problem hiding this comment.
The TestStereoCameraFastRT* classes no longer enable an RT backend; SimulationManagerCfg is created without render_cfg, so these tests won’t actually validate FastRT behavior (and RenderCfg import is unused). Pass an explicit render_cfg=RenderCfg(renderer=...) per variant.
| trainer: | ||
| gym_config: configs/agents/rl/basic/cart_pole/gym_config.json | ||
| exp_name: cart_pole | ||
| device: cpu | ||
| headless: true | ||
| enable_rt: false | ||
| gpu_id: 0 | ||
| num_envs: 64 | ||
| iterations: 200 |
There was a problem hiding this comment.
These benchmark configs removed enable_rt but don’t specify the replacement renderer field. If the previous intent was “no RT”, add renderer: legacy under trainer to preserve behavior and avoid silently switching to the global default renderer.
| """Open the simulation window.""" | ||
| self._world.open_window() | ||
| self._window = self._world.get_windows() | ||
| self._register_default_window_control() | ||
| # self._register_default_window_control() | ||
| self.is_window_opened = True |
There was a problem hiding this comment.
The default window controls are never registered when opening a window because the call is commented out. This makes _register_default_window_control() effectively unused and may break expected interactions in RT mode. Consider restoring the call (it already no-ops for non-RT) or removing the helper if intentionally deprecated.
| def setup_simulation(self, sim_device): | ||
| # Setup SimulationManager | ||
| config = SimulationManagerCfg( | ||
| headless=True, sim_device=sim_device, enable_rt=enable_rt, num_envs=NUM_ENVS | ||
| headless=True, | ||
| sim_device=sim_device, | ||
| num_envs=NUM_ENVS, | ||
| ) |
There was a problem hiding this comment.
The TestCameraFastRT* classes no longer enable an RT backend; all variants call setup_simulation() which builds SimulationManagerCfg without render_cfg. As a result, these tests won’t exercise FastRT vs raster differences (and the RenderCfg import is unused). Consider passing render_cfg=RenderCfg(renderer=...) based on the test variant.
| trainer: | ||
| gym_config: configs/agents/rl/push_cube/gym_config.json | ||
| exp_name: push_cube | ||
| device: cpu | ||
| headless: true | ||
| enable_rt: false | ||
| gpu_id: 0 | ||
| num_envs: 64 | ||
| iterations: 200 |
There was a problem hiding this comment.
These benchmark configs removed enable_rt but don’t specify the replacement renderer field. If the previous intent was “no RT”, add renderer: legacy under trainer to preserve behavior and avoid silently switching to the global default renderer.
| env = self._env.sim.get_env() | ||
| env.clean_materials() |
There was a problem hiding this comment.
This code shadows the env argument and calls clean_materials() on the raw dexsim env, which bypasses SimulationManager.clean_materials() (and its _visual_materials cache). Prefer calling env.sim.clean_materials() (or similar) and avoid reassigning env to a different type.
| env = self._env.sim.get_env() | |
| env.clean_materials() | |
| env.sim.clean_materials() |
Description
Type of change
Screenshots
Please attach before and after screenshots of the change if applicable.
Checklist
black .command to format the code base.