graspit-simulator / graspit

The GraspIt! simulator
http://graspit-simulator.github.io/
Other
174 stars 82 forks source link

fixed tendon wrapping error #173

Closed mateiciocarlie closed 4 years ago

mateiciocarlie commented 4 years ago

Fixed error in tendon wrapping due to new transforms API.

mkghaas commented 4 years ago

For reference, here are the implementations of the transf class before and after the Eigen switch.

This is strange... It appears that the lines you changed used to just get the 3x3 rotation matrix and multiply it by the vector. This functionality should have been preserved by the switch so I'm not sure why it broke.

To make things stranger, looking at the * operator in the current code, the way you have it now applies the transform to the vector including the translational part. I'm not sure if that's what you intended.

To be honest though, it's difficult doing forensics on decade old code. Does it work now?

mkghaas commented 4 years ago

Looking at the implementation of the mat3::set method and the implementation of the mat3::operator* I have a feeling that they are in row-major and column-major format respectively. This would mean that you were getting the inverse of the rotation to the vectors than you were expecting. As long as there are no translational components in the transforms by which you are multiplying this would explain why using the new operator* fixed your issues. But if there is any translational component to the transforms you are multiplying by this would break.

Could you check if it also works if instead of removing the .affine() you switched to .affine().transpose()? I remember very hazily that I had to fix a bunch of lines that included .affine() by transposing the resulting matrix.

mateiciocarlie commented 4 years ago

Yes, the intended functionality is to apply the complete 4x4 transform, including the translational part. Conceptually, this is moving a point from one coordinate system to another.

Historically, there used to be two classes , "position" and "vec3". By default, when you would apply a transform to a position, the translation part of the transform would be applied too. However, when you applied a transform to a vec3, only the rotation would be applied (since presumably a vec3 is intended to contain a direction, as opposed to a point in space). However, I was not aware of that, and I used to use position and vec3 interchangeably in my code, which led to countless bugs.

In the current version, if you get the affine() of a transform before multiplying, you only apply the rotation part of the transform to whatever it is you are multiplying.

When the switch to Eigen happened, it was not clear when the translation part was wanted and when not, so additional bugs were introduced if all transform multiplications were uniformly changed to use .affine(). Here is a case where the translation was indeed wanted, which is why I dropped the .affine().

mkghaas commented 4 years ago

Interesting. How did this work before then? affine() appears to have always returned a 3x3.

In any case, if you intended to get the full transformation and not just a frame rotation, this looks good to me.

mateiciocarlie commented 4 years ago

I don't think it ever worked again after the Eigen transition. It's very likely it last work in 2015. And yes, the intent is to get and apply the full transformation.

mkghaas commented 4 years ago

I was wondering how it worked even before the Eigen transition. It looks to me like it only ever applied the rotation.

As far as I'm concerned feel free to merge.