[dynamics]: deep copy shapes

[costashatz]

This PR adds the possibility to deep copy shapes. In robot_dart, we clone very frequently robots and we would like to be able to make deep copies of the robot. At the current state, DART does not copy shapes. This is generally OK unless we would like to alter the shapes later (e.g. create a damage or change the ColorMode of a MeshShape). So, this PR solves the above issue by making it able to deep copy shapes. I hope that I did not forget any shape. The only shape that I do not deep copy is the SoftMeshShape (for now I just return a nullptr in the new copy function; you might want to handle this differently [e.g. output a warning or exception]). Here there is a tight relationship between the SoftBodyNode it is attached to, so I do not touch this. By the way, when cloning a skeleton, is this type of meshes re-created to point to the correct BodyNode?

Let me know if this is OK for you or you would like to handle it differently. If this is OK, we can do unit tests.

---

Before creating a pull request

• Document new methods and classes
• Format new code files using clang-format

Before merging a pull request

• Set version target by selecting a milestone on the right side
• Summarize this change in CHANGELOG.md
• Add unit test(s) for this change

[jslee02]

The only shape that I do not deep copy is the SoftMeshShape (for now I just return a nullptr in the new copy function; you might want to handle this differently [e.g. output a warning or exception]).

SoftMeshShape is a special shape that wouldn't need the deep copy functionality. This is because SoftMeshShape should be created only by SoftBodyNode in the constructor of SoftBodyNode. So SoftMeshShape is automatically created when cloning skeleton and so SoftBodyNode as well.

For this, I would make the constructor of SoftMeshShape private and add SoftMeshShape as its friend class, and the clone implementation of SoftMeshShape would just return nullptr adding an error log saying this function shouldn't be used for any case.

By the way, when cloning a skeleton, is this type of meshes re-created to point to the correct BodyNode?

Yes, for the reason above.

If this is OK, we can do unit tests.

Yeah, unit tests would be great!

[costashatz]

For this, I would make the constructor of SoftMeshShape private and add SoftMeshShape as its friend class, and the clone implementation of SoftMeshShape would just return nullptr adding an error log saying this function shouldn't be used for any case.

I did all the rest (apart from unit tests: I'll do them later), but I did not understand what you mean here. Can you be more elaborate? I did the log, but not sure what you mean by making the constructor of SoftMeshShape private (in terms of the expected behavior).

Yeah, unit tests would be great!

I'll do this next..

[jslee02]

I did all the rest (apart from unit tests: I'll do them later), but I did not understand what you mean here. Can you be more elaborate? I did the log, but not sure what you mean by making the constructor of SoftMeshShape private (in terms of the expected behavior).

It's an optional change, but what I expected was something like:

class SoftMeshShape {
private:
  friend class SoftBodyNode;
  SoftMeshShape();
};

This way SoftMeshShape can be created only by SoftBodyNode. I'm happy to make this change myself though.

[costashatz]

This way SoftMeshShape can be created only by SoftBodyNode. I'm happy to make this change myself though.

Ah I see; I am aware of this concept, but I am not sure how this change is related to this PR though. I'd make it in a different PR, because it handles a different thing/behavior, no? Feel free to do this in case this is important for you (I just tried moving the constructor and there were many errors, so since this is not related to this PR, I'd like to not spend time for this ;)).

I'll do the unit tests and then feel free to edit. By the way, should we include this behavior into the Skeleton::cloneSkeleton() function? Even with an optional boolean parameter (e.g. Skeleton::cloneSkeleton(bool deepCloneMeshes=false) which can be false by default). At the moment, I prefer not to (to be honest), but from a DART API point of view, it makes more sense, no?

[jslee02]

I am aware of this concept, but I am not sure how this change is related to this PR though.

Please don't worry about the change for this PR. I can make this change in a separate PR.

By the way, should we include this behavior into the Skeleton::cloneSkeleton() function?

Yeah, I was thinking about making a PR for this change. World would also need similar options. By the way, let's save those changes for the future PRs to keep this PR a good size to review.

[costashatz]

Yeah, I was thinking about making a PR for this change. World would also need similar options. By the way, let's save those changes for the future PRs to keep this PR a good size to review.

Good. I agree. I'll do the unit tests and leave the rest for future PRs. Thanks!

[jslee02]

@costashatz For clarification, is it correct that you'd add the unit tests to this PR?

[costashatz]

@costashatz For clarification, is it correct that you'd add the unit tests to this PR?

That is indeed my intention. Although I am not sure if I am going to find time for this any time soon. Is it OK if we merge this and create an issue about the tests?

[jslee02]

Sounds good to me