jrl-umi3218 / RBDyn

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

Various improvements #72

Closed BenjaminNavarro closed 4 years ago

BenjaminNavarro commented 4 years ago

This PR brings two major improvements:

  1. The ability to compute a Jacobian using any body as reference, without having to reconstruct a new graph. Thanks to @sonny-tarbouriech for the original implementation
  2. Turn Werror back on. All warnings have been fixed and the CI passes on all platforms. The main sources of warnings were:
    1. Pessimistic moves: things like return std::move(obj); which disables copy elision
    2. signed/unsigned integer implicit conversions. All indexing is based on signed integers (partly to handle the -1 case) but STL containers use unsigned integers for size and indexing. All conversions are now explicit and performed using static_cast
BenjaminNavarro commented 4 years ago

I'll see how to add a unit test for that, even though it might be a bit difficult to test properly.

I commented when necessary and will implement your suggestions otherwise

sonny-tarbouriech commented 4 years ago

I did some modifications in the Jacobian class to make the new feature (the ability to compute a Jacobian using any body as reference) compatible with the original version.

I also modified the Jacobian::subMultiBody function in order to return a MultiBody going from the reference frame to the body frame by adapting the transforms between the bodies and the joint directions when the kinematic path is reversed. This way, computing FK, FV and FA for this MB directly gives the poses, velocities and accelerations of bodies expressed wrt the reference frame.

I implemented a unit test with a 2D dual-arm robot to check that the Jacobian computation and the velocities from the subMultiBody give consistent results.

There is still one fail in CI with Windows but I don't know why.

@gergondet could you tell me if the changes are OK? Do you know what's the problem with the CI?

aescande commented 4 years ago

There is still one fail in CI with Windows but I don't know why.

You have a bug due to a mismatch between l314 and l319 of JacobianTest.cpp: you're creating a dual arm with floating base, but you associate an empty configuration to it. This ends up crashing in forwardKinematics->Joint::pose<double>->QuatToE<double> when the conversion tries to access non-existing elements.

I'm surprised only windows STL catches that

sonny-tarbouriech commented 4 years ago

There is still one fail in CI with Windows but I don't know why.

You have a bug due to a mismatch between l314 and l319 of JacobianTest.cpp: you're creating a dual arm with floating base, but you associate an empty configuration to it. This ends up crashing in forwardKinematics->Joint::pose<double>->QuatToE<double> when the conversion tries to access non-existing elements.

I'm surprised only windows STL catches that

Thanks Adrien, I wanted to see if the test would pass with a floating-based robot but forgot to update the joint configuration.

gergondet commented 4 years ago

Sorry for the very long delay on this. I've rebased on master and merged, thanks for the work @BenjaminNavarro @sonny-tarbouriech