[IGSTK-Developers] Bug in igstkTracker with calibration transform

[Patrick Cheng]

I agree, more transforms bring more errors.

And the problem with the CylinderObject's origin, should be solved by 
adjusting it's coordinates system, to make it align with Z, and tip 
location at (0,0,0).

I will test the Calibration Transform's rotation part against the recent 
change in the tracker class, to see if we can get the CylinderObject's 
orientation right.

Patrick

Julien Jomier wrote:
> Hi Patrick,
> 
> Thanks for the quick reply.
> 
>  > The problem here is. We should not put the offet of the from
>  > SpatialObject's origin to its tip in the Calibration Transform in step
>  > 1. This will lead to a wrong registration result, because we are not
>  > reporting the exactly location of the tip (the fiducial) now.
> 
> Well, you can add the offset in the ToolCalibrationTransform and tell 
> the registration algorithm to use point different from (0,0,0) as the 
> tip of the needle.
> Anyways, I really don't like the idea of adding a new transform and I 
> think all the tools should have a tip at (0,0,0).
> 
> my two cents,
> 
> Julien
> 
> Patrick Cheng wrote:
>> Hi Julien,
>>
>> The process of the demo is as following:
>>
>> 1. In constructor, we need to set the calibration transform, to get 
>> the tip position, because we need to use the tip to touch the fiducial 
>> to get its transform in tracker space.
>>
>> 2. Then we use the translation part of that transform to register with 
>> the points identified in the image.
>>
>> 3. The result transform of the registration, will be set to the 
>> PatientTransform.
>>
>> 4. We then attach the SpatialObject to the tracker. The tracker 
>> calculate the final transform T' (with PatientTransform and 
>> CalibrationTransform applied) and sent to the SpatialObject.
>>
>> The problem here is. We should not put the offet of the from 
>> SpatialObject's origin to its tip in the Calibration Transform in step 
>> 1. This will lead to a wrong registration result, because we are not 
>> reporting the exactly location of the tip (the fiducial) now.
>>
>> We can probably set a new calibration transform between step 3 and 4. 
>> But that's erroneous, user might forget that step.
>>
>> The solution is:
>> 1. the one you proposed. Use a common coordinate frame.
>> 2. Let the spatial object take care of that internally. Either set a 
>> new Calibration Transform when it's being attached to a tracker, or 
>> apply an additional transform after it gets the transform from the 
>> tracker.
>>
>> Patrick
>>
>>
>> Julien Jomier wrote:
>>
>>> Ok, I think everything is working well now :)
>>>
>>>  > Julien, what do you think of adding a protected method to
>>>  > igstkSpatialObject
>>>  > to allow subclasses to adjust the position/orientation of the object?
>>>
>>> I don't really like the idea of modifying the internal object because 
>>> we  don't really have to. Basically the CalibrationToolTransform 
>>> defines where the tracker his w.r.t to the origin of the object so 
>>> you don't have to add an offset somewhere else.
>>>
>>> I've started an application using IGSTK and the ultrasound probe 
>>> tracking and I've used the CylinderObject without any issues.
>>>
>>> Patrick, can you tell me which transform your are optimizing when you 
>>> do your registration? From what I've seen during the demo, the tool 
>>> is already calibrated so the CalibrationTransform should be set to 
>>> identity and you are optimizing the PatientTransform, which is 
>>> correct. However if you are using the CylinderSpatialObject you know 
>>> that the tip of the needle will be roughly h/2 (with h the height of 
>>> the needle) so you have to set the CalibrationTransform accordingly.
>>>
>>> That said, I think we should define a common coordinate frame for all 
>>> igstk objects. Meaning that by default the tip of the object will be 
>>> at (0,0,0) and the main orientation will be along the Z-Axis. What do 
>>> you think about changing the CylinderObject to follow this assumption?
>>>
>>> Julien
>>>
>>> David Gobbi wrote:
>>>
>>>> Hi Patrick,
>>>>
>>>> It goes to show that I need to spend more time in the lab working 
>>>> with the
>>>> tracking systems, to make sure that my code works the way it should.
>>>>
>>>> At the T-con me an Luis talked about applying translations directly to
>>>> the spatial object, and we agreed that the safe way to do what you want
>>>> is to write a new spatial object class that places the origin at the 
>>>> end of the
>>>> cylinder instead of at the center, and that orients the cylinder 
>>>> along the
>>>> right axis.
>>>>
>>>> Julien, what do you think of adding a protected method to 
>>>> igstkSpatialObject
>>>> to allow subclasses to adjust the position/orientation of the 
>>>> object?  I notice
>>>> that itkSpatialObject has an IndexToWorldTransform and an
>>>> ObjectToParentTransform, but I'm not sure what they are meant for.
>>>>
>>>> - David
>>>>
>>>>
>>>> Patrick Cheng wrote:
>>>>
>>>>> 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
>>>>>>
>>>>>>
>>>>>
>>>>
>>>>
>>>>
>>>
>>>
>>
>>
> 
>