Skip to content

Add flex self-collisions#1262

Open
quagla wants to merge 17 commits intogoogle-deepmind:mainfrom
quagla:main
Open

Add flex self-collisions#1262
quagla wants to merge 17 commits intogoogle-deepmind:mainfrom
quagla:main

Conversation

@quagla
Copy link
Copy Markdown
Collaborator

@quagla quagla commented Mar 27, 2026

No description provided.

@quagla quagla requested a review from thowell March 27, 2026 17:13
<mujoco model="Poncho">
<include file="mannequin.xml"/>
<option solver="CG" tolerance="1e-6" jacobian="sparse"/>
<option integrator="Euler" solver="CG" viscosity="0.05" tolerance="1e-6"/>
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

do we need to include 'integrator="Euler"' since this is the default integrator option?

<mujoco model="Poncho">
<include file="mannequin.xml"/>
<option solver="CG" tolerance="1e-6" jacobian="sparse"/>
<option integrator="Euler" solver="CG" viscosity="0.05" tolerance="1e-6"/>
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

do we expect the viscosity option to be changed/removed in the future? if so, maybe we add a todo to update this parameter

np.testing.assert_equal(result, True, f"Contact {i} not found in Gjk results")

self.assertEqual(d.nacon.numpy()[0], mjd.ncon)
if check_version("mujoco>=3.5.1.dev888028695"):
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

i think these changes are also available in #1263

</worldbody>
</mujoco>
"""
mjm = mujoco.MjModel.from_xml_string(xml)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

lets utilize test_data.fixture


mujoco.mj_forward(mjm, mjd)

# Count C self-contacts.
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

maybe vectorized this code?

d = mjwarp.put_data(mjm, mjd)
mjwarp.kinematics(m, d)
mjwarp.collision(m, d)
wp.synchronize_device()
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

do we need this?

warp_self = 0

self.assertGreater(warp_self, 0, f"Warp should produce self-contacts (nacon={nacon})")
self.assertGreaterEqual(warp_self, c_self, f"Warp={warp_self} should be >= C={c_self} self-contacts")
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

should the number of contacts generated by mujoco and mujoco warp be the same? if not, maybe add a comment explaining why the counts are different

</worldbody>
</mujoco>
"""
mjm_c = mujoco.MjModel.from_xml_string(xml)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

lets utilize test_data.fixture

mjd_c = mujoco.MjData(mjm_c)
mujoco.mj_forward(mjm_c, mjd_c)
c_self = 0
for c in range(mjd_c.ncon):
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

maybe vectorize

d = mjwarp.put_data(mjm_c, mjd_clean)
mjwarp.kinematics(m, d)
mjwarp.collision(m, d)
wp.synchronize_device()
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

is this necessary?

else:
warp_self = 0

self.assertGreaterEqual(warp_self, c_self, f"Warp={warp_self} should be >= C={c_self} self-contacts")
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

is there a reason to expect that mujoco warp might generate more contacts than mujoco? if so, lets add a comment for the reason

self.assertGreaterEqual(warp_self, c_self, f"Warp={warp_self} should be >= C={c_self} self-contacts")

def test_unsupported_selfcollide_raises(self):
"""Verify unsupported selfcollide modes raise NotImplementedError."""
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

the docstring says 'modes', should we be testing additional configurations?

</mujoco>
"""
mjm = mujoco.MjModel.from_xml_string(xml)
mjd = mujoco.MjData(mjm)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

is this necessary?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

lets add a test with more than one world in order to test batched evaluation?

d02 = wp.dot(v0, v2)
d11 = wp.dot(v1, v1)
d12 = wp.dot(v1, v2)
inv = 1.0 / (d00 * d11 - d01 * d01 + 1e-30)
Copy link
Copy Markdown
Collaborator

@thowell thowell Mar 31, 2026

Choose a reason for hiding this comment

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

1e-30 is quite small, should we consider increasing this value a bit?

# ensure njmax_nnz can hold the actual data from mjd
if is_sparse(mjm) and mjd.nefc > 0 and mujoco.mj_isSparse(mjm):
actual_nnz = int(mjd.efc_J_rownnz[: mjd.nefc].sum())
njmax_nnz = max(njmax_nnz, actual_nnz)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

maybe we should raise an error if njmax_nnz < actual_nnz?

worldid, pairid = wp.tid()

for f in range(nflex):
if flex_selfcollide[f] == int(FlexSelfCollideType.NONE):
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

do we need the int cast?

# Self-collision: only for dim=2 elements with selfcollide enabled.
# flex_self_max_pairs is precomputed in put_model to avoid device→host
# copies during CUDA graph capture.
if m.flex_self_max_pairs > 0:
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

we can remove this check since flex_self_max_pairs is one of the launch dimensions

Sign up for free to 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