From ad602d9dec5c54f0ae142a034a6a1be09bdf7999 Mon Sep 17 00:00:00 2001 From: Haoran Geng <71596067+geng-haoran@users.noreply.github.com> Date: Thu, 3 Sep 2026 04:46:06 +0000 Subject: [PATCH] fix(sim): report joint_pos_target in sorted order on isaacgym and pybullet MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit joint_pos/joint_vel/joint_effort_target are emitted in alphabetically-sorted joint order (isaacgym via _get_joint_ids_reindex, pybullet via joint_reindex), but both backends assembled the reported joint_pos_target by iterating their native URDF joint order instead — isaacgym from _joint_info[...]["names"] in _joint_pos_target_from_cache, pybullet from object_joint_order in _get_states. Whenever a robot's native joint order is not already alphabetical (e.g. numeric names joint_2/joint_10, or A,C,B), joint_pos_target[i] then referred to a different joint than joint_pos[i] — a silent index misalignment for downstream consumers that assume the fields share an ordering. Iterate _get_joint_names(..., sort=True) in both materializers instead. Values are name-keyed, so only the output ordering changes; the control path (isaacgym _set_dof_targets / _get_action_array_all and pybullet _apply_action, which legitimately drive the articulation in native order) is untouched. Completes the fix started in 92755f6 for sapien2/genesis, bringing isaacgym and pybullet into parity with sapien3/mujoco/mjx. Adds a general (no-GPU) AST regression guard pinning that both materializers build joint_pos_target from the sorted joint-name list and no longer reference the native-order list. Verified by inspection and the AST guard; the isaacgym and pybullet backends are not runnable in this environment (isaacgym needs a GPU and special import order), so the live end-to-end path could not be executed here. --- CHANGELOG.md | 1 + metasim/sim/isaacgym/isaacgym.py | 8 +- metasim/sim/pybullet/pybullet.py | 8 +- .../test/test_joint_target_order_general.py | 91 +++++++++++++++++++ 4 files changed, 106 insertions(+), 2 deletions(-) create mode 100644 metasim/test/test_joint_target_order_general.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 6e1726e..2ac64cc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -36,6 +36,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed - `RLTaskEnv.step` publishes the *terminal* observation in `info["observations"]["raw"]["obs"]` instead of the episode's first one (off-policy truncation bootstraps in clean_rl SAC/TD3 and FastTD3 read it). +- IsaacGym and PyBullet reported `joint_pos_target` in native DoF order while `joint_pos` is in sorted-name order; both now use `get_joint_names(sort=True)` (completes #12). - MuJoCo: `` is reserved by default; humanoid + mesh scenes no longer die with `mj_stackAlloc: out of memory` (get_started/10_mount_camera.py). - `hf_util`: a symlinked `roboverse_data` is no longer refused as path traversal; concurrent diff --git a/metasim/sim/isaacgym/isaacgym.py b/metasim/sim/isaacgym/isaacgym.py index 7a779cc..b87edb2 100644 --- a/metasim/sim/isaacgym/isaacgym.py +++ b/metasim/sim/isaacgym/isaacgym.py @@ -834,7 +834,13 @@ def _joint_pos_target_from_cache(self, robot) -> torch.Tensor | None: cache = self._actions_cache if not cache or isinstance(cache, (torch.Tensor, np.ndarray)): return None - joint_names = self._joint_info[robot.name]["names"] + # Iterate joints in alphabetically-sorted order so the reported + # ``joint_pos_target`` aligns with ``joint_pos`` (emitted via + # ``_get_joint_ids_reindex``, i.e. sorted-name order). ``_joint_info[...]["names"]`` + # is native DOF order, so using it produced a target vector misaligned with + # ``joint_pos`` whenever the URDF joint order was not already alphabetical. + # Values are name-keyed, so only the output ordering changes. + joint_names = self._get_joint_names(robot.name, sort=True) targets_per_env = [] for env_idx, env_action in enumerate(cache): if env_idx >= self._num_envs: diff --git a/metasim/sim/pybullet/pybullet.py b/metasim/sim/pybullet/pybullet.py index 8e8530b..075e9a6 100644 --- a/metasim/sim/pybullet/pybullet.py +++ b/metasim/sim/pybullet/pybullet.py @@ -449,8 +449,14 @@ def _get_states(self, env_ids=None) -> TensorState: cached_action = (self._actions_cache or {}).get(robot.name) if cached_action is not None and cached_action.get("dof_pos_target") is not None: dof_pos_target = cached_action["dof_pos_target"] + # Iterate joints in alphabetically-sorted order so the reported + # ``joint_pos_target`` aligns with ``joint_pos``/``joint_vel`` (emitted + # via ``joint_reindex``, i.e. sorted-name order). ``object_joint_order`` + # is native URDF order, so using it produced a target vector misaligned + # with ``joint_pos`` whenever that order was not already alphabetical. + # Values are name-keyed, so only the output ordering changes. joint_pos_target = torch.tensor( - [dof_pos_target[name] for name in self.object_joint_order[robot.name]], + [dof_pos_target[name] for name in self._get_joint_names(robot.name, sort=True)], dtype=torch.float32, ).unsqueeze(0) state = RobotState( diff --git a/metasim/test/test_joint_target_order_general.py b/metasim/test/test_joint_target_order_general.py new file mode 100644 index 0000000..04d3184 --- /dev/null +++ b/metasim/test/test_joint_target_order_general.py @@ -0,0 +1,91 @@ +"""Regression guard: ``joint_pos_target`` must be reported in the same +alphabetically-sorted joint order as ``joint_pos``/``joint_vel`` on every +backend that materializes it from a name-keyed action cache. + +Motivation: ``joint_pos``/``joint_vel``/``joint_effort_target`` are emitted in +sorted-name order (via ``joint_reindex`` / ``_get_joint_ids_reindex``), but +several backends assembled the reported ``joint_pos_target`` by iterating their +*native* URDF joint order instead. Whenever a robot's native joint order is not +already alphabetical (e.g. numeric names ``joint_2``/``joint_10``, or ``A,C,B``), +``joint_pos_target[i]`` then referred to a different joint than ``joint_pos[i]`` +— a silent index misalignment. Commit 92755f6 fixed this on sapien2/genesis; +isaacgym and pybullet were the remaining offenders. + +The faithful check needs a live backend (GPU/import order), which CI can't run, +so this is a static AST guard instead: for each backend it pins that the +``joint_pos_target`` materializer iterates ``_get_joint_names(..., sort=True)`` +and no longer references the native-order joint list. Pure-Python, no sim env, +no GPU — runs under ``-k general``. +""" + +from __future__ import annotations + +import ast +from pathlib import Path + +import pytest + +_SIM_ROOT = Path(__file__).resolve().parents[1].joinpath("sim") + + +def _find_function(tree: ast.AST, class_name: str, func_name: str) -> ast.FunctionDef: + cls = next(n for n in ast.walk(tree) if isinstance(n, ast.ClassDef) and n.name == class_name) + return next(n for n in ast.walk(cls) if isinstance(n, ast.FunctionDef) and n.name == func_name) + + +def _calls_get_joint_names_sorted(fn: ast.FunctionDef) -> bool: + """True if ``fn`` calls ``*._get_joint_names(...)`` with ``sort=True``. + + Accepts either the keyword form ``sort=True`` or the positional form + ``_get_joint_names(obj_name, True)`` — both mean sorted order. + """ + for node in ast.walk(fn): + if not (isinstance(node, ast.Call) and isinstance(node.func, ast.Attribute)): + continue + if node.func.attr != "_get_joint_names": + continue + for kw in node.keywords: + if kw.arg == "sort" and isinstance(kw.value, ast.Constant) and kw.value.value is True: + return True + # positional sort is the 2nd arg after obj_name + if len(node.args) >= 2 and isinstance(node.args[1], ast.Constant) and node.args[1].value is True: + return True + return False + + +def _references_attr(fn: ast.FunctionDef, attr: str) -> bool: + return any(isinstance(node, ast.Attribute) and node.attr == attr for node in ast.walk(fn)) + + +# (source file, class, function that materializes joint_pos_target, native-order +# attribute that must NOT be used to build it). +_CASES = [ + pytest.param( + "isaacgym/isaacgym.py", "IsaacgymHandler", "_joint_pos_target_from_cache", "_joint_info", id="isaacgym" + ), + pytest.param("pybullet/pybullet.py", "SinglePybulletHandler", "_get_states", "object_joint_order", id="pybullet"), +] + + +@pytest.mark.general +@pytest.mark.parametrize("rel_path,class_name,func_name,native_attr", _CASES) +def test_joint_pos_target_uses_sorted_joint_order(rel_path: str, class_name: str, func_name: str, native_attr: str): + """The ``joint_pos_target`` materializer must iterate sorted joint names. + + Fails if a backend reverts to iterating its native joint order, which would + re-open the silent ``joint_pos_target``/``joint_pos`` index misalignment + fixed for sapien2/genesis in 92755f6 and here for isaacgym/pybullet. + """ + source = _SIM_ROOT.joinpath(rel_path).read_text(encoding="utf-8") + fn = _find_function(ast.parse(source), class_name, func_name) + + assert _calls_get_joint_names_sorted(fn), ( + f"{class_name}.{func_name} must build joint_pos_target from " + f"_get_joint_names(..., sort=True) so it aligns with joint_pos " + f"(sorted-name order); no such call found." + ) + assert not _references_attr(fn, native_attr), ( + f"{class_name}.{func_name} still references native joint order " + f"({native_attr!r}) — joint_pos_target[i] would refer to a different " + f"joint than joint_pos[i] whenever the native order is not alphabetical." + )