Glowing led decks in sim. by yufei4hua · Pull Request #45 · learnsyslab/crazyflow

yufei4hua · 2025-11-19T19:59:24Z

Add led deck geom and corresponding materials to cf21B_500.xml
Implement change_material method under sim/visualize.py to change certain materials' rgba & emission values.
Input sim, specify mat_name, drone_ids, rgba, and emission. Effects are long term valid.
Add example script for rendering led decks, with top/bottom led activated for drones with even/odd ids. BUT requires drone_model package.
p.s. User can dim the scene by specifying their own scene.xml when creating the Sim.

amacati

This seems to be a cool functionality, but there are several things that need to change.

For every new addition to the sim that is relatively confined, let's also try to add tests to make sure it works and doesn't break with future iterations.

For these kind of tests, please also check if it does not succeed for problematic inputs. See e.g. https://docs.pytest.org/en/7.1.x/how-to/assert.html#assertions-about-expected-exceptions

amacati · 2025-11-19T23:56:30Z

+        raise ValueError(f"drone_ids must be in range [0, {sim.n_drones - 1}], got {drone_ids}")
+
+    if rgba is not None:
+        rgba = np.asarray(rgba, dtype=float)


The function signature already sais we require a numpy array. Or is the cast because we need dtype to be float64?

amacati · 2025-11-19T23:58:41Z

+        try:
+            # this returns itself if rgba is already the right shape
+            rgba = np.broadcast_to(rgba, (len(drone_ids), 4)).copy()
+        except Exception:
+            raise ValueError(f"rgba must be shape (4,) or ({len(drone_ids)}, 4), got {rgba.shape}")


Why the try block? If this fails, just let it fail. Also, try to avoid bare Exceptions at all cost. They can be extremely confusing for users. Anything could go wrong, but they wouldn't see any of it because the bare except just swallows the error.

Removed.
The np.broadcast_to() will report:
"ValueError: operands could not be broadcast together with remapped shapes [original->remapped]: (3,4) and requested shape (13,4)"
if shape is incorrect, which is also clear.

amacati · 2025-11-19T23:59:21Z

+            rgba = np.broadcast_to(rgba, (len(drone_ids), 4)).copy()
+        except Exception:
+            raise ValueError(f"rgba must be shape (4,) or ({len(drone_ids)}, 4), got {rgba.shape}")
+        rgba = np.clip(rgba, 0.0, 1.0)


Do we need the clip? Can't mujoco deal with [2 0 0 0]?

No, we don't. Removed.

Btw, I found that mujoco can also handle emission<0, so I removed "emission = np.maximum(emission, 0.0)" as well.

amacati · 2025-11-19T23:59:32Z

+        rgba = np.clip(rgba, 0.0, 1.0)
+
+    if emission is not None:
+        emission = np.asarray(emission, dtype=float)


Same thing. Do we need the cast?

amacati · 2025-11-20T00:00:24Z

+
+    if emission is not None:
+        emission = np.asarray(emission, dtype=float)
+        try:


Just fail fast, it's okay to error in the function.

amacati · 2025-11-20T00:02:40Z

+
+    mj_model = sim.mj_model
+
+    for i, id in enumerate(drone_ids):


Do not use id, it's a built-in function that you are shadowing (https://docs.python.org/3/library/functions.html#id).

Changed to drone_id

amacati · 2025-11-20T00:04:38Z

+        if rgba is not None:
+            mj_model.mat_rgba[mat_id, :] = rgba[i]
+
+        if emission is not None:
+            mj_model.mat_emission[mat_id] = emission[i]


Can we broadcast this? I.e. create an index of the drones and then set mj_model.mat_rgba[mat_id, drone_ids] = rgba?

Yes, we can.
However, we still need to loop over the drone_ids to find the corresponding mat_id.
I changed it to mj_model.mat_rgba[mat_ids, :] = rgba

amacati · 2025-11-20T00:05:26Z

+        if emission is not None:
+            mj_model.mat_emission[mat_id] = emission[i]
+
+


Please add tests for this function! Even if we are not rendering in the tests, we should at least try to see if this errors as expected and succeeds for the correct input.

I added 2 small tests under test_sim.py
One mainly to make sure all the drone models have "led_top" and "led_bot" in xml file.
One to test the error output of the function.

amacati · 2025-11-20T00:07:29Z

+    n_worlds, n_drones = 1, 25
+    sim = Sim(
+        n_worlds=n_worlds,
+        n_drones=n_drones,
+        drone_model="cf21B_500",
+        control=Control.state,
+        physics=Physics.so_rpy,
+        device="cpu",
+    )


The basic scripts made these kwargs explicit to show users the options we provide with the sim. In specialized examples such as this one, we can skip this.

Suggested change

n_worlds, n_drones = 1, 25

sim = Sim(

n_worlds=n_worlds,

n_drones=n_drones,

drone_model="cf21B_500",

control=Control.state,

physics=Physics.so_rpy,

device="cpu",

)

sim = Sim(n_drones=25, drone_model="cf21B_500")

Simplified.

amacati · 2025-11-20T00:09:05Z

+    rgba: NDArray | None = None,
+    emission: NDArray | None = None,
+):
+    """Change the material of all drones matching the mask.


Not a mask, but the drone_ids, right?

Yes, corrected.

mschuck-rai · 2025-11-20T09:44:10Z


 def main():
    """Spawn 25 drones in one world and activate led decks."""
-    sim = Sim(n_drones=25, drone_model="cf21B_500", control=Control.state)


We can use the new ones, they look great. When I commented on removing the defaults I was referring to repeating stuff that is already set by default. I.e. n_worlds is 1 by default, so there is no need to repeat it.

I understood. I was trying to align with other examples.
I reverted this in the latest commit.

Use new drones in example.

yufei4hua · 2025-11-20T10:29:17Z

Thanks a lot for your reviews! I learnt a lot from that.
Please review the tests I wrote for this method. I'm not sure if this is the way to do it.

amacati

Suggested some very minor changes, otherwise this looks good.

amacati · 2025-11-20T16:12:34Z

+        raise ValueError(f"drone_ids must be in range [0, {sim.n_drones - 1}], got {drone_ids}")
+
+    if rgba is not None:
+        # this returns itself if rgba is already the right shape


Suggested change

# this returns itself if rgba is already the right shape

amacati · 2025-11-20T16:13:12Z

+        mat_ids.append(mat_id)
+
+    if rgba is not None:
+        mj_model.mat_rgba[mat_ids, :] = rgba


Is [mat_ids, :] necessary?

Suggested change

mj_model.mat_rgba[mat_ids, :] = rgba

mj_model.mat_rgba[mat_ids] = rgba

amacati · 2025-11-20T16:16:11Z

+            sim, mat_name="bad_mat", drone_ids=drone_ids, rgba=rgba, emission=emission
+        )
+
+    with pytest.raises(ValueError, match=r"drone_ids must be 1D array"):


I don't think you need the r-string. But please check.

Suggested change

with pytest.raises(ValueError, match=r"drone_ids must be 1D array"):

with pytest.raises(ValueError, match="drone_ids must be 1D array"):

amacati · 2025-11-20T16:16:43Z

+            emission=emission,
+        )
+
+    with pytest.raises(ValueError, match=r"drone_ids must be in range \[0, 1\]"):


Same here, but again, please check.

Suggested change

with pytest.raises(ValueError, match=r"drone_ids must be in range \[0, 1\]"):

with pytest.raises(ValueError, match="drone_ids must be in range [0, 1]"):

I looked up, so "r" s needed when "[]" appears in a regular expression.
I'll leave it for the second check.

amacati · 2025-11-20T16:18:48Z

Thanks a lot for your reviews! I learnt a lot from that. Please review the tests I wrote for this method. I'm not sure if this is the way to do it.

Yes, that was what I had in mind. Thanks for adding those.

yufei4hua · 2025-11-21T08:56:18Z

I applied the minor suggestions. Sorry for the delay.

amacati

LGTM, thanks!

yufei4hua added 3 commits November 19, 2025 19:50

Add change_material function in visualize utils.

9a52470

Fix linting.

29398f4

Add example for rendering led decks.

f1df346

amacati requested changes Nov 20, 2025

View reviewed changes

yufei4hua added 2 commits November 20, 2025 09:21

Simplify code.

d5ca60d

Use lagecy drones in example.

33c844c

mschuck-rai reviewed Nov 20, 2025

View reviewed changes

Add test cases for change_material.

b3e3d5e

Use new drones in example.

Clean up test error function.

bb59cff

amacati requested changes Nov 20, 2025

View reviewed changes

amacati self-assigned this Nov 20, 2025

amacati added the enhancement New feature or request label Nov 20, 2025

Final cleaning up.

c0c4c96

amacati approved these changes Nov 21, 2025

View reviewed changes

amacati merged commit 057cd6c into learnsyslab:dev Nov 21, 2025
2 checks passed

		if emission is not None:
		mj_model.mat_emission[mat_id] = emission[i]

	mj_model.mat_rgba[mat_ids, :] = rgba
	mj_model.mat_rgba[mat_ids] = rgba

	with pytest.raises(ValueError, match=r"drone_ids must be 1D array"):
	with pytest.raises(ValueError, match="drone_ids must be 1D array"):

Conversation

yufei4hua commented Nov 19, 2025

Uh oh!

amacati left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yufei4hua commented Nov 20, 2025

Uh oh!

amacati left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amacati commented Nov 20, 2025

Uh oh!

yufei4hua commented Nov 21, 2025

Uh oh!

amacati left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants