Musculoskeletal dog creation by vittorione94 · Pull Request #402

Musculoskeletal dog creation by vittorione94 · Pull Request #402 · google-deepmind/dm_control

Choose a reason for hiding this comment

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

Looking good.

The Euler-angle free joint is a deal breaker though. We have to undo this and fix the underlying issue.

Choose a reason for hiding this comment

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

Nice work! As far as I can tell the logic looks good but I added a bunch of small style comments. In general more comments and doc strings would be great as well.

Choose a reason for hiding this comment

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

There are two main things missing from this PR:

Add dog_v2_test where the model is instantiated and stepped. Look at https://github.com/google-deepmind/dm_control/blob/main/dm_control/locomotion/walkers/cmu_humanoid_test.py
The public API of the module needs to be clear. Are users expected to call add_markers.add_markers, etc.? The test will make it a bit clearer for me how to use the module, but also, you should choose the small number of functions that are to be used by the clients, and import them publically into __init__.py

Choose a reason for hiding this comment

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

@vittorione94

I tried to import this and encountered several issues:

A bunch of files are missing the standard copyright header. Make sure to make the year 2024 for new files.
The standard dm_control tests are failing because the existing dog doesn't load because you deleted/moved dog_assets/dog_skin.skn. Please make sure the existing tests pass.
Please make sure all code is wrapped to 80 columns.
Added some specific issues our linter found in specific lines.
Please sync to HEAD.

		arma = physics.bind(j).armature[0]
		# Print torque actuators stuff
		if not use_muscles:
		joint_acts = [model.find("actuator", j.name) for j in actuated_joints]

Choose a reason for hiding this comment

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

Variable 'actuated_joints' is possibly uninitialized

Choose a reason for hiding this comment

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

actuated_joints is definitely intialized if we're not using muscles. The build_dog script creates an actuated model (either with torque joints or tendons).

Choose a reason for hiding this comment

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

This is not my opinion, this is what the static code checker is saying 🤷

I can try and import again and see if I get complaints...

		muscle.ctrllimited = True
		muscle.ctrlrange = [0.0, 1.0]
		if lengthrange_from_joints:
		muscle.lengthrange = [vector_min[tend_idx], vector_max[tend_idx]]

Choose a reason for hiding this comment

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

Variable 'vector_min' and 'vector_max' are possibly uninitialized

Choose a reason for hiding this comment

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

if lengthrange_from_joints is true then those are definitely intialized no?

Choose a reason for hiding this comment

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

This is not my opinion, this is what the static code checker is saying 🤷

I can try and import again and see if I get complaints...