Skip to content

feat: G1 per-joint kp/kd domain randomization (issue #129) - #256

Closed
lemon-star608 wants to merge 1 commit into
mainfrom
g1_deploy
Closed

lemon-star608 wants to merge 1 commit into
mainfrom
g1_deploy

Conversation

@lemon-star608

@lemon-star608 lemon-star608 commented Apr 15, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

  • 为 G1 joystick(PPO 和 SAC)引入与 Go2 一致的 per-joint kp/kd 域随机化架构,取代此前通过 backend_overrides 的临时方案
  • 在 joystick.py 中新增 G1DomainRandConfig,继承 DomainRandConfig 并扩展 randomize_kp/kp_multiplier_range、randomize_kd/kd_multiplier_range 四个字段(默认启用,范围 [0.9, 1.1])
  • 在 MuJoCoBackend 上新增 get_actuator_gains() 方法,从当前 MuJoCo 模型状态读取 per-joint kp/kd 数组,使随机化基准值与模型 XML 保持一致,而非依赖标量配置字段
  • 重构 dr_utils.build_common_reset_randomization():优先通过 backend.get_actuator_gains() 获取基准 gains,回退到 control_config.Kp/Kd 标量;随机化结果从 (num_reset, 1) 广播改为直接矩阵乘法,保持 per-joint 精度
  • 在 Motrix 配置(g1_joystick/motrix.yaml、g1_sac/motrix.yaml)中通过 env.domain_rand.randomize_kp/kd: false 禁用 kp/kd 随机化,无需 backend_overrides workaround
  • G1JoystickSACCfg.domain_rand 同步切换为 G1DomainRandConfig,SAC 和 PPO 共用同一套配置结构

Linked Work

Validation

  • make check
  • uv run pytest -m "not slow"
  • Additional task-specific validation listed below

Commands actually run:

# paste exact commands here

Impact

  • Backend impact: both
  • Platform impact: both
  • Training effect expected: yes(MuJoCo backend 下 kp/kd 域随机化现在生效;Motrix backend 行为不变,通过配置显式禁用)

Checklist

  • Added or updated tests where needed
  • Updated docs if behavior or workflow changed
  • Linked the driving issue
  • Noted any follow-up work explicitly

@TATP-233

Copy link
Copy Markdown
Collaborator

I don't think this change can merge in its current form because it breaks the backend capability contract instead of modeling it explicitly.

docs/zh_CN/00-development-architecture.md already requires backend differences to stay inside backend/env adapter layers and says capability gaps must be explicit (Backend isolation, lines 44-48). ADR-0001 also treats SimBackend as the stable backend abstraction boundary, and ADR-0002 is even more explicit that backend differences must be modeled as capability contracts rather than scattered ad-hoc checks.

With that standard, the current shape has two problems:

  1. MuJoCoBackend gets a new get_actuator_gains() method, but SimBackend does not. That means the new behavior is not part of the formal backend contract.
  2. src/unilab/dr/dr_utils.py then probes env._backend via getattr(...)/hasattr(...) to decide whether to use that path. This is exactly the kind of implicit backend branching the architecture docs try to prevent.

The behavioral consequence is worse than style: the same env/domain-rand config now takes different reset paths on different backends without an explicit capability declaration or an explicit failure. In this repo, backend behavior is allowed to differ only when the difference is modeled as a real capability boundary and unsupported paths fail explicitly, rather than silently falling back.

I think this needs to be reworked in one of these directions:

  • Add an explicit actuator-gain contract/capability at the SimBackend layer, and make unsupported backends fail during validation with NotImplementedError instead of silently taking another path.
  • Or keep the kp/kd randomization logic fully inside the backend-specific adapter/config layer, without dr_utils reaching into backend-private methods.

Separately, please do not solve this later by parsing XML/assets inside reset/domain-randomization hot paths. Even if we need model-derived actuator metadata, asset/XML access belongs in cold/init-time setup, not per-reset logic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants