Conversation
acrlw
added
enhancement
|
I recommend moving the reusable runtime mechanisms in this PR into standard EmbodiChain components, while keeping task semantics and robot-specific configuration in the locomotion task package.
Contact handling should be the first boundary to address before merging. In The standard contact component should expose a consistent contract for counterpart filtering and contact history, including selective reset. It should also distinguish current air time from the completed air time captured on landing: the current task code clears air time before the ANYmal landing reward consumes it. Action components need explicit interfaces before being promoted. The task RNG should participate in explicit reseeding. Suggested order:
Validation should cover these ownership boundaries: foot self-contact must not count as ground contact; a foot contact occurring only during an early substep must remain observable; a landing reward must receive the completed air time; repeated explicit seeds must reproduce task random streams; and selective reset must preserve untouched rows. These recommendations are independent of the declared #635 prerequisite. They would address the underlying ownership problems rather than leave each task adapter responsible for maintaining its own contact and lifecycle implementation. |
|
Updated in this PR:
All 207 focused tests passed, including the five regression cases identified in the review. All seven tasks passed smoke tests on both backends, and the existing CartPole check passed. |
yuecideng
left a comment
yuecideng
left a comment
There was a problem hiding this comment.
I reviewed the shared environment and sensor abstractions introduced for locomotion. The separation is promising, but the inline comments below identify scalability, lifecycle, observability, and API-contract issues that should be addressed before these APIs are treated as reusable foundation components.
| force = torch.where(valid.unsqueeze(-1), force, 0.0) | ||
| matches = [] | ||
| for side in (0, 1): | ||
| selected = pair[:, :, side, None] == self.actor_ids[:, None, :] |
There was a problem hiding this comment.
[P1] Avoid dense env × contact × body reductions in the substep hot path
This broadcast materializes an (E, C, B) selection tensor, and the force path below materializes (E, C, B, 3) before reducing it. With the default G1 PPO setup (E=4096, contact capacity C=192) and only B=20 tracked bodies, the float intermediate alone is roughly 180 MiB per physics substep, before masks, conversions, and history buffers; locomotion executes this four times per control step. The current small-environment validation will not expose this training-scale cost. Please use a fused sparse Warp/CUDA reduction over the contacts that actually exist (for example, with precomputed actor-to-history indices and device-side counts), and validate at least one 4096-environment rollout plus PPO update.
| for sensor in sensors: | ||
| sensor.begin_control_step() | ||
| for _ in range(self.cfg.sim_steps_per_control): | ||
| self.sim.update(self.physics_dt, 1) |
There was a problem hiding this comment.
[P2] Preserve the SimulationManager.update control-step boundary
Replacing one sim.update(dt, decimation) call with decimation calls to sim.update(dt, 1) changes observable manager behavior, not just sensor timing. Each call now runs the manager's prepare/profiling boundary, and the Viser path treats every single-step call as the final step and can capture camera images at physics rate instead of control rate. Please keep one manager update per control step and expose an after-substep observer/callback inside SimulationManager.update for sensors that require substep sampling.
| self._component_generators: dict[str, torch.Generator] = {} | ||
| if name not in self._component_generators: | ||
| generator = torch.Generator(device=self.device) | ||
| generator.manual_seed(self._component_seed(name, self.cfg.seed or 0)) |
There was a problem hiding this comment.
[P2] Do not turn seed=None into a fixed deterministic seed
EnvCfg.seed=None currently means that the environment is not explicitly seeded, but self.cfg.seed or 0 gives every fresh environment the same component seed derived from zero. That silently changes randomization behavior for unseeded runs. Please call generator.seed() (or otherwise use nondeterministic entropy) when cfg.seed is None, and derive stable named seeds only when an explicit base seed is provided. A regression test should compare separately constructed unseeded environments as well as reproducibility under an explicit seed.
| self._data_buffer["is_valid"].zero_() | ||
|
|
||
| contact_buffer = self._query.fetch() | ||
| self._query_dropped_count = contact_buffer.dropped_count |
There was a problem hiding this comment.
[P2] Accumulate dropped contacts across the full control interval
This overwrites _query_dropped_count after every substep. If an early substep overflows and the final one does not, the public diagnostic reports zero even though contacts used by the interval-level history/reward logic were lost. Clear an interval counter in begin_control_step() and accumulate every fetch result instead. Also record scatter overflow with a separate device-side counter: _num_contacts_per_env is capped/decremented when the per-environment buffer is full, so deriving overflow from that value cannot reliably reveal discarded contacts.
| @property | ||
| def requires_substep_update(self) -> bool: | ||
| """Whether this sensor participates in the physics-substep lifecycle.""" | ||
| return self.cfg.track_substeps and bool(self._histories) |
There was a problem hiding this comment.
[P2] Do not let a successfully created history remain silently inert
create_history() succeeds regardless of track_substeps, but this predicate disables all lifecycle updates when the option retains its default value (False). Callers then receive a valid-looking history whose data stays zero, despite the history API promising that the sensor lifecycle updates it. Either make the existence of a history enable substep sampling automatically, or reject create_history() with a clear configuration error when sampling is disabled. Please cover the default configuration in a regression test.
| track_substeps: bool = False | ||
| """Sample registered contact histories after every physics substep.""" | ||
|
|
||
| self_collision_force_threshold: float | None = None |
There was a problem hiding this comment.
[P3] Keep the locomotion reward threshold out of the generic sensor config
The sensor itself does not consume this field; only the locomotion setup reads it and passes it to create_history(). Putting a reward/policy threshold on ContactSensorCfg makes reusable sensor and embodiment configuration task-specific and creates two owners for history semantics. Please move this value to the locomotion task/reward configuration and pass it explicitly as force_threshold when constructing the self-contact history.
2 participants
Description
Add six velocity-control tasks and Humanoid Run to the official task package, with downloadable robot assets and default/Newton environment and PPO configurations. This makes the local locomotion implementations available through EmbodiChain's standard task discovery, asset loading and RL entry points.
Merge prerequisite: #635 must be merged first. Keep this PR in draft until the target simulator dependency version is confirmed and asset loading, environment and policy checks pass on the updated main branch.
Tasks and assets
UnitreeG1FlatRL-v1UnitreeG1LocomotionUnitreeH1_2FlatRL-v1UnitreeH1_2LocomotionUnitreeGo1FlatRL-v1UnitreeGo1LocomotionUnitreeGo2FlatRL-v1UnitreeGo2LocomotionANYmalCFlatRL-v1ANYmalCLocomotionMicroDuckFlatRL-v1MicroDuckLocomotionHumanoidRun-v1HumanoidRunThe seven ZIPs are published in DexForceAI/embodichain_data. Add their descriptors in
embodichain/data/assets/locomotion_assets.pyand register them with the download CLI. Task configurations resolve models throughget_data_path(). The CLI now returns a nonzero exit status if any requested download fails.Register the tasks and manager functions, with separate actor/critic observations, robot-specific control settings and PPO configurations.
Contact collection
Reading contacts only after the final physics substep can miss brief contacts needed by rewards and termination checks. Add an
_advance_physics()hook for task-level collection after each substep, cache actor metadata until its tables change, and expose dropped-contact counts.Validation
Local validation uses
main@6a419136with #635 atc502be19:main@3224ac1e, seven task registrations, 41 asset/documentation checks, Black and API documentation checks passed.Runtime and assets
The checks above used a locally modified simulator build. MicroDuck currently requires a short cache path; its model files carry the upstream BY-SA-NC designation. Source and license notices are included in each asset archive.
Type of change
Checklist
python docs/scripts/check_api_docs.py.