Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Musculoskeletal dog creation #402

Open
wants to merge 30 commits into
base: main
Choose a base branch
from

Conversation

vittorione94
Copy link
Contributor

As per title

Copy link
Collaborator

@yuvaltassa yuvaltassa left a comment

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.

def add_markers(model):
bodies = model.find_all('body')

TOT_MARKERS = 0
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is TOT? TOTAL? maybe rename for clarity?

@@ -58,11 +58,19 @@ def add_motors(physics, model, lumbar_joints, cervical_joints, caudal_joints):
all_spinal_joints = []
for region in spinal_joints.values():
all_spinal_joints.extend(region)
root_joint = model.find('joint', 'root')

# root_joint = model.find('joint', 'root')
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not happy about the free joint -> euler angle decision. Why did you need this again? This will definitely become a problem for large rotations (e.g. rolling on the floor).

I think we should understand whatever the underlying cause was and fix it, rather than do this.

Ca_1.add('site', name='Ca_1_wrapping_forward', pos=[0, 0, -0.05], dclass='wrapping')
Ca_1.add('site', name='Ca_1_wrapping_backward', pos=[0, 0, 0.05], dclass='wrapping')

# L_1 = model.find('body', 'L_1')
Copy link
Collaborator

Choose a reason for hiding this comment

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

Either delete the commented code or add an option to activate it that is off by defaults

@@ -54,7 +54,20 @@ def create_torso(
sternum = [m for m in bones if 'Sternum' in m]
torso_bones = thoracic_spine + ribs + sternum # + ['Xiphoid_cartilage']
torso = parent.add('body', name='torso')
torso.add('freejoint', name='root')
# torso.add('freejoint', name='root')
Copy link
Collaborator

Choose a reason for hiding this comment

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

See my other comment.

@@ -23,7 +23,7 @@
from scipy import spatial


def create(model, skin_msh):
def create(model, mesh_file, name_mesh, asset_dir, tex_coords=True, transform=True):
"""Create and add skin in the dog model.

Args:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please fix the docstring

<site name="marker_torso_8" class="marker" rgba="1 0.5 0 0.5" size="0.01 0.01 0.01" pos="0.05 0.09 -0.02"/>
</body>
<body name="marker_0" mocap="true">
<site name="marker_0" class="marker" rgba="0 1 0 0.5" size="0.01 0.01 0.01"/>
Copy link
Collaborator

Choose a reason for hiding this comment

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

rgba="0 1 0 0.5" size="0.01 0.01 0.01"

Please put all repeated values into default classes

@@ -3,23 +3,24 @@

<option timestep="0.005" noslip_iterations="4"/>

<size njmax="900" nconmax="300" nkey="1"/>
<size njmax="900" nconmax="900" nkey="1"/>
Copy link
Collaborator

Choose a reason for hiding this comment

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

'm_tranversus_abdominis_R']

to_skip = [
# 'm_serratus_dorsalis_caudalis_L1',
Copy link
Collaborator

Choose a reason for hiding this comment

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

bunch of comments and commented-out lines in this file.

either delete or explain why they are here like

# uncomment the lines below for serratus muscles

or whatever.

Also for other comments and commented lines in this file.

return ' '.join(['%8g' % num for num in array])


def calculate_transformation(element):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add docstrings to this and all other nontrivial functions in this file

Copy link

Choose a reason for hiding this comment

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

+1

@@ -904,6 +904,7 @@
<attribute name="inflate" type="float"/>
<attribute name="material" type="reference"/>
<attribute name="rgba" type="array" array_type="float" array_size="4"/>
<attribute name="group" type="int"/>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Really?! Here?
Please submit this as a separate PR 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh yes this must've slipped in my branch to get it work, I'll create a separate PR.

Copy link

@lqh20 lqh20 left a comment

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.


import numpy as np

from scipy.spatial.transform import Rotation as Rot
Copy link

Choose a reason for hiding this comment

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

Does dm_control.utils.transformations work for this instead?


"""Add markers to the dog model, used for motion capture tracking."""

markers_per_body = {
Copy link

Choose a reason for hiding this comment

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

This is quite minor but generally file level constant should be in all caps. I also prefer making constants immutable if possible (so in particular, tuples rather than lists).

marker_body = model.worldbody.add('body', name="marker_" + str(i), mocap=True)
marker_body.add('site', name="marker_" + str(i), dclass="mocap_marker")

print("TOT_MARKERS:", TOTAL_MARKERS)
Copy link

Choose a reason for hiding this comment

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

nit: Maybe make this a little more informative.

from os.path import isfile, join

import numpy as np
from scipy.spatial.transform import Rotation as Rot
Copy link

Choose a reason for hiding this comment

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

Not sure this is essential but consider whether we need the scipy dependency here or whether the dm_control utils offer the same.


from utils import array_to_string, slices2paths

muscle_legs = extensors_back + extensors_front + \
Copy link

Choose a reason for hiding this comment

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

nit: prefer constants in all caps.

@@ -55,6 +55,7 @@ def create_torso(
torso_bones = thoracic_spine + ribs + sternum # + ['Xiphoid_cartilage']
torso = parent.add('body', name='torso')
torso.add('freejoint', name='root')

Copy link

Choose a reason for hiding this comment

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

revert this change.

'geom',
name='skinmesh',
mesh='skin_msh',
name=name_mesh,
Copy link

Choose a reason for hiding this comment

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

nit: maybe mesh_name is more intuitive?

distance = np.zeros((skin_vertices.shape[0], physics.model.nbody))
for i in range(1, physics.model.nbody):
distance = np.ones((skin_vertices.shape[0], physics.model.nbody)) * 1e6
for i in range(2, physics.model.nbody):
Copy link

Choose a reason for hiding this comment

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

+1

@@ -0,0 +1,232 @@
lateral = ['m_abdominis_Aponeurosis',
Copy link

Choose a reason for hiding this comment

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

nit: constants in all caps for readability.

return ' '.join(['%8g' % num for num in array])


def calculate_transformation(element):
Copy link

Choose a reason for hiding this comment

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

+1

dm_control/locomotion/walkers/assets/__init__.py Outdated Show resolved Hide resolved
dm_control/locomotion/walkers/assets/dog_v2/add_markers.py Outdated Show resolved Hide resolved
dm_control/locomotion/walkers/assets/dog_v2/add_markers.py Outdated Show resolved Hide resolved
dm_control/locomotion/walkers/assets/dog_v2/add_markers.py Outdated Show resolved Hide resolved
dm_control/locomotion/walkers/assets/dog_v2/utils.py Outdated Show resolved Hide resolved
dm_control/locomotion/walkers/assets/dog_v2/utils.py Outdated Show resolved Hide resolved
dm_control/locomotion/walkers/assets/dog_v2/utils.py Outdated Show resolved Hide resolved
dm_control/locomotion/walkers/assets/dog_v2/utils.py Outdated Show resolved Hide resolved
dm_control/locomotion/walkers/assets/dog_v2/utils.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@nimrod-gileadi nimrod-gileadi left a comment

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:

  1. 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
  2. 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

Args:
model: The model to add markers to.

Returns:
Copy link
Collaborator

Choose a reason for hiding this comment

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

If the function returns None you don't need to document it. Just drop it (and find any other places where such documentation is added).

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.

None yet

4 participants