[IGSTK-Developers] Conclusion on igstkTransform issues - David&Patrick

[Luis Ibanez]

Hi David,

Adding boolean Flags inside classes is poor way of
implementing the logic that should be managed by a
State Machine.

The flags will plague the code with "if" statements,
and will generate spaghetti logic that cannot be fully
tested nor formally verified.

Conceptually, any new extra boolean flag in a class that
already has a State Machine is equivalent to duplicating
the number of States in that class, since now the developer
has to consider the effect of the flag on every transition.
Although it is likely that the flag will affect only a small
subset of all the States, it still will be up to the developer
to make sure that she/he will hack the appropriate places in
the code. The process of maintaining such style of code over
many years is very error-prone.


I agree with you that the use of special values is not the
correct way of managing error conditions, nor special cases.
My point, that I probably exposed poorly in my previous email,
is that by replacing the plain "doubles" with a custom made C++
class for representing the concept of Time and TimeLapses is the
best way of enforcing that the values will be used consistently
and that the application developers will not have the option of
performing risky operations.


A custom made Time class may have its own algorithmic rules,
such as making sure that:

                 Forever + 1000  = Forever

The point is that the Time class will remove from the API
the notion that a number is used for representing time.
When I mention the concept of "Forever", more than a value
in the Time class, it is a "State" of the time class. The users
of the Time class will not have to perform checks ("if"s) on
its state, instead we will implement inside the Time and TimeLapse
classes all the algebraic operations that we need for the time.


If that state is managed by the Time class itself, we can make sure
that Time operations are done consistently all over the toolkit,
instead of relying on developers to use the rules correctly on
every new class.



     Luis




------------------
David Gobbi wrote:
> Hi Luis, Patrick,
> 
> In general I don't like "special values" and would prefer if  there was a
> separate flag in igstkTransform to identify static vs. dynamic.
> 
> This would mean adding methods like SetStaticTranslationAndRotation(),
> and the "static" flag would be set when these methods are called.  The
> SetStaticFlag() method for setting the flag would be private, and there 
> would
> be a public GetStaticFlag() method for getting the flag.  How does this 
> sound?
> 
> On problem of defining a special value like "Forever" is that simple 
> math on
> very large floating point values can cause bad things to happen.  For 
> example,
> if "Forever" is used as a duration, then "Forever" plus the start time 
> might
> cause an overflow, or it might be equal to "Forever" due to limited 
> numerical
> precision, which is counter-intuitive.
> 
> So what I'm saying is that if we use a special value, we would need to 
> check
> for that special value whenever we do any math that involves the time.  But
> if we're checking for a special value, we might as well just have a flag
> instead.
> 
> - David
> 
> 
> Luis Ibanez wrote:
> 
>>
>>
>> Hi Patrick,
>>
>>
>> You are right about the potential misuse of the long times.
>>
>>
>> We can remomve that risk when we add a specific type for
>> representing Time and TimeLapses, since that type could
>> define a constant that is considered to be an infinite time.
>>
>>
>> In practice we can take the NumericTraits::max() of the
>> type that we choose for internal representation of the time.
>> We could call that static value something like:
>>
>>
>>                Time::Forever()
>>
>>
>> In this way, the intent of the value will be clearer
>> to the readers of the code.
>>
>>
>>
>>    Luis
>>
>>
>> -----------------------
>> Patrick Cheng wrote:
>>
>>> Hi Luis,
>>>
>>> I agree on statement 2) and 3).
>>>
>>> But for 1), to be 'safe by design', we should not make any 
>>> presumption that an operation does no go over a "long period of time".
>>>
>>> This "long duration" is easily being misused, and at least, it's not 
>>> consistent through out our code. some examples:
>>>
>>> **********************************************************************
>>> [[[Super Long Period]]]
>>> igstkTrackerTool.cxx
>>> Line 31
>>>   m_ToolCalibrationTransform.SetToIdentity( 1e300 );
>>>
>>> igstkMeshReader.cxx
>>> line91-92:
>>>   // Provide an identity transform with a long validity time.
>>>   const double validityTimeInMilliseconds = 1e30;
>>>
>>> [[[Wrong]]]
>>> igstkPivotCalibration.cxx
>>> Line 149
>>> this->m_CalibrationTransform.SetTranslationAndRotation( translation, 
>>> quaternion, 0.1, 1000);
>>> Line 286
>>> this->m_CalibrationTransform.SetTranslation(translation, 0.1, 1000);
>>>
>>> igstkPrincipalAxisCalibration.cxx
>>> Line 204
>>> this->m_CalibrationTransform.SetRotation( quaternion, 0.1, 1000);
>>> ************************************************************************
>>>
>>> Luis Ibanez wrote:
>>>
>>>>
>>>> Hi Patrick,
>>>>
>>>>
>>>> 1) The double use of Static and Dynamic transform will require
>>>>    SpatialObject to have both of them as member variables.
>>>>    Since SpatialObjects should be able to be "static" or "tracked".
>>>>
>>>>    What is the difficulty with having the "static" transforms be
>>>>    represented with a time stamp of long duration ?
>>>>
>>>>
>>>> 2) When composing transforms, the time stamp of the final transform
>>>>    must by the intersection among all the time stamps of the transforms
>>>>    involved on the composition.
>>>>
>>>>    We could add to the TimeStamp class, as service method that will
>>>>    compote the intersection between two TimeStamps. This operation
>>>>    is associative, so by applying it by pairs we can compose any
>>>>    number of TimeStamps.
>>>>
>>>>
>>>> 3) It is better to have a explicit "Compose" method, than having
>>>>    an operator overload.  The Compose method name could also make
>>>>    clearer whether we are pre-composing or post-composing the
>>>>    transform.
>>>>
>>>>
>>>>
>>>>
>>>>   Luis
>>>>
>>>>
>>>>
>>>> ---------------------
>>>> Patrick Cheng wrote:
>>>>
>>>>> Hi everybody,
>>>>>
>>>>> This is the conclusion of the discussion between david and me. 
>>>>> Welcome to comment on it.
>>>>>
>>>>> ======================================================================= 
>>>>>
>>>>>
>>>>> Problem 1. We need both static and dynamic transform. (Registration 
>>>>> and calibration transforms should be static, and tracker transforms 
>>>>> should be dynamic)
>>>>>
>>>>> Solution: Make subclasses of igstk::Transform,
>>>>> igstk::DynamicTransform and igstk::StaticTransform StaticTransform 
>>>>> which do not have time stamp. In base class we provide pure virtual 
>>>>> fuction:
>>>>> bool IsStatic()/IsDynamic();
>>>>>
>>>>>   Q? Do we some times have to switch the transform of a object from 
>>>>> dynamic to static or the other way around?
>>>>>
>>>>> ======================================================================= 
>>>>>
>>>>>
>>>>> Problem 2. We currently don't have a simple transform compose 
>>>>> method, and we are not taking care of time stamps in the current 
>>>>> implementation of the transform multiplication.
>>>>>
>>>>> e.g.
>>>>> Ta is static transform, Tb is dynamic, and Tc is dynamic.
>>>>> When we do T = Ta * Tb * Tc
>>>>> to get the final valid time stamp for T, we should first ignore Ta, 
>>>>> and pick up the earlier expiration time from Tb and Tc, and minus 
>>>>> current time, to get the valid time period for the T, and set a 
>>>>> right time stamp for it.
>>>>>
>>>>> Solution: Add a simple function or operator such as:
>>>>> transform = transform1.compose( transform2 )
>>>>> equals to:
>>>>> transform = transform1 * transform2
>>>>>
>>>>> This will avoid the wrong matrix composition, and make code simpler 
>>>>> and cleaner.
>>>>> Also this compose() method will calculate the correct time stamp 
>>>>> automatically according to predefine rules.
>>>>>
>>>>> ======================================================================= 
>>>>>
>>>>>
>>>>> David, I hope you still agree on these points.
>>>>>
>>>>>
>>>>
>>>>
>>>>
>>>
>>>
>>
>> _______________________________________________
>> IGSTK-Developers mailing list
>> IGSTK-Developers at public.kitware.com
>> http://public.kitware.com/cgi-bin/mailman/listinfo/igstk-developers
>>
> 
> 
>