Body models refactoring

[kristijanbartol]

I noticed that the extended SMPL classes abuse the original SMPL in several places, for example:

1. The override implementation of the forward() method returns a Python dictionary instead SMPLOutput or ModelOutput as it is done in the original class. This forces the default dictionary syntax, such as model['vertices'] instead of model.vertices. Also, it creates code redundancy that we see in several places where the outputs are recreated as dictionaries, such as here and here. The proper solution is to use an (extended) data class definition such as this.
2. The argument list for extended classes can be simplified. Instead of explicitly providing arguments, even repeating the arguments of the original method, the arguments can be kept as (*args, **kwargs) such as in SPIN or least we should avoid repeating the arguments from the original methods.
3. The keypoint (re)mapping logic should be moved from the body models implementations as the keypoints have nothing to do with the body models once they are extracted from the vertices.
4. The gendered models such as this seem redundant as the original SMPL model already accepts the gender parameter.
5. Improve unit tests (at least for the code coverage).

Note that this is not a comprehensive or fixed list of issues/proposals, therefore, I am opening a discussion.

[caizhongang]

Hi @kristijanbartol , thank you very much for the discussion. The current implementation is far from perfect and we appreciate the community's feedback. Nevertheless, we would like to clarify some of the design intentions, and then we can see if it's still necessary to refactor some of the points.

---

1. The override implementation of the forward() method returns a Python dictionary instead SMPLOutput or ModelOutput as it is done in the original class. This forces the default dictionary syntax, such as model['vertices'] instead of model.vertices. Also, it creates code redundancy that we see in several places where the outputs are recreated as dictionaries, such as here and here. The proper solution is to use an (extended) data class definition such as this.

We find dictionary is more flexible than SMPLOutput or ModelOutput. For an ordinary Python user, a dictionary seems more intuitive and users can easily add, remove, and check keys in the output. Dictionary has also been extensively used in the OpenMMLab family to act as a media for data transfer. As for code redundancy, there are indeed additional lines of code to recreate the dictionary but it hardly contributes to any overhead as the original values are not copied.

---

2. The argument list for extended classes can be simplified. Instead of explicitly providing arguments, even repeating the arguments of the original method, the arguments can be kept as (*args, **kwargs) such as in SPIN or least we should avoid repeating the arguments from the original methods.

We expose the most frequently used arguments such as betas, global_orient, and so on purposely so that anyone who is not sure of what arguments to use can easily refer to the docstring in MMHuman3D, instead of tracing all the way back to SMPL-X. Note that we also allow additional, less frequently used arguments to pass through with **kwargs: example.

---

3. The keypoint (re)mapping logic should be moved from the body models implementations as the keypoints have nothing to do with the body models once they are extracted from the vertices.

Keypoint mapping is a pretty troublesome operation and we devote a good amount of effort to sort it out for all datasets. This problem is especially severe in MMHuman3D because it has to accommodate many works and some (and potentially future works) come with different keypoint conventions used in the training. The current implementation hopes to wrap processing inside the body model so the output to users is ready to use (if the user sets the keypoints conventions right at initialization of the body model). Keypoint mapping can be moved outside the body model, however, that means everywhere the body model is used, the keypoint mapping has to be added, potentially resulting in a lot of duplications.

---

4. The gendered models such as this seem redundant as the original SMPL model already accepts the gender parameter.

The GenderedSMPL serves a different role. The original SMPL model takes gender as a single string to create male/female/neutral body models. In contrast, GenderedSMPL takes gender with shape (B, ), where B is batch size. GenderedSMPL returns a batch of parameters/keypoints that is a mixture of male/female body models. The purpose of GenderedSMPL is for datasets such as 3DPW where the SMPL annotations come with genders (i.e. a mixture of male and female SMPL parameters). This section from SPIN does a similar job.

---

5. Improve unit tests (at least for the code coverage).

Agree! And we also appreciate the community's contributions.

[kristijanbartol]

We can settle with the given arguments. I am least satisfied with 3. the keypoint (re)mapping logic, but as it's only local (inside of the SMPL class), it might stay as it is, for now.

Regarding the tests, we can gradually revisit them when doing other PRs.