From IGSTK

Contents 1 Classes Scheduled for Code Reviews 2 Reviews 2.1 igstkObjectRepresentation 2.1.1 Luis' Comments 2.1.2 K2's Comments 2.2 igstkCylinderObjectRepresentation 2.2.1 Luis' Comments 2.3 igstkTubeObject 2.3.1 Luis' Comments 2.3.2 K2's Comments 2.4 igstkTubeObjectRepresentation 2.4.1 Luis' Comments 2.4.2 K2's Comments 2.5 igstkTubeReader 2.5.1 Luis' Comments 2.6 igstkMeshObject 2.6.1 David's Comments 2.6.1.1 Notes on MeshObjectRepresentation 2.6.2 Patrick's Comments 2.7 igstkMeshReader 2.7.1 David's Comments 2.7.2 Patrick's Comments 2.8 igstkTimeStamp 2.8.1 Luis's Comment 2.8.2 David's Comments 2.9 igstkRealTimeClock 2.9.1 Luis's Comment

Classes Scheduled for Code Reviews

These classes are ready for immediate review.

Name	Author	Reviewer 1	Reviewed	Code Fixed	Reviewer 2	Reviewed	Code Fixed	Needs Merge	Moved
igstkObjectRepresentation	Julien/Patrick	LI	Y	N	KG	N	N	Must not be merged	N
igstkCylinderObjectRepresentation	Julien	LI	Y	N	KG	N	N	Merge	N
igstkTubeObject	Julien	LI	Y	N	KG	N	N	Merge	N
igstkTubeObjectRepresentation	Julien	LI	Y	N	KG	N	N	Merge	N
igstkTubeReader	Julien	LI	Y	N	DG	N	N	Merge	N
igstkMeshObject	Julien	DG	Y	N	PC	Y	Y	Merge	N
igstkMeshReader	Julien	DG	Y	N	PC	Y	Y	Merge	N
igstkTimeStamp	Patrick	DG	Y	Y	LI	Y	N	Merge	N
igstkRealTimeClock	Patrick	LI	Y	N			N	Merge	N

K2 - I must be missing something. The files I am on the list to review do not have any changes other than fixes for BUG 3073, styles issues, for igstk*ObjectRepresentation?

Reviews

igstkObjectRepresentation

Luis' Comments

The changes on the StateMachine of this class are incorrect.

The method RequestVerifyTimeStamp() has been modified in order to push a "ValidTimeStamp" input if the transform from the spatial object has an expiration time that preceed the start time. This is a useless test, because that condition is always satisfied.

if ( m_SpatialObjectTransform.GetExpirationTime() <= 
     m_SpatialObjectTransform.GetStartTime())
  

Since this condition is always true, then the actual useful part of the test is never performed, which is where the timestamp of the SpatialObject transform should be compared against the time to render.

This is a dangerous bug, because the purpose of checking the timestamp is to identify when the position of an object is not know for the time at which the scene is going to be rendered. With the current changes, objects will always be rendered even if they have expired timestamps with respect to the TimeToRender.

The approach of moving the StateMachine logic into a cascade of "if" statements inside the Request method is not consistent with the design of the toolkit and must be avoided.

This files must not be moved into the main toolkit.

K2's Comments

Indenting appears to use tabs not spaces. The lines are misformatted in my editor, particularly in methods SetColor and UpdateActorsPositionProcessing.

A few instances of line lengths greater than 79 characters, though long names on identifiers makes it difficult to shorten.

It is not entirely clear where the if test can be moved to.

igstkCylinderObjectRepresentation

Luis' Comments

The modifications in this files are a bug fix, and style changes.

The bug fix is in the position of the Cylinder in space:

 cylinderActor->SetPosition(0,0,m_CylinderSpatialObject->GetHeight()/2.0);

versus:

 cylinderActor->SetPosition(0,0,-m_CylinderSpatialObject->GetHeight()/2.0);

This SHOULD NOT have motivated the copy of this file in the Sandbox. Bug fixes should be done directly in the IGSKT library. Files should not be moved into the Sandbox with the sole purpose of fixing bugs in them.

The acceptable reasons for moving files into the Sandbox are the following:

1. Refactoring
2. Adding new features in the class itself
3. API changes related to new classes or new features of other classes in the Sandbox

The file is good to be merged, but we should avoid bringing files into the Sandbox just for fixing bugs on them.

igstkTubeObject

Luis' Comments

Changes in this file include the following:

1. Add Clear() method for clearing points
2. Add SetTubeSpatialObject() method (private)
3. Add as Friend class the TubeReader

Unfortunately none of the CVS commit comments mention the important changes (2) and (3) !

The Clear() method has been made "public",... but that seems to be Ok because the AddPoint() method is also available in the public interface.

The file is good to be merged.

K2's Comments

Looking in the cvs logs I see no changes made since December???

igstkTubeObjectRepresentation

Luis' Comments

This file has the folllowing changes:

1. A bug fix for setting the Opacity
2. Style changes

The first one was the reason for moving the file from the IGSTK library into the Sandbox. This is NOT an acceptable reason for moving files into the Sandbox. Bug fixes MUST be done in the main library.

The acceptable reasons for moving files from the main IGSTK in to the Sandbox are:

1. Refactoring
2. Adding new features in the class itself
3. API changes related to new classes or new features of other classes in the Sandbox

The file is good to be merged, but we should avoid bringing files into the Sandbox just for fixing bugs on them.

K2's Comments

I must be looking in the wrong place - the only modification I see is a fix for BUG 3073, a style bug, in the header file.

igstkTubeReader

Luis' Comments

The changes in this file are

1. Using internally a TubeSO instead of a TubeGroupSO.
2. Add a Friend class in order to secure the passage of TubeSpatialObject as output.
3. Replacing the way the points are copied.

Codying style, operators of methods are missing spacing around parenthesis. For example:

1. if(tube) in line 63 of the .cxx file should be if( tube )
2. most while() and if() are missing spaces around its operators. Like line 57

 while(it != children->end()) should be 
 while( it != children->end() )

Once fixed this small code style issue, the class is good for merging.

igstkMeshObject

David's Comments

• No changes to this file have been to this file in the main repository since it was moved to Sandbox, it can be directly merged to the main repository
• Changes since it was moved to Sandbox include:
  • addition of private SetMesh() method to be used by a helper class of igstk::MeshReader called MeshReaderToMeshSpatialObject
  • style fixes
• All the typedefs other than the standard IGSTK typedefs should be protected to avoid exposing ITK types (none of these typedefs are used in the public API for this class)

Notes on MeshObjectRepresentation

I noticed that igstkMeshObjectRepresentation was also recently copied to the sandbox, to add opacity support. Opacity should be added to the requirements before this file is merged back to main repository. I also suggest that a polygon depth sort should be added to this class, otherwise the opacity will lead to rendering artifacts.

Patrick's Comments

• Code coverage 79%
  • AddTriangleCell methods is not called (fixed)
• MeshObject.h extra private keyword(fixed)
• MeshObject.h main doxygen comments wrong class name(fixed)
• MeshObject.h main doxygen comments is not being extracted (fixed)
  • \par Overview is not recognizable in current doxygen page, the whole comment is missing when using this tag
  • same situation can be found in
    1. IGSTK\Source\igstkGroupObject.h
    2. IGSTK\Source\igstkGroupObject.h
    3. IGSTK\Source\igstkTubeGroupObject.h
    4. Sandbox\IGSTK\Source\igstkVascularNetworkObject.h
  • These are not fixed yet
• Naming of the Friend class MeshReaderToMeshSpatialObject, should it be MeshReaderToMeshObject? Since we have MeshObject class instead of MeshSpatialObject.
• Extra include header file (fixed)
  • No need for #include "igstkMacros.h" in igstkMeshObject.h
  • No need for #include "igstkEvents.h" in igstkMeshObject.cxx

igstkMeshReader

David's Comments

• This class has not been changed in the main repository since it was copied to the sandbox, the merge will be straightforward
• The main change since it was moved to the sandbox was the addition of a helper class for efficiency
• The "MeshType" typedef should be protected, not public

Patrick's Comments

• Should add state machine logic for GetITKMesh before move the code?
• Extra include header file (fixed)
  • No need for "igstkMacros.h", "itkObject.h", "itkEventObject.h", and "itkSpatialObjectReader.h" in igstkMeshReader.h
  • No need for "igstkEvents.h" in igstkMeshReader.cxx

igstkTimeStamp

Luis's Comment

Now that the realtime clock in IGSTK returns the time in milliseconds, the changes that were made originally in the TimeStap, and that justified to add a modified file in the Sandbox, are no longer needed. More specifically, those changes were the division of time by 1000.0 in order to make all the time units consistent with seconds. Now all the times units are in milliseconds.

The only differences between this file and the one in the main IGSTK library are style changes.

The file can be merged without major difficulties.

WARNING this file MUST be moved simultaneously with the igstkRealTimeClock, in order to preserve consistency of time units.

David's Comments

• No changes have been made to this class in the main repository since it was moved to the sandbox. Merging will be straightforward.
• This class was originally moved to the sandbox to fix a bug, which was later determined to be a bug in igstkRealTimeClock and not in this class.

igstkRealTimeClock

Luis's Comment

Changes in this file include:

1. Bug fix for the inconsitency of time units (seconds versur milliseconds).
2. Style Changes.

The file is good to be merged.

WARNING this file MUST be moved simultaneously with the igstkTimeStamp, in order to preserve consistency of time units.