Skip to content

sim/mujoco: Simulation mixin split is cosmetic — extract _SimulationState or merge #118

@cagataycali

Description

@cagataycali

Problem

class Simulation(
    PhysicsMixin,        # ~1100 lines
    RenderingMixin,      #  ~700 lines
    RecordingMixin,      #  ~200 lines
    RandomizationMixin,  #  ~100 lines
    SimEngine,           # ABC
    AgentTool,           # strands base
):
    ...  # +1600 lines in simulation.py itself

The mixin split is cosmetic, not real. Every mixin reaches into
self._world, self._lock, self._mj, self._policy_threads,
self._renderer_tls, and multi-robot prefix state that lives on Simulation
itself.

Evidence:

  • PhysicsMixin calls self._require_no_running_policy(...) — which lives on
    Simulation, not PhysicsMixin. mypy flagged this as attr-defined until
    we silenced it.
  • RenderingMixin._apply_sim_action mutates self._world.sim_time,
    self._world.step_count, and syncs the viewer — physics+rendering concerns
    tangled in the "rendering" file.
  • simulation.py itself still hosts step, reset, destroy, add_robot,
    add_object, etc. The alleged "orchestrator" is carrying half the primitives.

Not fatal, but: the file layout doesn't describe the coupling graph. Any
future refactor to genuinely separate concerns will be painful because every
mixin tested the coupling implicitly.

Proposal

Extract a _SimulationState dataclass owning the real shared state:

@dataclass
class _SimulationState:
    world: SimWorld | None
    lock: threading.Lock
    mj: ModuleType             # cached mujoco import
    policy_threads: dict[str, Future]
    renderer_tls: threading.local
    multi_robot_prefix: dict[str, str]
    ...

Mixins take state: _SimulationState as a method arg (or become classmethods
over it). Makes coupling explicit at the type level, fixes mypy narrowing,
and makes it obvious which pieces are truly independent.

Alternative: be honest, merge the mixins back into a single Simulation class
with clear # --- physics --- section headers. The current setup pretends to
be modular and isn't.

Acceptance

  • Pick a path
  • If _SimulationState: incremental migration, mixin-by-mixin
  • If merge: single commit that preserves all tests
  • Either way: zero behavioural change, all existing tests pass
  • Delete the cross-mixin TYPE_CHECKING stubs for _require_world /
    _require_no_running_policy once the pattern is properly expressed

Size

L/XL. Don't start without a quick prototype (one mixin migrated) to confirm
mypy is actually happier.


Surfaced by a second-opinion review of PR #85.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    Status

    Done

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions