[IGSTK-Developers] Bug in igstkTracker with calibration transform

[Patrick Cheng]

It seems that we have reached a nice agreement here.
To avoid the mis-calculation, we should offer that Transform concatenation.

I will go ahead to see if the orientation will look right or not.

We still need to think about where to put the offset of the 
SpatialObject's origin though.

Patrick

David Gobbi wrote:
> Hi Julien,
> 
> I agree with you.  It certainly does look like the right-left ordering 
> of the
> rotation multiplication is wrong.  I have identified four places in the 
> code
> where a fix is needed, the fix is always as follows:
> 
>  rotation *= transform.GetRotation();   //   WRONG
>  rotation = transform.GetRotation()*rotation; //  CORRECT
> 
> Go ahead and commit the fix.
> 
> To avoid this kind of thing in the future, we need a method to concatenate
> our igstkTransforms:
> 
>  /** Set this transform to the concatenation of the two given transforms:
>  * If you have two transforms and you wish to concatenate them, then
>  *   t1->Concatenate(t2,t1)   implies   t1 = t2*t1
>  void Transform::Concatenate(const Transform *t1, const Transform *t2);
> 
> If this makes sense, I can add it to igstkTransform.
> 
> - David
> 
> 
> Julien Jomier wrote:
> 
>> Hi David,
>>
>> Thanks for the explanation. I think I understand the way it works and 
>> I agree with the design but I think the implementation is wrong.
>> Let me try to explain.
>>
>> From the documentation:
>>
>>   T ' = P * R^-1 * T * C
>>
>>   where:
>>   " T " is the original tool transform reported by the device,
>>   " R^-1 " is the inverse of the transform for the reference tool,
>>   " P " is the Patient transform (it specifies the position of
>>         the reference with respect to patient coordinates), and
>>   " T ' " is the transformation that is reported to the spatial objects
>>   " C " is the tool calibration transform.
>>
>> So we are interested by this part of the equation here:
>>
>> T*C = RotationT * (RotationC+TranslationC)+ TranslationT
>> T*C = RotationT*RotationC + RotationT*TranslationC + TranslationT (1)
>>
>> Looking at the code:
>>
>> // start with ToolCalibrationTransform
>> rotation = toolCalibrationTransform.GetRotation();
>> translation = toolCalibrationTransform.GetTranslation();
>>
>> // transform by the tracker's tool transform
>> rotation *= transform.GetRotation();
>> translation = transform.GetRotation().Transform(translation);
>> translation += transform.GetTranslation();
>>
>> Which can be written as:
>>
>> T*C = RotationC*RotationT + RotationT*TranslationC + TranslationT (2)
>>
>> Therefore, comparing (1) and (2) the rotation is computed in the wrong 
>> order or maybe I'm missing something really obvisous...
>>
>> And the m_PatientTransform is following the same implementation so 
>> might be wrong as well.
>>
>> Let me know what you think,
>>
>> Julien
>>
>>
>> David Gobbi wrote:
>>
>>> Hi Julien,
>>>
>>> I think that the tool calibration transform code is correct the way 
>>> it is, but note that the
>>> way the code is written, the tool calibration transform goes from the 
>>> "calibrated tool"
>>> coordinate system to the "raw tool" coordinate system.  This might be 
>>> the opposite of
>>> what you expect.
>>>
>>> There is a very good reason for having the calibration transform go 
>>> in this direction.
>>> You know how an igstkTransform defines a rotation followed by a 
>>> translation? Since the tool tip is at (0,0,0) in the "calibrated" 
>>> coord system, our "calibration transform"
>>> applies a rotation about the tool tip, which is at (0,0,0) in the 
>>> calibrated coord system,
>>> followed by a translation to the "raw" tool tip location.
>>>
>>> This means that even if the rotation portion of the calibration 
>>> transformation is incorrect,
>>> the "tool tip" portion of the calibration will still give the correct 
>>> tool tip location, since the
>>> rotation part of the calibration occurs around the tool tip 
>>> position.  If the calibration
>>> transform was the other way around, then we would not get this nice 
>>> independence of
>>> translation from rotation.
>>>
>>> If you already had your calibration transformation going in this 
>>> direction, email me and I
>>> will take a close look at the code to make sure that there is no bug.
>>>
>>> - David
>>>
>>>
>>> Julien Jomier wrote:
>>>
>>>> Hello,
>>>>
>>>> There is an important bug in the igstkTracker class preventing any 
>>>> calibration matrix to be applied correctly.
>>>>
>>>> The problem is in the UpdateStatusSuccessProcessing() function.
>>>>
>>>> The calibration transform should be applied first (rotation 
>>>> variable) and then the transform from the tracker (transform variable).
>>>> However the code is as follow:
>>>>
>>>> rotation *= transform.GetRotation();
>>>>
>>>> whis is in the wrong order and should be:
>>>>
>>>> rotation = transform.GetRotation()*rotation;
>>>>
>>>> Let me know if I should fix it, or if someone else is currently on 
>>>> the igstkTracker class right now or if I'm missing something...
>>>>
>>>> Julien
>>>> _______________________________________________
>>>> IGSTK-Developers mailing list
>>>> IGSTK-Developers at public.kitware.com
>>>> http://public.kitware.com/cgi-bin/mailman/listinfo/igstk-developers
>>>>
>>>
>>>
>>>
>>
> 
> _______________________________________________
> IGSTK-Developers mailing list
> IGSTK-Developers at public.kitware.com
> http://public.kitware.com/cgi-bin/mailman/listinfo/igstk-developers
> 
>