google-deepmind / mujoco

Multi-Joint dynamics with Contact. A general purpose physics simulator.
https://mujoco.org
Apache License 2.0
8.25k stars 823 forks source link

`island`-flag affects physics despite documentation stating otherwise #2099

Closed hartikainen closed 1 month ago

hartikainen commented 1 month ago

Toggling the island-flag affects the simulation behavior, despite the documentation explicitly stating that the islands should have no effect on the physics:

The flag currently has no effect on the physics pipeline, but enabling it allows for island visualization.

Originally posted by @hartikainen in https://github.com/google-deepmind/mujoco/discussions/2098#discussioncomment-10774316

Here's a video of what happens:

https://github.com/user-attachments/assets/9c62987e-fb8a-45a6-aea8-e5623d601bd3

I think this is a bug: either the documentation is wrong/outdated or there's something unexpected happening as a result of the island flag.

yuvaltassa commented 1 month ago

The documentation is wrong, but in a very very subtle way. I'll shortly clarify a bit more in the docs but let me expand here first.

In one sense this is true, the constraint solver (CG, until we add island support to Newton and PGS -- coming soon!) is solving exactly the same convex optimization problem, so in that sense yes, "the physics is the same".

However, "the same physics " is only true if the optimization problem is actually solved. If you do reach the minimum, the minimum is the same whether islands are on or off. However if you terminate early by reducing iterations, you don't actually reach the minimum and the solution will be different if islands are on or off. This is particularly egregious with CG, which—until the minimum is reached—will mix accelerations from unrelated dofs. By turning islands on, you are preventing this mixing since the separate per-island CG solvers can't interfere with each other. In this sense islands should always be turned on for CG.

I'd love to tell you to do that if you use CG you should always turn them on, but unfortunately I can't, since we need to refactor the island code. They way it currently works is not memory-local and adds some indirection, so even though it's supposed to be a strict improvement is sometimes isn't. (tl;dr we need to sort the constraints and the dofs so that each island is contiguous in memory).

I recommend turning on simulate's profiler (F3), to see this in action.

In terms of upcoming featureset for MuJoCo C, the order is roughly:

  1. Fix the over-allocation in the Newton solver to allow it to handle very large models.
  2. Refactor islands to be memory-local.
  3. Add island support to the Newton solver.
  4. Add island support to the PGS solver.
  5. Allow multithreading over islands.
  6. Allow sleeping islands.

This work package was delayed by about a year but is now finally making progress, you'll be happy to hear @thowell is very close to completing 1, which is a longstanding thorn in our side :slightly_smiling_face:

cc @bd-mdelasa

hartikainen commented 1 month ago

Thanks for the super fast and detailed answer. Guaranteed Yuval quality hehe. Much appreciated!

bd-mdelasa commented 1 month ago

@yuvaltassa thanks for the detailed answer. This info and the breakdown of next steps are very useful.