KG CR 022305

From IGSTK

I reviewed igstkSpatialObject, igstkCylinderObject, igstkObjectRepresentation, igstkCylinderObjectRepresentation, and igstkScene on 2/23 after a clean checkout of the current ITK, IGSTK, and IGSTK Sandbox (not the Release-Iteration-2-Sandbox-Phase tagged version) with fltk1.1.6 and VTK 4.4.

Code Review Check List

• All files must have the copyright notice at the top

These files have the ITK copyright at the top. I know these files may be candidates to be moved to a spatial object core library, but at this time if they are in IGSTK then shouldn't they have the IGSTK copyright?

• Complete class doxygen description

These files have the brief comments but not the complete ones

All other code review checklist items are fine, good work on the test coverage!

Some questions:

• igstkObjectRepresentation::SetSpatialObject allows the possibility that the spatial object a view representation is tied to to change. While I see it is a private method, it still seems strange that one would reset this relationship, I would think it should be durable for the lifetime of the view representation object (therefore set in the Constructor, which actually calls this method)?
• itkTypeMacro is used instead of igstkTypeMacro, even though the definition of these macros seems the same, and igstkTypeMacro is preferred in other igstk code. Is this because these objects inherit from itkObject?

Code looks good!!!

Luis Comments

• Copyright notice has been updated. We replaced the old ITK notice with the new IGSTK one. Thanks KG for pointing this out.

• I agree with KG in that the SetSpatialObject() could be a relationship for the life of the Representation class. We can require this in the structure of the StateMachine, instead of forcing it into the Constructor. Meaning, we can simply invoke a NoAction as a response to a RequestSetSpatialObject() that arrives when the object has already been set.
  • This has now been done. The transitions have been modified in such a way that only the first call to RequestSetSpatialObject is honored. Any subsequent invokation of this method will result in a NoAction operation.

• itkTypeMacro was replaced with igstkTypeMacro. Thanks KG for pointing this out.

• Our pending item is to improve documentation. This may require more team effort.