[IGSTK-Developers] Re: State Machine for ObjectRepresentation class

[Patrick Cheng]

Hi Luis,

This is being tested in the "application" for both tracked and 
non-tracked object, and it meets the function requirements. We should 
also modify the tests to reflect this change.

The reason I think those states are unnecessary is: we mixed the 
'inputs' and 'states' here.
We receive a 'ValidTimeStampInput', we get into 'VisibleState'
We receive a 'InvalidTimeStampInput', we get into 'InvisibleState'

I don't see the reason of using a 'AND-States' here.

Patrick

Luis Ibanez wrote:
> 
> 
> Hi Patrick,
> 
> I understand your desire for simplifying this class,
> and it seems to be a good idea to do so, *as long as*
> the class is still working for *all* the use cases that
> it is designed to satisfy.
> 
> 
> When you say that:
> 
> 
>   "after removing the states, the class still works"
> 
> 
> Do you mean that you run exhaustive testing in this class ?
> 
> 
> or simply that your application is still running after
> you remove the states ?
> 
> 
> 
> In particular:
> 
> 
> 1) Did you test that the objects still become invisible
>    when their timestamps expire ?
> 
> 
> 2) Did you test that the objects still become visible again
>    when they get updated (non-expired) timestamps ?
> 
> 
> 3) Did you run both tests for "tracked" and "non-tracked"
>    objects ?
> 
> 
> 
> The State Machine of this class may look unfamiliar because it
> is using the concept of AND-States, that we discussed during
> the annual review meeting. The states that you propose to
> remove are representing the cases when the position of a
> Spatial Object is updated, and it is in an visible or invisible
> states. You should test this cases with a tracked object (an
> object being attached to an igstk tracker).
> 
> 
> The functionality of Visibility/Invisibility is fundamental
> for the toolkit, and altering it represents a serious risk
> to the patient (since we may end up displayin objects in
> positions whose timestamps have expired. In other words,
> we could tell the surgeon that an object is in a particular
> position, when the information about that position happens
> to be outdated, and the object is no longer there.)
> 
> 
> 
> 
> Please let me know what tests you ran after removing the
> states from this class, that lead you to conclude that
> the states are unnecessary.
> 
> 
> 
> 
>    Thanks
> 
> 
> 
>       Luis
> 
> 
> 
> 
> --------------------
> Patrick Cheng wrote:
>> Hi Luis,
>>
>> I think the state machine of this class it too complicated, and 
>> causing the class to stuck in some stat won't be able to get out.
>>
>> This is the change I made for the demo.
>>
>> I took these states out:
>>   //igstkAddStateMacro( ValidTimeStamp );
>>   //igstkAddStateMacro( InvalidTimeStamp );
>>   //igstkAddStateMacro( AttemptingUpdatePositionAndVisible );
>>   //igstkAddStateMacro( AttemptingUpdatePositionAndInvisible );
>>
>> My reason is:
>> 1. TimeStamp is a property of SpatialObejct, not the Representation 
>> class.
>> 2. Only the visibility makes sense to the representation class
>>
>> When we verify the time stamp in representation class, we translate 
>> them into Valid/InvalidTimeStampInput, and let them act as a switch 
>> for the visibility, I think that's enough.
>>
>> The state machine is simpler, and it works.
>>
>> I know you did some recent refactoring of the class, so I want your 
>> opinion on this.
>>
>> PS: Both version of this class is attached
>> Patrick
>>
>>
> 
> 
> 
> 

-- 
Peng (Patrick) Cheng
Software Engineer

Imaging Science and Information Systems (ISIS) Center
Department of Radiology, Georgetown University Medical Center
2115 Wisconsin Avenue, Suite 603
Washington, DC, 20007

Work phone: 202-687-2902
Work fax:   202-784-3479
Cell phone: 202-674-5626