Release 4 Code Reviews (Sandbox)

From IGSTK

Jump to: navigation, search

Contents

Classes Scheduled for Code Reviews

These classes are ready for immediate review.

Please fix the code when you do the review

  • SO: Sebastian Ordas
  • ZY: Ziv Yaniv
  • PC: Patrick Cheng
  • AE: Andinet Enquobahire
  • MA: Michel Audette

Core igstk components:

Name Author Reviewer 1 Reviewed Code Fixed Reviewer 2 Reviewed Code Fixed Moved
igstkReslicerPlaneSpatialObject SO AE Done PC
igstkImageResliceSpatialObjectRepresentation SO AE Done PC
igstkMeshResliceSpatialObject SO AE Done PC
igstkMeshResliceSpatialObjectRepresentation SO AE Done PC
igstkCrossHairObject SO AE Done ZY
igstkCrossHairRepresentation SO AE Done ZY
igstkToolProjectionObject SO AE Done ZY
igstkToolProjectionRepresentation SO AE Done ZY
igstkTransformBase ZY MA 01/06/08 AE 1/27/08
igstkTransform ZY MA 01/06/08 AE 1/27/08
igstkAffineTransform ZY MA 01/06/08 AE 1/27/08
igstkPerspectiveTransform ZY MA 01/06/08 AE 1/27/08
igstkPivotCalibrationAlgorithm ZY MA 01/06/08 SO
igstkAscensionCommandInterpreter SO AE PC
igstkAscensionTracker SO AE PC
igstkAscensionTrackerTool SO AE PC

Utility classes intended to make life easier for igstk users:

Name Author Reviewer 1 Reviewed Code Fixed Reviewer 2 Reviewed Code Fixed Moved
igstkTransformXMLFileReaderBase ZY MA 01/06/08 AE
igstkRigidTransformXMLFileReader ZY MA 01/06/08 AE
igstkAffineTransformXMLFileReader ZY MA 01/06/08 AE
igstkPerspectiveTransformXMLFileReader ZY MA 01/06/08 AE
igstkPrecomputedTransformData ZY MA 01/06/08 AE
igstkTransformFileReader ZY MA 01/06/08 AE
igstkAuroraTrackerConfiguration ZY PC MA 01/15/08
igstkMicronTrackerConfiguration ZY PC MA 01/15/08
igstkAscensionTrackerConfiguration ZY PC MA 01/15/08
igstkPolarisTrackerConfiguration ZY PC MA 01/15/08
igstkTrackerConfiguration ZY PC MA 01/15/08
igstkTrackerController ZY PC MA 01/15/08
igstkTrackerConfigurationXMLFileReaderBase ZY PC MA 01/23/08
igstkSerialCommunicatingTrackerConfigurationXMLFileReader ZY PC MA 01/23/08
igstkAuroraConfigurationXMLFileReader ZY PC MA 01/23/08
igstkMicronConfigurationXMLFileReader ZY PC MA 01/23/08
igstkAscensionConfigurationXMLFileReader ZY PC MA 01/23/08
igstkPolarisHybridConfigurationXMLFileReader ZY PC MA 01/23/08
igstkPolarisWirelessConfigurationXMLFileReader ZY PC MA 01/23/08
igstkPolarisSpectraConfigurationXMLFileReader ZY PC MA 01/23/08
igstkPolarisVicraConfigurationXMLFileReader ZY PC MA 01/23/08
igstkTrackerConfigurationFileReader ZY PC MA 01/23/08
igstkOIGTLinkTrackerConfigurationFileReader ZY PC MA 01/23/08
igstkOIGTLinkTrackerConfigurationXMLFileReader ZY PC MA 01/23/08

Please use the Code Review Check List

Code Review Check List

Reviews

igstkReslicerPlaneSpatialObject

Andinet's Comments

  • igstkReslicerPlaneSpatialObjectTest Fails on all machines in the dashboard --Sebastian.ordas 00:25, 24 November 2008 (UTC) fixed
  • Low code coverage ReslicerPlaneSpatialObject.cxx ( 69.32 % ) http://www.cdash.org/CDash/viewCoverage.php?buildid=215960
  • Move method implementations to implementation file
  • Make all Get methods const methds. Methods such as GetOrientationType, GetReslicingMode, GetToolTransform etc... --Sebastian.ordas 00:27, 24 November 2008 (UTC) fixed
  • Remove commented out methods such as GetReslicerPlaneNormal ( Fixed )
  • In RequestSetReslicingMode method, implement a condition for InvalidReslicingInput --Sebastian.ordas 01:16, 24 November 2008 (UTC) fixed
  • Remove redundant class forward declaration and redundant header files
    • vtkMatrix4x4, vtkPlane, vtkPlaneSource etc ( Fixed )
    • vtkMatrix4x4.h, vtkCutter ( Fixed )
  • Print method misses printing out several private member variables ( such as ImageBounds, CursorPositon, PlaneNormal, PlaneCenter etc ) --Sebastian.ordas 00:51, 24 November 2008 (UTC) fixed
  • Suggestion: Since this class is used for reslicing of image and mesh data, I suggest changing the private data member variable from m_ImageBounds to m_Bounds or m_DataBounds... Using the name m_ImageBounds would create confusion.. --Sebastian.ordas 00:55, 24 November 2008 (UTC) fixed

Patrick's Comments

igstkImageResliceSpatialObjectRepresentation

Andinet's Comments

  • Failing tests in all platforms on the dashbaord ( --Sebastian.ordas 03:05, 30 December 2008 (UTC) renamed tests to "igstkImageResliceObjectRepresentation_xxx")
    • igstkImageResliceSpatialObjectRepresentationFltkTest
    • igstkImageResliceSpatialObjectRepresentationFltTest3
    • igstkImageResliceSpatialObjectRepresentationQtTest
    • igstkImageResliceSpatialObjectRepresentationQtTest2
  • Low code coverage
    • igstkImageResliceSpatialObjectRepresentation.txx ( 80.74%)
  • Remove commented out code ( Fixed )
  • Move methods that are not needed by class users from public to private/protected interface
    • Such as GetFrameRed(), GetFrameGreen(), GetFrameBlue() --Sebastian.ordas 01:20, 24 November 2008 (UTC) fixed

Patrick's comments

igstkMeshResliceSpatialObject

Andinet's comments

  • Missing unit test ( add igstkMeshResliceSpatialObjectTest )

igstkMeshResliceSpatialObjectRepresentation

Andinet's comments

  • Missing unit test

igstkCrossHairObject

Andinet's comments

  • Missing unit test
    • Added igstkCrossHairObjectTest
    • Needs to be beefed up. All class methods need to be exercised for testing. We are shooting for 100% code coverage
  • Header file, typedef and declaration included in WorkingVolumeTester example application, but not used ( Fixed: I have removed them all )
  • Documentation missing in the header file. Explain briefly the class.
    • Added a short description. But it will benefit from more documentation as the class is not straight forward to use.
  • Print out all private data member variables in the PrintSelf method
  • Should this class be renamed as igstkCrossHairSpatialObject

igstkCrossHairRepresentation

Andinet's comments

  • Shouldn't this class be named igstkCrossHairObjectRepresentation to be consistent with the other representation classes in the toolkit? --Sebastian.ordas 03:06, 30 December 2008 (UTC) renamed
  • Convert the tolerance for difference between tool expiration time and render start time ( which is currently hard coded to 150 ) to a const variable or a class member variable

igstkToolProjectionObject

Andinet's comments

  • Missing unit test
    • Added a test but needs to be beefed up
  • Should this class be renamed as igstkToolProjectionSpatialObject (?) --Sebastian.ordas 03:07, 30 December 2008 (UTC) renamed
  • The header documentation indicates that this spatial object is representation of axes object. This description is incorrect. igstkToolSpatialObject represent a tool object which is geometrically described by vtkLineSource (Polyline) ( Documentation fixed )

igstkToolProjectionRepresentation

Andinet's comment

  • Update the class documentation indicating that the visual representation is a projection of the tool spatial object onto a reslicing plane ( Done )
  • Convert the tolerance for difference between tool expiration time and render start time ( which is currently hard coded to 250 ) to a const variable or a class member variable
  • This class should be named igstkToolProjectionObjectRepresentation for consistency. --Sebastian.ordas 03:08, 30 December 2008 (UTC) renamed


igstkTransformBase

Michel's comment

  • Copyright notice added.
  • Coverage test 86%.
  • Check list Rules currently relaxed: 1) protected constructor/destructor; 2) no = and copy; 3) typedefs generally public, apparently required by API; 4) non-const rule.

igstkTransform

Michel's comment

  • Coverage test 98%.
  • Check list Rules currently relaxed: 1) protected constructor/destructor; 2) no = and copy; 3) typedefs generally public, apparently required by API; non-const rule.
  • Found one FIXME, under bool Transform::operator==( const Transform & inputTransform ).

igstkAffineTransform

Michel's comment

  • Copyright notice added.
  • Coverage test 62%.
  • Check list Rules currently relaxed: 1) protected constructor/destructor; 2) no = and copy; 3) typedefs generally public, apparently required by API; 4) non-const rule.

igstkPerspectiveTransform

Michel's comment

  • Copyright notice added.
  • Coverage test 54%.
  • Check list Rules currently relaxed: 1) protected constructor/destructor; 2) no = and copy; 3) typedefs generally public, apparently required by API; 4) non-const rule.


igstkAscensionCommandInterpreter

Andinet's comment

  • Redundant header included
    • igstkNDIErrorEvent ( Removed )

igstkAscensionTracker

Andinet's comment

igstkAscensionTrackerTool

Andinet's comment

igstkPivotCalibrationAlgorithm

Michel's comment

  • Copyright notice, virtual destructor added.
  • Coverage test 90%.
  • Check list Rules currently relaxed: 1) protected constructor/destructor; 2) no = and copy; 3) typedefs generally public, apparently required by API; 4) non-const rule; 5) State Machine export.

igstkTransformXMLFileReaderBase

Michel's comment

  • Copyright notice & virtual destructor added, and unnecessary headers deleted.
  • Coverage test not applied here.
  • Check list Rules currently relaxed: 1) typedefs generally public, apparently required by API; 2) non-const rule.

igstkRigidTransformXMLFileReader

Michel's comment

  • Copyright notice & virtual destructor added.
  • Coverage test not applied here.
  • Check list Rules currently relaxed: 1) typedefs generally public, apparently required by API; 2) non-const rule.

igstkAffineTransformXMLFileReader

Michel's comment

  • Copyright notice & virtual destructor added.
  • Coverage test not applied here.
  • Check list Rules currently relaxed: 1) typedefs generally public, apparently required by API; 2) non-const rule.

igstkPerspectiveTransformXMLFileReader

Michel's comment

  • Copyright notice & virtual destructor added.
  • Coverage test not applied here.
  • Check list Rules currently relaxed: 1) typedefs generally public, apparently required by API; 2) non-const rule.

igstkPrecomputedTransformData

Michel's comment

  • Copyright notice & virtual destructor added.
  • Coverage test not applied here.
  • Check list Rules currently relaxed: 1) no = and copy; 2) typedefs generally public, apparently required by API; 3) non-const rule; 4) State Machine export.

igstkTransformFileReader

Michel's comment

  • Copyright notice & virtual destructor added.
  • Coverage test not applied here.
  • PrintSelf() does not print anything.
  • Check list Rules currently relaxed: 1) no = and copy; 2) typedefs generally public, apparently required by API; 3) non-const rule; 4) State Machine export

igstkAuroraTrackerConfiguration

Michel's comment

  • Modified file to include brief doxygen description.
  • Members protected only, not private.
  • Public igstkGet/SetMacro calls not explained with comments.
  • Coverage test not applied here.
  • Check list Rules currently relaxed: 1) protected constructor/destructor; 2) no = and copy;

igstkMicronTrackerConfiguration

Michel's comment

  • Modified file to include brief doxygen description.
  • Members protected only, not private.
  • Public igstkGet/SetMacro calls not explained with comments.
  • Coverage test not applied here.
  • Check list Rules currently relaxed: 1) protected constructor/destructor; 2) no = and copy;

igstkAscensionTrackerConfiguration

Michel's comment

  • Modified file to include brief doxygen description.
  • Members protected only, not private.
  • Public igstkGet/SetMacro calls not explained with comments.
  • Coverage test not applied here.

igstkPolarisTrackerConfiguration

Michel's comment

  • Modified file to include brief doxygen description.
  • Members protected only, not private.
  • Public igstkGet/SetMacro calls not explained with comments.
  • Coverage test not applied here.

igstkTrackerConfiguration

Michel's comment

  • Members protected only, not private. ( AE: the member variables are used by derived classes )
  • Coverage test not applied here.
  • Check list Rules currently relaxed: 1) protected constructor/destructor; 2) no = and copy;

igstkTrackerController

Michel's comment

  • Destructor made virtual.
  • Both .h and .cxx have a lot of header files. Why not all in .h?
  • Several functions call StateMachine.ProcessInputs() but are not a Request type method.
  • Derived from igstkObject, but no PrintSelf(). [Is such a function required here?]
  • Coverage test not applied here.
  • Check list Rules currently relaxed: 1) protected constructor/destructor; 2) no = and copy; 3) non-const rule; 4) State Machine export.

igstkTrackerConfigurationXMLFileReaderBase

Michel's comment

  • Virtual destructor added.
  • Members protected only, not private.
  • Coverage test not applied here.

igstkSerialCommunication...XMLFileReader

Michel's comment

  • Copyright notice added.
  • Members protected only, not private.
  • Modified doxygen description.
  • Coverage test not applied here.

igstkAuroraConfigurationXMLFileReader

Michel's comment

  • Copyright notice added.
  • Members protected only, not private.
  • Modified doxygen description.
  • Coverage test not applied here.

igstkMicronConfigurationXMLFileReader

Michel's comment

  • Copyright notice added.
  • Members protected only, not private.
  • Modified doxygen description.
  • Coverage test not applied here.

igstkAscensionConfigurationXMLFileReader

Michel's comment

  • Copyright notice added.
  • Members protected only, not private.
  • Function m_HaveCurrentBirdPort() labeled in variable style.
  • Modified doxygen description.
  • Coverage test not applied here.

igstkPolarisHybridConfigurationXMLFileReader

Michel's comment

  • Copyright notice added.
  • Members protected only, not private.
  • Modified doxygen description.
  • Coverage test not applied here.

igstkPolarisWirelessConfigurationXMLFileReader

Michel's comment

  • Copyright notice added.
  • Members protected only, not private.
  • Modified doxygen description.
  • Coverage test not applied here.

igstkPolarisSpectraConfigurationXMLFileReader

Michel's comment

  • Copyright notice added.
  • Modified doxygen description.
  • Coverage test not applied here.

igstkPolarisVicraConfigurationXMLFileReader

Michel's comment

  • Copyright notice added.
  • Modified doxygen description.
  • Coverage test not applied here.

igstkTrackerConfigurationFileReader

Michel's comment

  • Copyright notice added.
  • Modified doxygen description.
  • Made destructor virtual.
  • Some public methods (in sub-classes: needed?) not explained/justified.
  • Several functions call StateMachine.ProcessInputs() but are not a Request type method.
  • Derived from igstkObject- PrintSelf not found: needed?
  • Coverage test not applied here.

igstkOIGTLinkTrackerConfigurationFileReader

Michel's comment

  • Copyright notice added.
  • Modified doxygen description.
  • Several functions call StateMachine.ProcessInputs() but are not a Request type method.
  • Coverage test not applied here.

Check list Rules currently relaxed: 1) State Machine export.

igstkOIGTLinkTrackerConfigurationXMLFileReader

Michel's comment

  • Copyright notice added.
  • Members protected only, not private.
  • Modified doxygen description.
  • Made destructor virtual.
  • Some public methods not explained/justified.
  • Coverage test not applied here.

Additional comments on Trackers and File Readers

  • Mods made in keeping with KWStyle, namely length of lines limited to 81 chars.

Appendix: Michel's test grids (so far)

Name 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23
igstkTransformBase y a y y y y n1 y n2 y y y y n3 86% n5 y y y y y y y
igstkTransform y y y y y y n1 y n2 y y y y n3 98% n5 y y y y y n n6
igstkAffineTransform y a y y y y n1 y n2 y y y y n3 62% n5 y y y y y y y
igstkPerspectiveTransform y a y y y y n1 y n2 y y y y n3 54% n5 y y y y y y y
igstkPivotCalibrationAlgorithm y a y y y y n1 a n2 y y y y n3 90% n5 y y y y y y n6
igstkTransformXMLFileReaderBase y a y y y y y a y y m y y n3 n4 n5 y y y y y y y
igstkRigidTransformXMLFileReader y a y y y y y a y y y y y n3 n4 n5 y y y y y y y
igstkAffineTransformXMLFileReader y a y y y y y a y y y y y n3 n4 n5 y y y y y y y
igstkPerspectiveTransformXMLFileReader y a y y y y y a y y y y y n3 n4 n5 y y y y y y y
igstkPrecomputedTransformData y a y y y y y a n2 y y y y n3 n4 n5 y y y y y y n6
igstkTransformFileReader y a y y y y y a n2 y y y y n3 n4 n5 y n y y y y n6
igstkAuroraTrackerConfiguration y y y m y y n1 y n2 y y n n y n4 y y y y y m y y
igstkMicronTrackerConfiguration y y y m y y n1 y n2 y y n n y n4 y y y y y m y y
igstkAscensionTrackerConfiguration y y y m y y n1 y n2 y y n n y n4 y y y y y m y y
igstkPolarisTrackerConfiguration y y y m y y n1 y n2 y y n n y n4 y y y y y m y y
igstkTrackerConfiguration y y y y y y n1 y n2 y y y n y n4 y y y y y m y y
igstkTrackerController y y y y y y y n n2 y n y y y n4 y n n y y m y n6
igstkTrackerConfigurationXMLFileReaderBase y y y y y m y y y y m y n y n4 y y y y y y y y
igstkSerialCommunication...XMLFileReader y m y y y m y y y y m y n y n4 y y y y y m y y
igstkAuroraConfigurationXMLFileReader y m y y y m y y y y m y n y n4 y y y y y m y y
igstkMicronConfigurationXMLFileReader y m y y y m y y y y m y n y n4 y y y y y m y y
igstkAscensionConfigurationXMLFileReader y m y y y m y y y y m y n y n4 y y y y y m y y
igstkPolarisHybridConfigurationXMLFileReader y m y y y m y y y y m y n y n4 y y y y y y y y
igstkPolarisWirelessConfigurationXMLFileReader y m y y y m y y y y m y n y n4 y y y y y m y y
igstkPolarisSpectraConfigurationXMLFileReader y m y y y m y y y y y y y y n4 y y y y y y y y
igstkPolarisVicraConfigurationXMLFileReader y m y y y m y y y y y y y y n4 y y y y y y y y
igstkTrackerConfigurationFileReader y y y y y m y a y y m p y y n4 y n n y y m y n6
igstkOIGTLinkTrackerConfigurationFileReader y m y y y m y a y y y y n y n4 y n y y y m y n6
igstkOIGTLinkTrackerConfigurationXMLFileReader y m y y y m y y y y m p n y n4 y y y y y y y y

y: yes; n: no; m: modified by Michel; a: added; p: partially
n1: constructors and destructors public; rule relaxed for time being.
n2: no = and copy operator; rule relaxed for time being.
n3: typedefs generally public. Seem to be needed for public API.
n4: coverage test not applied to Examples directory.
n5: non-const rule relaxed for time being.
n6: export of State Machine rule relaxed for time being.

List of files in the sandbox to be moved to CVS as part of IGSTK 4.0 release

  • Classes
Name Status Remark
igstkTransformBase Moved
igstkAffineTransform Moved
igstkPerspectiveTransform Moved
igstkPivotCalibrationAlgorithm
igstkTransform Moved Modified version in IGSTK/Source
igstkSpatialObject Moved Modified version in IGSTK/Source
igstkImageSpatialObject Moved Modified version in IGSTK/Source
igstkDICOMImageReader Moved Modified version in IGSTK/Source
igstkEvents Moved Modified version in IGSTK/Source
igstkCoordinateSystem Moved Modified version in IGSTK/Source
igstkCoordinateSystemInterfaceMacros Moved Modified version in IGSTK/Source
igstkCoordinateSystemTransformToResult Moved Modified version in IGSTK/Source
  • Example applications
Name Status Remark
ApplicationCommon See the note from Ziv below. This example is decomposed into two independent examples (TrackerConfiguration, TransformReaders )
Navigator Moved
WorkingVolumeTester Moved name will be changed to TrackingVolumeViewer
Needle Biopsy Moved

Ziv's comments on ApplicationsCommon

All classes in CalibrationRegistrationLoading and TrackerConfiguration and its subdirectories should be part of this release. I believe Michel reviewed all of them. Bellow are details about things that should probably go into the release.


The details:

CalibrationRegistrationLoading (Michel reviewed) - There is a UsageExample that should be moved to the examples directory.

  • TrackerConfiguration directory

Contains many classes that should not be there (committed by our collaborator from NAMIC). The classes that we do need are (Michel reviewed):

  1. igstkTrackerController.{cxx,h}
  2. igstkTrackerConfiguration.{cxx,h}
  3. igstkPolarisTrackerConfiguration.{cxx,h}
  4. igstkMicronTrackerConfiguration.{cxx,h}
  5. igstkAuroraTrackerConfiguration.{cxx,h}
  6. igstkAscensionTrackerConfiguration.{cxx,h} or igstkMedSafeTrackerConfiguration.{cxx,h}

In 6 I am not sure which as Sebastian updated these, Sebastian?


  • TrackerConfiguration/configurationFileReaders directory

The classes that we do need are (Michel reviewed):

  1. igstkTrackerConfigurationFileReader.{cxx,h}
  2. igstkSerialCommunicatingTrackerConfigurationXMLFileReader.{cxx,h}
  3. igstkPolarisWirelessConfigurationXMLFileReader.{cxx,h}
  4. igstkPolarisVicraConfigurationXMLFileReader.{cxx,h}
  5. igstkPolarisSpectraConfigurationXMLFileReader.{cxx,h}
  6. igstkPolarisHybridConfigurationXMLFileReader.{cxx,h}
  7. igstkMicronConfigurationXMLFileReader.{cxx,h}
  8. igstkAuroraConfigurationXMLFileReader.{cxx,h}
  9. igstkAscensionConfigurationXMLFileReader.{cxx,h}

Not sure about 9, Sebastian did you update this?

There is a subdirectory containing a usage example, it will probably work but is very outdated. We could provide a nicer example where we are given an xml file and by trying to read it we find out which tracker we have...(similar to TransformOIGTLinkBroadcast example).

  • TrackerConfiguration/configurationFileReaders/oigtLinkConfigurationReaders directory*

The classes that we do need are (Michel reviewed):

  1. igstkOIGTLinkTrackerConfigurationXMLFileReader.{cxx,h}
  2. igstkOIGTLinkTrackerConfigurationFileReader.{cxx,h}

There is a subdirectory containing a usage example, but it has been superseded by the TransformOIGTLinkBroadcast example.

  • TransformOIGTLinkBroadcast*

The files that we do need are:

  1. Cmakelists.txt
  2. IGSTK-thumbnail.gif
  3. mainCommandLine.cxx
  4. mainFLTK.cxx
  5. OIGTLinkTrackingBroadcaster.{cxx,h}
  6. OIGTLinkTrackingBroadcasterGUI.fl

The file main.cxx is my old main and should be discarded. Before we do this be aware that Junichi documented that file and didn't see that I had completely changed the application structure.

The example is actually two applications. The one you get depends on the CMake configuration. If you don't have FLTK you will get a command line program, otherwise it has a simple GUI (start/stop tracking).

Retrieved from "http://public.kitware.com/IGSTKWIKI/index.php/Release_4_Code_Reviews_%28Sandbox%29"
Personal tools
TOOLBOX
LANGUAGES