Skip to content

draft: refactor spawn - #516

Open
MahooX wants to merge 11 commits into
feature/newton-physics-backendfrom
xiemh
Open

draft: refactor spawn#516
MahooX wants to merge 11 commits into
feature/newton-physics-backendfrom
xiemh

Conversation

@MahooX

@MahooXMahooX commented Aug 18, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Replace the legacy scene-construction paths with DexSim Spawn.
  • Keep the existing EmbodiChain add_* APIs and stable object facades through a declaration → prepare → bind lifecycle.
  • Unify Default/PhysX and Newton preparation through sim.prepare(), including rigid, articulation, robot, soft-body, cloth, and rigid-object-group integration.
  • Preserve native DexSim handling for lights and CameraGroup-based sensors, and update simulation tutorials for the new lifecycle.
  • Keep singleton USD asset loading compatible through Spawn descriptors.

Validation

  • Rebuilt and installed the matching DexSim package.
  • Validated focused simulation tutorials and multi-environment flows on both Default/PhysX and Newton backends.
  • Ran targeted syntax, formatting, and integration checks.

Type of change

  • Enhancement

Dependency

Requires the matching DexSim Spawn implementation.

@MahooX
MahooX requested a review from yuecidengAugust 18, 2026 04:03
@greptile-apps

greptile-appsBot commented Aug 18, 2026

Copy link
Copy Markdown

Greptile Summary

The PR replaces legacy scene construction with a Spawn-backed declaration, preparation, and facade-binding lifecycle while preserving the existing simulation APIs.

  • Adds Spawn descriptors and scene coordination for physical resources.
  • Unifies Default and Newton runtime preparation through SimulationManager.prepare().
  • Moves environment robot and sensor setup across the new preparation boundary.
  • Updates object backends, examples, tutorials, documentation, and focused lifecycle tests.

Confidence Score: 2/5

The PR does not appear safe to merge while Spawn attribute operations, articulation physics overrides, and failed-binding retries remain broken.

Spawn-bound friction access still reaches methods that raise, articulation preparation still drops configured physics values, and a binding exception can leave a facade partially initialized and skipped on retry.

Files Needing Attention: embodichain/lab/sim/objects/backends/spawn.py, embodichain/lab/sim/objects/rigid_object.py, embodichain/lab/sim/objects/articulation.py, embodichain/lab/sim/spawn/scene.py, and embodichain/lab/sim/sim_manager.py

Important Files Changed

FilenameOverview
embodichain/lab/sim/sim_manager.pyIntroduces the unified Spawn preparation boundary, but failed facade binding can still leave a non-retryable partial facade.
embodichain/lab/sim/spawn/scene.pyCoordinates declaration, commit, and facade binding; retry behavior still depends on mutable facade declaration state.
embodichain/lab/sim/objects/backends/spawn.pyAdds Spawn-backed ready views, while exposed rigid-body friction operations remain unsupported.
embodichain/lab/sim/objects/articulation.pyAdapts articulation binding and post-Spawn configuration, but configured physics overrides remain incompletely applied.
embodichain/lab/gym/envs/base_env.pySplits scene declaration from post-prepare robot and sensor setup to consume runtime metadata only after materialization.
embodichain/lab/sim/spawn/descriptors.pyIntroduces Spawn descriptor conversion for EmbodiChain physical object configurations.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart LR
A[Declare physical scene] --> B[Spawn descriptors]
B --> C[SimulationManager.prepare]
C --> D[Finalize Spawn scene]
D --> E[Prepare backend runtime]
E --> F[Bind EmbodiChain facades]
F --> G[Configure robot and sensors]
G --> H[Reset and step environment]
Loading

Reviews (13): Last reviewed commit: "modify default cfg" | Re-trigger Greptile

Comment threadembodichain/lab/sim/objects/backends/spawn.py
Comment threadembodichain/lab/sim/objects/articulation.py Outdated
Comment threadembodichain/lab/sim/sim_manager.py Outdated
@yuecideng

yuecideng commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

Review conclusion

The overall direction is sound: moving EmbodiChain to a single Spawn-owned declaration → prepare → bind lifecycle is the right architectural change. However, this PR is not ready to merge yet. The following items are EmbodiChain-side blockers.

Merge blockers

  1. Preparation and binding failures are not retryable

    • SimulationManager.prepare() calls prepare_runtime() and scene.bind() only when commit/rebuild is required.
    • If runtime preparation or a bind callback fails after the scene has been committed, a later prepare() can skip both operations and report success while a facade remains unusable.
    • SpawnScene should track pending/bound declarations explicitly. A minimal convergent lifecycle is: commit_if_dirty(), always call the idempotent prepare_runtime(), then bind_pending().
  2. Articulation binding can leave a half-bound facade

    • Articulation.bind_spawn() reinitializes self in place before _apply_spawn_config() completes.
    • If configuration fails, _spawn_result is already set, is_declared becomes false, and the callback guard prevents a retry.
    • Match the existing rigid-object pattern: construct and configure a temporary bound instance, then atomically replace the public facade only after success. Longer term, physics configuration should happen before materialization rather than during bind.
  3. Spawn-bound rigid-body property APIs regress existing behavior

    • SpawnRigidBodyView.is_ready is true, but friction, restitution, and contact-offset methods unconditionally raise NotImplementedError.
    • RigidObject.set_attrs() rejects every Spawn-bound call, and damping getters/setters also reject Spawn-bound objects.
    • This breaks existing tests and real consumers such as the physical-property observation functor, which calls both get_friction() and get_damping().
    • Use the DexSim property capability matrix for per-field dispatch: apply runtime-supported fields, retain rebuild-backed fields in descriptors, and reject only the fields that are truly unsupported.
  4. Articulation physics configuration is silently incomplete

    • _apply_spawn_config() applies joint drives and some link masses, but discards non-mass base/link attributes such as friction, restitution, damping, contact settings, CCD, velocity limits, and solver behavior.
    • Newton link-mass overrides are skipped, and Newton qpos_limits are rejected by the declaration path.
    • Prefer typed link/joint override rules on ArticulationDesc, applied by DexSim after source metadata is resolved and before native materialization. This would remove most post-bind configuration and avoid an immediate Newton rebuild.
  5. The new explicit prepare contract has not been migrated across the test suite

    • Initial SimulationManager.add_* calls now return declared, metadata-light facades until prepare().
    • Existing CPU rigid-object/group tests do not call prepare(), and a static scan finds at least twenty test modules that construct a manager, add a robot/object, and immediately consume metadata without preparing.
    • Either declare this as a breaking API contract and migrate all callers, or provide a clearly scoped compatibility boundary. Avoid implicit auto-prepare on random facade getters because that would make batching unpredictable.
  6. Kinematic rigid objects are currently unsafe on PhysX Direct GPU

    • EmbodiChain creates a RigidBodyBatch for every non-static object, including kinematic objects.
    • DexSim currently only warns that kinematic Direct GPU batch operations may silently do nothing.
    • Until the engine path is fixed, EmbodiChain should fail fast during binding instead of exposing a ready facade with unreliable state operations.
  7. The declared DexSim dependency is incompatible with this PR

    • pyproject.toml still pins dexsim_engine==0.4.3.
    • The existing DexSim v0.4.3 release predates the Spawn APIs used here, including the new articulation/result lifecycle.
    • Merge this only after a new DexSim wheel is published, then update the exact pin and add a cross-repository installation smoke test.

Design simplifications

  • Push row/DOF/link selection into DexSim batches. The current _SpawnSelectionAdapter performs full-batch fetch/modify/write for every partial update, which is expensive for vectorized RL. Zero-filled force/qf writes can also overwrite values authored by another partial call in the same step.
  • Keep EmbodiChain adapters limited to public tensor layout and quaternion conversion; backend selection, capability handling, status checking, and topology rebinding should remain in DexSim.
  • Remove raw native access from high-level facades. For example, RigidObjectGroup.set_collision_filter() currently reaches through get_physical_body() instead of using a typed Spawn API.
  • DexSim now supports RenderDesc.load_option, while spawn/descriptors.py still warns and drops MeshCfg.load_option; pass it through and remove that compatibility branch.
  • Narrow or split this 72-file PR. A safer stack is: lifecycle infrastructure, rigid-body integration and parity tests, articulation/robot integration and parity tests, then soft/cloth/sensor migration.

Required validation before merge

Please add coverage for failed-then-successful prepare/bind, direct-manager CPU usage, runtime add/remove, dynamic and kinematic rigid bodies, full and partial batch writes, rigid property round trips, articulation config overrides, fixed/floating roots, and state retention across rebuilds. The matrix should include PhysX CPU, PhysX Direct GPU, Newton semi-implicit, and Newton MuJoCo-Warp.

The current PR has only the Greptile check, no functional CI or human approval, and it deletes the previous Newton lifecycle test without replacing it with tests for the new lifecycle.

@greptile-apps

Copy link
Copy Markdown

Want your agent to iterate on Greptile's feedback? Start a greploop in Codex and it will work through the open comments and keep going until this PR reviews clean.

Sign up for freeto 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

@MahooX@yuecideng