search

jrl-umi3218 / RBDyn

RBDyn provides a set of classes and functions to model the dynamics of rigid body systems.
BSD 2-Clause "Simplified" License
163 stars 47 forks source link

Store mimic joint information #23

Closed gergondet closed 7 years ago

gergondet commented 7 years ago

This PR makes it possible to store mimic joint information in a Joint object.

By itself, the information is not very useful in RBDyn and there is currently no way to enforce the mimic relationship in MultiBodyConfig for example.

See jrl-umi3218/Tasks#13 for usage in control.

haudren commented 7 years ago

This looks pretty good, but I'm unsure on the actual implementation: the fact that we can call set_mimic() suggests that we could somehow clear_mimic(). I would be in favor of actually having a method make_mimic(joint, mimic_params) that returns a new joint, in order to make it clear that there is no way to modify the mimic state once added to the MultiBody. We could also add a new type MimicJoint : public Joint but I'm afraid that testing dynamic_cast<MimicJoint> has succeeded is too cumbersome.

I will also add a few comments directly in the PR.

gergondet commented 7 years ago

I've made the changes concerning the case.

Concerning setMimic or makeMimic, why not meet in the middle by keeping the current implementation but renaming it makeMimic to make it clear that the operation cannot be reversed.

I don't think the third option (a new type) would be good. Structures in RBDyn store joints by value and would have to be changed plus the dynamic casting to find out whether a joint is a mimic or not seems like a waste to avoid storing 4 additional values in the structure.

What happens if we want to have a non-uniform mimic offset on a non-revolute joint ? mimic_offset should in that case probably be a std::vector.

I made myself the same remark when writing the function, I guess this can be added. Note: in an urdf, this is not an option.

haudren commented 7 years ago

As we mostly target URDF, I'd say we can drop it. If someone ever needs it, we can go back to this implementation.

And yes, your suggestion seems sensible, I was only providing the new type as an alternative but it is not desirable.

gergondet commented 7 years ago

I've made the change for setMimic and I'm ok with dropping the idea of a vector offset for now.