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

[Luis Ibanez]

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
> 
>