Skip to content

feat(sim): auto-disable mujoco shadows when rendering is slow - #4232

Open
spomichter wants to merge 3 commits into
mainfrom
sim-auto-disable-shadows
Open

spomichter wants to merge 3 commits into
mainfrom
sim-auto-disable-shadows

Conversation

@spomichter

Copy link
Copy Markdown
Contributor

@bogwi Dan — heads up, this is a slop PR (Claude-authored from a debugging session on my Mac) — shared in case it's helpful. Take what's useful or close it.

Problem

dimos --simulation run unitree-go2 runs at ~0.4x realtime on Apple Silicon. Profiling showed physics is fine (0.8 ms/step, 6.3x realtime headroom with the office scene) — the cost is shadow-mapping the ~1220-mesh scene in the offscreen camera renders: 28.6 ms with shadows vs ~7 ms without per 640x360 frame, out of a 50 ms video frame budget.

Changes

  • mujoco_shadows: Literal["auto", "on", "off"] = "auto" in GlobalConfig. auto benchmarks one shadowed offscreen render at sim startup (~150 ms) and zeroes model.vis.quality.shadowsize when it exceeds 30% of the video frame budget. This must happen before launch_passive/Renderer create their GL contexts — the shadow framebuffer is allocated at context creation, so toggling mjRND_SHADOW scene flags at runtime leaves the interactive viewer shadow-rendering the whole scene regardless (tried that first; didn't help).
  • Skip CoreMLExecutionProvider for the locomotion policy ONNX session: CoreML partitions the 14-node graph (13/14 nodes) and pays a per-call copy overhead making it ~20x slower than plain CPU inference (0.335 ms vs 0.014 ms).

Machines where the shadowed render is cheap (discrete GPUs measure ~2-5 ms) keep shadows; nothing changes for them.

Measured (M-series Mac, go2 + office1 scene)

before after
worst-case loop iteration 92 ms 28 ms
realtime factor 0.38x 1.25x

Verified end-to-end: startup logs Shadowed render took 28.6 ms of the 50 ms frame budget; disabling shadows and the sim runs at realtime.

🤖 Generated with Claude Code

@codecov

codecov Bot commented Sep 19, 2026

Copy link
Copy Markdown

❌ 1 Tests Failed:

Tests completed Failed Passed Skipped
6086 1 6085 103
View the full list of 1 ❄️ flaky test(s)
dimos.e2e_tests.test_voice_browser::test_hold_to_talk_ships_a_decodable_recording[firefox]

Flake rate in main: 68.75% (Passed 5 times, Failed 11 times)

Stack Traces | 22.7s run time
voice_bridge = ('http://127.0.0.1:62113/', [])
fake_mic_page = <Page url='http://127.0.0.1:62113/'>

    def test_hold_to_talk_ships_a_decodable_recording(
        voice_bridge: tuple[str, list[AudioChunk]], fake_mic_page: Page
    ) -> None:
        url, chunks = voice_bridge
        chunks.clear()  # the module fixture is shared across both engines
        fake_mic_page.goto(url)
        mic = fake_mic_page.get_by_test_id("chat-audio_in-mic")
        # Enabled == transport connected; the manifest already placed the panel.
        expect(mic).to_be_enabled(timeout=120_000)
        expect(mic).to_have_attribute("data-state", "idle")
    
        mic.hover()
        fake_mic_page.mouse.down()
>       expect(mic).to_have_attribute("data-state", "recording", timeout=15_000)
E       AssertionError: Locator expected to have attribute 'recording'
E       Actual value: arming 
E       Call log:
E         - Expect "to_have_attribute" with timeout 15000ms
E         - waiting for get_by_test_id("chat-audio_in-mic")
E       
E       Aria snapshot:
E       - button "hold to talk": talk

chunks     = []
fake_mic_page = <Page url='http://127.0.0.1:62113/'>
mic        = <Locator frame=<Frame name= url='http://127.0.0.1:62113/'> selector='internal:testid=[data-testid="chat-audio_in-mic"s]'>
url        = 'http://127.0.0.1:62113/'
voice_bridge = ('http://127.0.0.1:62113/', [])

dimos/e2e_tests/test_voice_browser.py:104: AssertionError

To view more test analytics, go to the Test Analytics Dashboard
📋 Got 3 mins? Take this short survey to help us improve Test Analytics.

@spomichter
spomichter force-pushed the sim-auto-disable-shadows branch from 337eea0 to 6d25d29 Compare September 19, 2026 09:01
Comment thread dimos/simulation/mujoco/mujoco_process.py Outdated
spomichter and others added 2 commits September 20, 2026 02:32
Shadow-mapping the office scene costs ~4x per offscreen render on
integrated GPUs (28.6ms vs 7ms per 640x360 frame on Apple Silicon),
pinning the whole sim at ~0.4x realtime. Benchmark one shadowed render
at startup and zero shadowsize before the viewer/renderer GL contexts
are created when it exceeds 30% of the video frame budget.

Also skip CoreMLExecutionProvider for the locomotion policy: it
partitions the 14-node graph and is ~20x slower than plain CPU
inference (0.335ms vs 0.014ms per call).
@bogwi
bogwi force-pushed the sim-auto-disable-shadows branch from 6d25d29 to a25ae8f Compare September 19, 2026 17:33
@bogwi
bogwi marked this pull request as ready for review September 19, 2026 17:33
@bogwi bogwi added the backport:skip Skip creating a backport to any release branches label Sep 19, 2026
@greptile-apps

greptile-apps Bot commented Sep 19, 2026

Copy link
Copy Markdown
Contributor

RetriggerConfidence Score: 4/5

The remaining findings are non-blocking documentation and regression-coverage improvements; they do not make the implementation unsafe to merge.

Findings

  1. P2 Document shadow setting
  2. P2 Test shadow selection

Summary

This change adds adaptive MuJoCo shadow control, simulation pacing updates, and CoreML avoidance for locomotion policies. Two non-blocking follow-ups remain: document the new public shadow override and add focused regression tests for its selection behavior.

Reviews (1) · Last reviewed commit: "fix control simulation speed"

Comment thread dimos/core/global_config.py
Comment thread dimos/simulation/mujoco/mujoco_process.py
@dimensionalOS dimensionalOS deleted a comment from greptile-apps Bot Sep 19, 2026
@github-actions github-actions Bot added the ready-to-merge Required CI checks have passed on this PR label Sep 19, 2026
Comment on lines +267 to +268
frame_sim_time = model.opt.timestep * config.mujoco_steps_per_frame
time_until_next_step = frame_sim_time - (time.time() - step_start)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change makes it so the speed is no longer faster than real time, making the robot move quite slowly.

@bogwi bogwi Sep 20, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Entirely disagree.

What the blueprint commands:

  1. unitree-go2 (smart/unitree_go2.py:42-50) uses ReplanningAStarPlanner. Its local planner sets _speed = 0.55 m/s (replanning_a_star/local_planner.py:67), the PController scales that down with heading error and pins it at _min_linear_velocity = 0.2 (controllers.py:43, :73-74).

  2. So the planner commands at most 0.55 m/s forward.

  3. MujocoConnection passes twist unchanged into shared memory (mujoco_connection.py:412-414), MockController copies it as [forward, lateral, yaw] (mujoco_process.py:56-64), and the Go1 policy feeds it straight into its observation (policy.py:97).

At 1x realtime, 0.55 m/s commanded is 0.55 m/s OBSERVED, which is what the real go2 does under the same blueprint.

That is the speed you call slow. But this the speed the blueprint commands.


Having "faster than realtime" is a defect, not a feature

Why should it run on my mac m4pro x N times faster when the shadows are disabled?


I have just tested on 3 laptops

After the change:
same identical speed, 0.55 m/s commanded and 0.55 m/s observed when walking and 1m/s when Shift is pressed for Apple m1pro, m4pro, and rtx 5070, meaning the changed ARE valid!

Before the change:
m1pro: runs observed x3 faster than 0.55m/s for unitree-go2 blueprint
m4pro12: runs observed x4 faster than 0.55 m/s for unitree-go2 blueprint
rxt 5070: runs orbserved x N faster than 0.55m/s for unitree-go2 blueprint -> IMPOSSIBLE TO CONTROL THE ROBOT AT ALL.

So, switching off the shadows on mac, surfaced the bug already present on main. Running with shadows on mac would make it seemingly right so it went unnoticed and linux consumers did not care look at it.

@paul-nechifor paul-nechifor left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The speed of the robot is too low now.

@bogwi

bogwi commented Sep 20, 2026

Copy link
Copy Markdown
Member

The speed of the robot is too low now.

It is not, run this on your machine and you will see the defect in action,

23407d7 -> without my changes, proves the deffect

git checkout 23407d7c96a023a11b806d4f5ecb82b97c913ec4 && uv run dimos --simulation run unitree-go2

Same defect is observable on main too, - the commanded velocity does not match observed velocity.


Everything outside the physics runs on wall time: odometry is stamped with time.time(), the planner controls at 10 Hz wall(local_planner.py:68)), lidar at 2 Hz wall, video at 20 Hz wall (mujoco/constants.py:32-33)

At N x realtime the robot travels N times farther between control updates and lidar scans, so lookahead, controller gains, min velocities and obstacle avoidance all behave differently from hardware.

The sim stops being a simulator for the robot. The benchmark results are no longer valid if the sim robot reaches the targer N x faster in before the changes I did.

@bogwi

bogwi commented Sep 20, 2026

Copy link
Copy Markdown
Member

The speed of the robot is too low now.

If you want faster-than-realtime for iteration speed, that should be an explicit, deterministic knob (a realtime factor applied to frame_sim_time), not a side effect of GPU speed. That is a separate feature from this PR and by intend a) defeats the purpose of simulation and b) affects benchmarks we do by the factor of N, so entirely incorrect

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

Labels

backport:skip Skip creating a backport to any release branches PlzReview ready-to-merge Required CI checks have passed on this PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants