From IGSTK

Contents 1 Code in Sandbox 1.1 Classes Scheduled for Code Reviews 1.2 Example Applications Scheduled for Code Reviews 2 Reviews 2.1 igstkAnnotation2D 2.1.1 Patrick Comments 2.1.2 Luis Comments 2.1.3 Rick Comments 2.2 igstkAxesObject 2.2.1 Luis Comments 2.2.2 Andinet Comments 2.3 igstkAxesObjectRepresentation 2.3.1 Luis Comments 2.3.2 Andinet Comments 2.4 igstkFlockOfBirdsTracker 2.4.1 David Comments 2.4.2 Luis Comments 2.5 igstkFlockOfBirdsTrackerTool 2.5.1 David Comments 2.5.2 Luis Comments 2.6 igstkLandmark3DRegistrationErrorEstimator 2.6.1 David Comments 2.6.2 Luis Comments 2.7 igstkPivotCalibration 2.7.1 Julien Comments 2.7.2 Luis Comments 2.8 igstkPrincipalAxisCalibration 2.8.1 Julien Comments 2.8.2 Luis Comments 2.9 igstkUltrasoundProbeObject 2.9.1 David Comments 2.9.2 Luis Comments 2.10 igstkUltrasoundProbeObjectRepresentation 2.10.1 David Comments 2.10.2 Luis Comments 2.11 igstkBoxObject 2.11.1 Luis Comments 2.11.2 Andinet Comments 2.12 igstkBoxObjectRepresentation 2.12.1 Luis Comments 2.12.2 Andinet Comments 2.13 igstkConeObject 2.13.1 Luis Comments 2.13.2 Andinet Comments 2.13.3 Brian Comments 2.14 igstkConeObjectRepresentation 2.14.1 Luis Comments 2.14.2 Andinet Comments 2.14.3 Brian Comments 2.15 igstkView 2.15.1 Brian Comments 2.15.2 Luis Comments 2.16 igstkMouseTracker 2.16.1 Julien Comments 2.16.2 Andinet Comments 2.16.3 Luis Comments 3 Applications Reviews 3.1 HelloWorld 3.1.1 Luis Comments 3.2 OneViewAndTracking 3.2.1 Luis Comments 3.3 FourViewsAndTracking 3.3.1 Luis Comments

Code in Sandbox

Classes Scheduled for Code Reviews

All of these classes are new, and are high priority for review:

Name	Author	Reviewer 1	Reviewed	Code Fixed	Reviewer 2	Reviewed	Code Fixed	Priority	Moved
igstkAnnotation2D	Andinet	Patrick	Y	Y	Rick	N	N	High	Y
igstkAxesObject	Julien	Luis	Y	Y	Andinet	Y	Y		Y
igstkAxesObjectRepresentation	Julien	Luis	Y	Y	Andinet	Y	Y		Y
igstkFlockOfBirdsTracker	Julien	David	Y	N	Luis	Y	N		N
igstkFlockOfBirdsTrackerTool	Julien	David	Y	N	Luis	Y	N		N
igstkLandmark3DRegistrationErrorEstimator	Andinet	David	Y	Y	Luis	Y	Y		Y
igstkPivotCalibration	James	Julien	Y	Y	Luis	Y	Y	High	Y
igstkPrincipalAxisCalibration	James	Julien	Y	Y	Luis	Y	Y	High	Y
igstkUltrasoundProbeObject	Julien	David	Y	Y	Luis	Y	Y		Y
igstkUltrasoundProbeObjectRepresentation	Julien	David	Y	Y	Luis	Y	Y		Y
igstkBoxObject	Julien	Luis	Y	Y	Andinet	Y	Y		Y
igstkBoxObjectRepresentation	Julien	Luis	Y	Y	Andinet	Y	Y		Y
igstkConeObject	Julien	Luis	Y	Y	Brian	Y	Y		Y
igstkConeObjectRepresentation	Julien	Luis	Y	Y	Brian	Y	Y		Y
igstkView	Andinet	Brian	Y	Y	Luis	Y	Y	High	Y
igstkMouseTracker	Sohan	Julien	Y	Y	Andinet	Y	Y		Y

The following classes have not changed this iteration so there is no point in reviewing them again, they will not be moved out of the sandbox:

igstkFlockOfBirdsTracker and igstkFlockOfBirdsTrackerTool will be moved to iteration 8.

• igstkAtamaiNDITracker

Example Applications Scheduled for Code Reviews

Name	Reviewer 1	Reviewed	Code Fixed	Reviewer 2	Reviewed	Code Fixed	Priority	Moved
Hello World	Luis	Y	Y	Andinet	Y	Y	High	Yes
Two Views	??	N	N	??	N	N	Medium	No
Four Views	??	N	N	??	N	N	Medium	No
OneViewAndTracking	Luis	N	N	Andinet	N	N	High	Yes
FourViewAndTracking	Luis	N	N	Andinet	N	N	Low	Yes

Reviews

igstkAnnotation2D

Patrick Comments

• Non-necessary include headers

#include <vector>
#include <string>
#include "itkCommand.h"
#include "itkLogger.h"
#include "igstkMacros.h"

Comment: Fixed (AE)

• No need for igstkLoggerMacro(), it's already in igstkObject class

Comment: Macro is needed for m_logger initialisation in the annotation constructor (AE)

• Testing program does not perform the task, as being designed to

Comment: Fixed (AE)

• Lack of State Machine in this class

Comment: Fixed (AE)

• AddAnnotations(int).. maybe can be changed to SetAnnotationSize(int)? We can probably set a default size, and do

this->m_AnnotationActor[0]->SetPosition(10,10);
this->m_AnnotationActor[1]->SetPosition(vSize[0] -60 ,10);
this->m_AnnotationActor[2]->SetPosition(10, vSize[1] - 10);
this->m_AnnotationActor[3]->SetPosition(vSize[0] - 60, vSize[1] - 10); 

in the constructor.

• SetAnnotationText() is more appropriate than AddAnnotationText(); (Fixed: AE)

Luis Comments

• Missing declarations of Copy constructor and operator=() [NOTE to discuss at the meeting: Maybe we should add these two methods to the StandardTraitsMacro]
• Having methods that accept pointers is very dangerous. The method RequestAddAnnotations() has a bad name (it should probably be called RequestSetAnnotationsViewPort(). It should not receive a pointer to integers as argument. Too many things can go wrong with pointers:
  • dangling pointers
  • null pointers
  • under allocated pointers

Comment: Fixed (AE)

Rick Comments

igstkAxesObject

Luis Comments

• The brief comment wasn't separated from the main Doxygen comment (it is separated now).
• main Doxygen comment was too short. (a couple of sentences added now).
• Declarations for Copy constructor and operator= were missing in the private section. (added now).
• Typedef for the ITK GroupSpatialObject does not need to be public. (moved to private now).
• In cxx, there is no need for including "igstkEvents.h"
• m_Size was missing from PrintSelf.
• There is no test for this class Therefore code coverage is 0% !! (fixed)

Andinet Comments

The class confirms to IGSTK coding style except that it misses a test program

igstkAxesObjectRepresentation

Luis Comments

• The brief comment was not a single sentence (fixed now)
• The full comment need a bit more of details. (not fixed yet)
• Added a "see also" \sa doxygen link for the AxesObject.
• Copy constructor and assignment operator declarations were missing. (added now)
• Unecessary #includes removed
  • vtkActor
  • vtkProperty
  • vtkPolyDataMapper
• This class doesn't have a test. Code coverage is 0% !! (fixed)

Andinet Comments

• Incorrect function comment (Fixed)
• Test program is missing

igstkFlockOfBirdsTracker

David Comments

This class is still just a shell, I'm guessing that it's targeted for a future release.

Luis Comments

• The header contains constanst in the IGSTK namespace. They shouldn't be put at that level of public.
  • The #defines for constants should be replaced with static variables, and initialized in the .cxx
  • The Datasize array should be static and initialized in the .cxx
  • The constants should not use acronyms such as "I_TO_CM" (it should be "InchesToCentimeters")
• FoB should not be used as prefix in variables and types: it is an acronym.
  • The variables and types in question are in the class namespace, so they dont need further prefixes.
• Typedefs for the enums shouldn't be in the public namespace unless they are needed in the API. Many of them are only used in private methods.
  • FoBMode (now moved to private section)
  • FoBType (now moved to private section)
  • FoBPositionScaling (now moved to private section)
  • FoBUnit is not used
  • FoBHemisphere is not used
• There is no test for this class. Code coverage is 0%
• Including unecessary headers
  • <iostream> (now removed)
  • <fstream> (now removed)
  • igstkTrackerPort.h (now removed)
• Coding style of for() loops inside the switch statement is not following IGSTK indentation (even one line for() must use brackets). (fixed)

igstkFlockOfBirdsTrackerTool

David Comments

See comments for FlockOfBirdsTracker.

Luis Comments

• The API of the class looks fine, but implementation is missing.
• There is no test for this class Therefore code coverage is 0% !!

igstkLandmark3DRegistrationErrorEstimator

David Comments

• Structurally, the code looks fine
• It seems that this class could be merged into igstkLandmark3DRegistration, since it uses the same data (not critical, just a suggestion)
• The final operation in EstimateTargetRegistrationError, division by "n", should be division by "n-2". (fixed)

Luis Comments

• The type LandmarkPointContainerType is std::vector< LandmarkPointType > and it is set by copy. This may be a performance issue if there are many points in the container. I could be interesting to replace the container with an itk::VectorContainer that could be passed as a SmartPointer, or with a SimpleDataObjectDecorator around the std::vector.
• The following types do not need to be public. They are is not used in the public API.
  • VectorType
  • PointsContainerConstIterator
  • DistanceType
  • VersorType

Comments: Fixed(AE)

• #include for itkVector.h is unnecessary in the .cxx (now removed)
• Coding Style is not followed for while() loops. The brackets should be indented two characters, along with the instructions in the body of the loop.(now fixed)
• Instantiation for the SymmetricEigenAnalysis<> class in line 120 of the .cxx should use the Augmented types of the Matrix and Vector instead of restating them. (FIXED:AE)
• Coding Style fault in indentation of for() loop in line 311-316 of the .cxx (Fixed: AE)
• Good code coverage 95%.
  • BUT the missing 5% actually highlights a bug in the method ComputeLandmarkPrincipalAxed(). because it is reusing an iterator without reseting it to the begin() of the container. Line 72 of the .cxx is missing a statement such as "pointItr = m_ImageLandmarks.begin()" (Fixed:AE)
  • This also raises concerns about the effectiveness of the test... since despite of this bug the test for this class is passing

Comment (Andinet) : This is a good observation. The test instantiates landmark registration error estimator object and calculates the uncertainity of registering a sample target point. Unfortuantely, the "uncertainity" estimate of the sample target point is a statistical measure and it is not a deterministic value we can pass through a logical evaluation (PASS or FAIL)

igstkPivotCalibration

Julien Comments

The class is nicely written, some minor style fixes.

Luis Comments

• variables should be declared one per line,
  • not grouped like "int i,j,k;"
• RequestGetInputSample() method doesn't really pass through the state machine. It sets a State Machine input, but right away returns a versor and a vector independently of the State Machine action. The correct method should have an Action method (with "Processing" suffix) that will throw the values as events with payload.
• Copy constructor and operator=() missing. They should be declared private, and must not be implemented.(fixed)
• There is no need for exposing VNL types on the public API of this class. VNL includes were moved to the .cxx file. (fixed)
• "RMS" is an acronym, that should not be used in the coding style. (replaced with RootMeanSquareError).

igstkPrincipalAxisCalibration

Julien Comments

The class is nicely written, some minor style fixes.

In some places SetVnlVector() is used instead of the = operator (this is not critical so I left them in the code).

Luis Comments

• Most of the arguments that are passed by copy (vectors, versos) could be passed as const references
• Variables should be declared one per line. Not multiple variables in the same line.
• Cross product of ITK vectors returning a CovariantVector is now defined in ITK.

igstkUltrasoundProbeObject

David Comments

• code looks unfinished, will re-review when it is done (new comments explain this: the current spatial object is generic and doesn't have any adjustable geometry yet)
• class description is incorrect (describes cylinder) (this has now been fixed)

Luis Comments

• Brief comment was connected to main Doxygen comment (separated now).
• Main Doxygen comment is too short. (fixed)
• Missing declarations of Copy constructor and operator=() [NOTE to discuss at the meeting: Maybe we should add these two methods to the StandardTraitsMacro]
• typedef for ultrasoundProbeSpatialObjectType does not need to be public. It is not part of the public API, it is only used by a private method. (typedef now moved into a private section).
• There is no test for this class. Code coverage = 0% (fixed)
• The actual implementation of the class seems to be missing. The actual description of the object as a combination of SpatialObject should probably be attached to the GroupSpatialObject.
• #include for igstkEvents.h is unnecessary in the .cxx file. (now removed).

igstkUltrasoundProbeObjectRepresentation

David Comments

• Class documentation describes a cylinder, not an ultrasound probe (has been fixed)
• The VTK code for generating the shape of the probe looks interesting... polygons generated from implicit functions. Doesn't this result in a bumpy surface? It is a powerful way of combining 3D shape primitives for the purpose of visualization, but seems inefficient.

Luis Comments

• Brief Doxygen comment was connected to the detailed comment. (fixed)
• Doxygen documentation mentions a vtkUltrasoundProbeSource class... (does such a class exists ? it seems to be a typo). (fixed)
• Copy constructor and operator= are missing from the private section. They should be declared but not instantiated. (fixed)
• This class does not have a test. Code coverage is 0% (fixed)
• #include for igstkEvents.h is not needed in .cxx (fixed)

igstkBoxObject

Luis Comments

• Brief Doxygen documentation was connected to the main Doxygen documentation. (now it is separated).
• BoxSpatialObjectType declaration does not need to be public. It is not part of the public API of this class. (now moved to the private section).
• GetSize should use a GetMacro() (fixed).
• PrintSelf is missing to print out the member variable m_Size (fixed).
• Copy constructor and operator= declarations are missing. (they should be declared but not implemented) (fixed).
• Code coverage is great = 100%.

Andinet Comments

• Member function calls are not logged. (FIXED: Added debug logs for each method )

igstkBoxObjectRepresentation

Luis Comments

• Brief Doxygen documentation was connected to the main Doxygen documentation (now it is separate)
• Doxygen documentation mention a vtkBoxSource it should be vtkCubeSource (now fixed).
• Copy constructor and operator= are missing. (they should be declared but not implemented). (fixed)
• if in line 117 119 have brackets that are not indented with the body of the if. (now fixed).
• #include for igstkEvents is not necessary in the .cxx.
• Code coverage is good: 94%. The only method that is not covered is UpdateRepresentationProcessing().

Andinet Comments

• Member function calls are not logged (FIXED)

igstkConeObject

Luis Comments

• Brief Doxygen documentation was connected to the detailed documentation (now it is separate)
• typedef for ConeSpatialObjectType doesn't need to be public. It is not part of the public API.
• Set/Get Radius and Set/Get Height should use Set/Get Macros.
  • Current implementation is plenty of commented out lines. (fixed)
• Copy constructor and operator= declarations are missing. They should be declared in the private section and not be implemented. (fixed)
• Code coverage is good = 100%.

Andinet Comments

• Member function calls are not logged (FIXED)

Brian Comments

Should probably add a brief comment describing the defaults that are set for Radius and Height.

m_Radius = 1; m_Height = 1;

igstkConeObjectRepresentation

Luis Comments

• Brief Doxygen documentation was connected to main documentation. (fixed)
• Copy constructor and operator= are missing. They should be declared in the private section and should not be implemented. (fixed)
• #include for igstkEvents.h is not necessary in the .cxx file. (fixed)
• Code coverage is good = 97%. The only uncovered method is UpdateRepresentationProcessing().

Andinet Comments

• Member function calls are not logged. (FIXED: Added debug logs for each method )

Brian Comments

Didn't perceive any new problems

igstkView

Brian Comments

Should add a comment to this declaration: m_Renderer->SetBackground(0.5,0.5,0.5);

Many of the switch statements are not declared, they may be defaulted to the next case

For example:

   case FL_FOCUS:
   case FL_UNFOCUS:
   ;   // Return 1 if you want keyboard events, 0 otherwise. Yes we do
   break;

Luis Comments

• The addition of tha connection to the new annotation class follows well the encapsulation rules.
• It can not be determined if the changes work, because they are only in use in the igstkAnnotation2DTest file, and this test is failing in all the platforms. (Fixed: AE)

igstkMouseTracker

Julien Comments

Andinet Comments

Luis Comments

• The class needs to be updated for using the macros for Class Traits. (the macro has been added).
• TrackerTool and TrackerPort types do not need to be public. (now moved to the private section).
• Copy constructor and assignment operator were missing.
  • They must be declared in the private section,
  • and must not be implemented. (added now).
• Type for ResultType does not need to be public. (now made protected).
• declaration of superclass does not need the namespace igstk. (redundant namespace removed).

Applications Reviews

HelloWorld

Luis Comments

• The Fl::run() method needed to be replaced with a while loop and invokations to the PulseGenerator.
• The fluid application needs an extra method for indicating when the user has clicked on the Quit button.
• Unnecessary #include for vtkInteractorObserver was removed.
• Start adding comments for TheBook.

OneViewAndTracking

Luis Comments

• The Fl::run() method needed to be replaced with a while loop and invokations to the PulseGenerator.
• The fluid application needs an extra method for indicating when the user has clicked on the Quit button.
• Start adding comments for TheBook.

FourViewsAndTracking

Luis Comments

• The Fl::run() method needed to be replaced with a while loop and invokations to the PulseGenerator.
• The fluid application needs an extra method for indicating when the user has clicked on the Quit button.
• Orientation request for Axial, Coronal and Sagittal was missing (now added).
• ResetCamera method was missing. This must be done after adding all the actors to the render window.