-
Notifications
You must be signed in to change notification settings - Fork 3.5k
Rendering correctness test determinism #5353
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -52,6 +52,15 @@ | |
| # | ||
| _PIXEL_L2_NORM_DIFFERENCE_THRESHOLD = 10.0 | ||
|
|
||
| # The max percentage of pixels allowed to differ. If the percentage exceeds this value, the test will fail. | ||
| # The value is set case by case based on the screen space taken up by the env in camera output images. It | ||
| # needs to be large enough to tolerate minor rendering noise while small enough to catch unexpected changes. | ||
| _MAX_DIFFERENT_PIXELS_PERCENTAGE_BY_ENV_NAME = { | ||
| "cartpole": 1.0, | ||
| "shadow_hand": 3.0, | ||
| "dexsuite_kuka": 4.0, | ||
| } | ||
|
|
||
| _OVRTX_DISABLED = pytest.mark.skip( | ||
| reason="OVRTX is optional and experimental feature and temporarily is excluded from testing." | ||
| ) | ||
|
|
@@ -66,9 +75,6 @@ | |
| # "img_result_path": str | None, "img_golden_path": str | None} | ||
| _COMPARISON_SCORES: list[dict] = [] | ||
|
|
||
| # Environment seed. | ||
| _ENV_SEED = 42 | ||
|
|
||
|
|
||
| # --------------------------------------------------------------------------- | ||
| # Fixtures | ||
|
|
@@ -643,7 +649,6 @@ def shadow_hand_env(request): | |
| env_cfg = _apply_overrides_to_env_cfg(env_cfg, override_args) | ||
|
|
||
| env_cfg.scene.num_envs = 4 | ||
| env_cfg.seed = _ENV_SEED | ||
|
|
||
| if data_type == "depth": | ||
| # Disable CNN forward pass as it cannot be meaningfully trained from depth alone and will raise a ValueError. | ||
|
|
@@ -652,7 +657,6 @@ def shadow_hand_env(request): | |
| env = None | ||
| try: | ||
| env = ShadowHandVisionEnv(env_cfg) | ||
| env.reset(seed=_ENV_SEED) | ||
| yield physics_backend, renderer, data_type, env | ||
| finally: | ||
| if env is not None: | ||
|
|
@@ -662,13 +666,13 @@ def shadow_hand_env(request): | |
| def test_shadow_hand(shadow_hand_env): | ||
| """Camera output must contain at least one non-zero pixel (Shadow Hand vision env).""" | ||
| physics_backend, renderer, _, env = shadow_hand_env | ||
|
|
||
| test_name = "shadow_hand" | ||
| _validate_camera_outputs( | ||
| "shadow_hand", | ||
| test_name, | ||
| physics_backend, | ||
| renderer, | ||
| env._tiled_camera.data.output, | ||
| max_different_pixels_percentage=8.0, | ||
| max_different_pixels_percentage=_MAX_DIFFERENT_PIXELS_PERCENTAGE_BY_ENV_NAME[test_name], | ||
| ) | ||
|
|
||
|
|
||
|
|
@@ -691,12 +695,10 @@ def cartpole_env(request): | |
| env_cfg = _apply_overrides_to_env_cfg(env_cfg, override_args) | ||
|
|
||
| env_cfg.scene.num_envs = 4 | ||
| env_cfg.seed = _ENV_SEED | ||
|
|
||
| env = None | ||
| try: | ||
| env = CartpoleCameraEnv(env_cfg) | ||
| env.reset(seed=_ENV_SEED) | ||
| yield physics_backend, renderer, data_type, env | ||
| finally: | ||
| if env is not None: | ||
|
|
@@ -706,13 +708,13 @@ def cartpole_env(request): | |
| def test_cartpole(cartpole_env): | ||
| """Camera output must contain at least one non-zero pixel (Cartpole camera env).""" | ||
| physics_backend, renderer, _, env = cartpole_env | ||
|
|
||
| test_name = "cartpole" | ||
| _validate_camera_outputs( | ||
| "cartpole", | ||
| test_name, | ||
| physics_backend, | ||
| renderer, | ||
| env._tiled_camera.data.output, | ||
| max_different_pixels_percentage=2.0, | ||
| max_different_pixels_percentage=_MAX_DIFFERENT_PIXELS_PERCENTAGE_BY_ENV_NAME[test_name], | ||
| ) | ||
|
|
||
|
|
||
|
|
@@ -739,12 +741,10 @@ def dexsuite_kuka_allegro_lift_env(request): | |
| env_cfg = _apply_overrides_to_env_cfg(env_cfg, override_args) | ||
|
|
||
| env_cfg.scene.num_envs = 4 | ||
| env_cfg.seed = _ENV_SEED | ||
|
|
||
| env = None | ||
| try: | ||
| env = ManagerBasedRLEnv(env_cfg) | ||
| env.reset(seed=_ENV_SEED) | ||
| yield physics_backend, renderer, data_type, env | ||
| finally: | ||
| if env is not None: | ||
|
|
@@ -754,13 +754,13 @@ def dexsuite_kuka_allegro_lift_env(request): | |
| def test_dexsuite_kuka_allegro_lift(dexsuite_kuka_allegro_lift_env): | ||
| """Camera output must contain at least one non-zero pixel (Dexsuite Kuka-Allegro Lift, single camera).""" | ||
| physics_backend, renderer, _, env = dexsuite_kuka_allegro_lift_env | ||
|
|
||
| test_name = "dexsuite_kuka" | ||
| _validate_camera_outputs( | ||
| "dexsuite_kuka", | ||
| test_name, | ||
| physics_backend, | ||
| renderer, | ||
| env.scene.sensors["base_camera"].data.output, | ||
| max_different_pixels_percentage=10.0, | ||
| max_different_pixels_percentage=_MAX_DIFFERENT_PIXELS_PERCENTAGE_BY_ENV_NAME[test_name], | ||
| ) | ||
|
|
||
|
|
||
|
|
@@ -769,34 +769,32 @@ def test_dexsuite_kuka_allegro_lift(dexsuite_kuka_allegro_lift_env): | |
| # --------------------------------------------------------------------------- | ||
|
|
||
| # Task IDs that expose camera/tiled_camera image observations; each is validated for non-blank rendering. | ||
| # The max different pixels percentage is set based on the screen space taken up by the env. | ||
| _RENDER_CORRECTNESS_TASK_IDS = [ | ||
| "Isaac-Cartpole-Albedo-Camera-Direct-v0", | ||
| "Isaac-Cartpole-Camera-Presets-Direct-v0", | ||
| "Isaac-Cartpole-Depth-Camera-Direct-v0", | ||
| "Isaac-Cartpole-RGB-Camera-Direct-v0", | ||
| "Isaac-Cartpole-SimpleShading-Constant-Camera-Direct-v0", | ||
| "Isaac-Cartpole-SimpleShading-Diffuse-Camera-Direct-v0", | ||
| "Isaac-Cartpole-SimpleShading-Full-Camera-Direct-v0", | ||
| "Isaac-Repose-Cube-Shadow-Vision-Direct-v0", | ||
| ("Isaac-Cartpole-Albedo-Camera-Direct-v0", "cartpole"), | ||
| ("Isaac-Cartpole-Camera-Presets-Direct-v0", "cartpole"), | ||
| ("Isaac-Cartpole-Depth-Camera-Direct-v0", "cartpole"), | ||
| ("Isaac-Cartpole-RGB-Camera-Direct-v0", "cartpole"), | ||
| ("Isaac-Cartpole-SimpleShading-Constant-Camera-Direct-v0", "cartpole"), | ||
| ("Isaac-Cartpole-SimpleShading-Diffuse-Camera-Direct-v0", "cartpole"), | ||
| ("Isaac-Cartpole-SimpleShading-Full-Camera-Direct-v0", "cartpole"), | ||
| ("Isaac-Repose-Cube-Shadow-Vision-Direct-v0", "shadow_hand"), | ||
| ] | ||
|
|
||
|
|
||
| @pytest.mark.parametrize("task_id", _RENDER_CORRECTNESS_TASK_IDS) | ||
| def test_registered_tasks(task_id): | ||
| @pytest.mark.parametrize("task_id, env_name", _RENDER_CORRECTNESS_TASK_IDS) | ||
| def test_registered_tasks(task_id, env_name): | ||
| """Camera output must be non-empty for each registered task with camera-based observations.""" | ||
| env = None | ||
| try: | ||
| env_cfg = parse_env_cfg(task_id, num_envs=4) | ||
| env_cfg.seed = _ENV_SEED | ||
|
|
||
| env = gym.make(task_id, cfg=env_cfg) | ||
| unwrapped: Any = env.unwrapped | ||
| sim = getattr(unwrapped, "sim", None) | ||
| if sim is not None: | ||
| sim._app_control_on_stop_handle = None | ||
|
|
||
| env.reset(seed=_ENV_SEED) | ||
|
|
||
| camera_outputs_nested_dict = _collect_camera_outputs(env) | ||
| num_camera_outputs = len(camera_outputs_nested_dict) | ||
|
Comment on lines
798
to
799
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The same pattern was removed from the other three env fixtures (
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is fine. we will check whether camera outputs exist and contain non-zero pixels inside
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Camera output buffers will be updated on invocation of Camera.data(): @property
def data(self) -> CameraData:
# update sensors if needed
self._update_outdated_buffers()
# return the data
return self._dataThere was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Got it, thanks for clarifying! I see now that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thank you for the clarification! You're right — the This is a nice pattern that decouples buffer population from the reset lifecycle. My concern about the Gymnasium contract was misplaced here since Isaac Lab's camera sensor implementation handles this internally. 👍 |
||
| assert num_camera_outputs == 1, f"[{task_id}] Expected 1 camera output, got {num_camera_outputs}." | ||
|
|
@@ -808,7 +806,7 @@ def test_registered_tasks(task_id): | |
| "default_physics", | ||
| "default_renderer", | ||
| camera_outputs, | ||
| max_different_pixels_percentage=5.0, | ||
| max_different_pixels_percentage=_MAX_DIFFERENT_PIXELS_PERCENTAGE_BY_ENV_NAME[env_name], | ||
| ) | ||
| finally: | ||
| if env is not None: | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it was 2.0 before