search

Slicer / ExtensionsIndex

Slicer extensions index
https://github.com/Slicer/ExtensionsIndex/#slicer-extensions-index
66 stars 244 forks source link

SlicerRT - Add extension #34

Closed lassoan closed 12 years ago

lassoan commented 12 years ago

See http://www.slicer.org/slicerWiki/index.php/Documentation/4.1/Extensions/SlicerRT

jcfr commented 12 years ago

Just tried to do an experimental and building the extension fails with the following error:

[...]
-- Setting MIDAS_PACKAGE_URL ...................: http://slicer.kitware.com/midas3/api
-- Setting MIDAS_PACKAGE_EMAIL .................: jchris.fillionr@kitware.com
-- Setting MIDAS_PACKAGE_API_KEY ...............: B7FZbXp2h9oZKuwl2GeWcooZx0ltnu2OsxEtkUms
-- Setting EXTENSION_HOMEPAGE ..................: https://www.assembla.com/spaces/slicerrt
-- Setting EXTENSION_CATEGORY ..................: Radiotherapy
-- Setting EXTENSION_ICONURL ...................: http://wiki.slicer.org/slicerWiki/images/f/f2/SlicerRtExtensionLogo.png
-- Setting EXTENSION_CONTRIBUTORS ..............: Csaba Pinter (PerkLab, Queen's University), Andras Lasso (PerkLab, Queen' [...]
-- Setting EXTENSION_DESCRIPTION ...............: Extensions for radiotherapy research (DICOM-RT import, dose volume histog [...]
-- Setting EXTENSION_SCREENSHOTURLS ............: https://www.assembla.com/spaces/slicerrt/documents/bhB--4vSSr4BSNacwqjQWU [...]
-- Setting EXTENSION_DEPENDS ...................: NA
-- Setting EXTENSION_LICENSE_FILE ..............: /home/jchris/Projects/Slicer4/License.txt
-- Setting EXTENSION_README_FILE ...............: /home/jchris/Projects/Slicer4/README.txt
-- Found Git: /usr/bin/git 
-- Found Subversion: /usr/bin/svn 
-- Configuring Qt loadable module: DicomRtImport [qSlicerDicomRtImportModuleExport.h]
-- Configuring Qt loadable module: DoseAccumulation [qSlicerDoseAccumulationModuleExport.h]
-- Configuring Qt loadable module: DoseVolumeHistogram [qSlicerDoseVolumeHistogramModuleExport.h]
CMake Error at CMakeLists.txt:27 (add_subdirectory):
  add_subdirectory given source "IsoDose" which is not an existing directory.

See http://slicer.cdash.org/viewConfigure.php?buildid=29608

lassoan commented 12 years ago

Thanks for testing it. There was a case error in the directory name, fixed now (in r196).

jcfr commented 12 years ago

There are now errors specific to unix-like system: http://slicer.cdash.org/viewBuildError.php?buildid=29623

/.../SlicerRT/DoseVolumeHistogram/Logic/vtkSlicerDoseVolumeHistogramLogic.h:56: error: ‘stricmp’ was not declared in this scope
/.../SlicerRT/DoseVolumeHistogram/Logic/vtkSlicerDoseVolumeHistogramLogic.cxx: In member function ‘void vtkSlicerDoseVolumeHistogramLogic::RefreshDvhDoubleArrayNodesFromScene()’:

For reference, here is the email I sent few months ago:

Email of May 29 2012

Everything looks fine within the different CMakeLists.

Few remarks, for sake of consistency: 1) Could you move the Data folder outside of the Testing folder ? 2) Could you rename the variable has it's done here: http://viewvc.slicer.org/viewvc.cgi/Slicer4?view=revision&revision=20260

Trying to build on Ubuntu (10.04, g++ 4.4.3), I got the following errors / warnings:

Remark 1

1) /home/jchris/Projects/DoseVolumeHistogram/Logic/vtkSlicerDoseVolumeHistogramLogic.cxx:161: error: ‘stricmp’ was not declared in this scope

...

If you are doing some case insensitive comparison, may be you could just specify your identifiers (DVH_TYPE_ATTRIBUTE_VALUE, ...) in upper case and always convert to upper case before comparing to it. That way you could use "strcmp"

Remark 2

2) Warnings in qSlicerDoseVolumeHistogramModule.ui

Generating ui_qSlicerDoseVolumeHistogramModule.h /home/jchris/Projects/DoseVolumeHistogram/Resources/UI/qSlicerDoseVolumeHistogramModule.ui: Warning: Z-order assignment: '' is not a valid widget. /home/jchris/Projects/DoseVolumeHistogram/Resources/UI/qSlicerDoseVolumeHistogramModule.ui: Warning: Z-order assignment: '' is not a valid widget.

Remark 3

3) line 59 of qSlicerDoseVolumeHistogramModuleWidget.cxx, you are using std::map and std::pair to keep track of QCheckBox and other information (see ChartCheckboxToStructureSetNameMap). You could probably use a QSignalMapper instead. Using QHash, QPair etc .. could also make manipulation easier. I would recommend not to mix style .. Qt container with Qt object. You could then leverage the "foreach" macro that would simplify your code.

When using template, having ">>" is incorrect with gcc ... you should add space and write "> >"

/home/jchris/Projects/DoseVolumeHistogram/qSlicerDoseVolumeHistogramModuleWidget.cxx:59: warning: ‘>>’ operator will be treated as two right angle brackets in C++0x [-Wc++0x-compat] /home/jchris/Projects/DoseVolumeHistogram/qSlicerDoseVolumeHistogramModuleWidget.cxx:59: warning: suggest parentheses around ‘>>’ expression [-Wc++0x-compat]

Remark 4

4) qSlicerDoseVolumeHistogramModuleWidget.cxx - line 814:

There is no need to dynamically allocate QString !

QString are using implicit sharing under the hood and you should not manage memory yourself. It makes the code more complex and introduce potential leaks.

See http://doc.qt.nokia.com/4.7-snapshot/implicit-sharing.html

Remark 5

5) In Qt code, you probably leverage function like QString::compare passing the Qt::CaseSensitivity

See line 200 of qSlicerDoseVolumeHistogramModuleWidget.cxx - It would then prevent the usage of non portable function like "stricmp"

After fixing this issue, I was able to compile and run the test ... see details below.

That said, I am not sure to understand the problem. Assuming I can the test using ctest .. the extension build system should be able to do to.

$ svn checkout https://subversion.assembla.com/svn/slicerrt/trunk/SlicerRt/src/DoseVolumeHistogram/
$ mkdir DoseVolumeHistogram-build
$ cd DoseVolumeHistogram-build
$ cmake -DSlicer_DIR:PATH=/home/jchris/Projects/Slicer4-Superbuild-Debug/Slicer-build/ ../DoseVolumeHistogram
$ make -j4
// Fix error reported above
$ make -j4
$ ctest -N
Test project /home/jchris/Projects/DoseVolumeHistogram-build
  Test #1: qSlicerDoseVolumeHistogramModuleGenericTest
  Test #2: qSlicerDoseVolumeHistogramModuleWidgetGenericTest
  Test #3: vtkSlicerDoseVolumeHistogramLogicTest_EclipseProstate
  Test #4: vtkSlicerDoseVolumeHistogramLogicTest_EclipseProstate_CompareMetrics
  Test #5: vtkSlicerDoseVolumeHistogramLogicTest_EclipseProstate_CERR
  Test #6: vtkSlicerDoseVolumeHistogramLogicTest_EclipseEnt_CERR
  Test #7: vtkSlicerDoseVolumeHistogramLogicTest_EclipseBreast_CERR
$ ctest -R
Test project /home/jchris/Projects/DoseVolumeHistogram-build
    Start 1: qSlicerDoseVolumeHistogramModuleGenericTest
1/7 Test #1: qSlicerDoseVolumeHistogramModuleGenericTest ............................   Passed    7.46 sec
    Start 2: qSlicerDoseVolumeHistogramModuleWidgetGenericTest
2/7 Test #2: qSlicerDoseVolumeHistogramModuleWidgetGenericTest ......................***Failed    2.86 sec
    Start 3: vtkSlicerDoseVolumeHistogramLogicTest_EclipseProstate
3/7 Test #3: vtkSlicerDoseVolumeHistogramLogicTest_EclipseProstate ..................   Passed   12.34 sec
    Start 4: vtkSlicerDoseVolumeHistogramLogicTest_EclipseProstate_CompareMetrics
4/7 Test #4: vtkSlicerDoseVolumeHistogramLogicTest_EclipseProstate_CompareMetrics ...***Failed    0.00 sec
    Start 5: vtkSlicerDoseVolumeHistogramLogicTest_EclipseProstate_CERR
5/7 Test #5: vtkSlicerDoseVolumeHistogramLogicTest_EclipseProstate_CERR .............   Passed   11.67 sec
    Start 6: vtkSlicerDoseVolumeHistogramLogicTest_EclipseEnt_CERR
6/7 Test #6: vtkSlicerDoseVolumeHistogramLogicTest_EclipseEnt_CERR ..................***Failed  Error regular expression found in output. Regex=[Warning] 14.28 sec
    Start 7: vtkSlicerDoseVolumeHistogramLogicTest_EclipseBreast_CERR
7/7 Test #7: vtkSlicerDoseVolumeHistogramLogicTest_EclipseBreast_CERR ...............***Failed  Error regular expression found in output. Regex=[Warning] 10.14 sec

43% tests passed, 4 tests failed out of 7

Label Time Summary:
qSlicerDoseVolumeHistogramModule    =  10.32 sec

Total Test time (real) =  58.76 sec

The following tests FAILED:
      2 - qSlicerDoseVolumeHistogramModuleWidgetGenericTest (Failed)
      4 - vtkSlicerDoseVolumeHistogramLogicTest_EclipseProstate_CompareMetrics (Failed)
      6 - vtkSlicerDoseVolumeHistogramLogicTest_EclipseEnt_CERR (Failed)
      7 - vtkSlicerDoseVolumeHistogramLogicTest_EclipseBreast_CERR (Failed)
Errors while running CTest

Hth Jc

lassoan commented 12 years ago

Thanks for the further testing. I was not aware that your earlier findings have not yet been fixed. I've fixed most of them (r198) and entered a ticket for a few remaining ones. See details below.

A Slicer build on linux is in progress but it will take a few more hours, so I could not test the changes on linux. If you have time it would be great if you could start another linux build for the extension and let me know about potential remaining errors.

Thanks a lot in advance. Andras

1) Could you move the Data folder outside of the Testing folder ? => We'll do a reorganization of data folders later (https://www.assembla.com/spaces/slicerrt/tickets/107-fix-jc-s-code-review-findings)

2) Could you rename the variable has it's done here: http://viewvc.slicer.org/viewvc.cgi/Slicer4?view=revision&revision=20260 => Could you please clarify this?

Remark 1 => fixed

Remark 2 => could not reproduce on Windows; Linux build is in progress (https://www.assembla.com/spaces/slicerrt/tickets/107-fix-jc-s-code-review-findings)

Remark 3 => space between > > fixed => not using STL in QT classes - will review later (https://www.assembla.com/spaces/slicerrt/tickets/107-fix-jc-s-code-review-findings)

Remark 4 => fixed

Remark 5 => fixed

jcfr commented 12 years ago

One build error left happening on linux - See http://slicer.cdash.org/viewBuildError.php?buildid=29663

/.../SlicerRT/DoseVolumeHistogram/Testing/Cxx/vtkSlicerDoseVolumeHistogramLogicTest1.cxx:161:
 error: ‘stricmp’ was not declared in this scope
jcfr commented 12 years ago

To answer your question about the naming of variable and the content of CMakeLists.txt

  1. In top level CMakeLists.txt, calling include(SlicerEnableExtensionTesting) is not needed anymore
  2. For sake of consistency, variable names, indent, comments .. in the module CMakeLists.txt could be updated to be similar to what has been done here

EDIT: Fixing 1 would be nice, Fixing 2 is definitively optional for now and won't prevent the integration of the extension into the index.

lassoan commented 12 years ago

Fixed in r200.

EDIT by @jcfr: It now compiles and all tests passes on Linux. Feel free to re-push the topic with an updated SVN revision. Would be nice if you could remove the extra include(SlicerEnableExtensionTesting). As soon as this is done, I will integrate it into the index and it should be then be built by the factory :)

jcfr commented 12 years ago

I squashed the commit of topic 34-add-SlicerRT and integrated it into master. When you will have a chance to address the lower priority stylistic issue discussed above, create an other issue on the extension index tracker, and reference it with a new topic.

jcfr commented 12 years ago

The continuous build successfully build and packaged the extension on windows 32bit and macOSX. See http://slicer.cdash.org/index.php?project=Slicer4&display=project&filtercount=1&showfilters=1&field1=buildname/string&compare1=65&value1=20655-SlicerRT-svn200&collapse=0

The linux didn't work because the nightly build of last night failed due to a different issue (See slicer issue http://www.na-mic.org/Bug/view.php?id=1961)

On the other hand, the Windows 64bit should have been triggered. Just created an issue to keep track of it. See http://www.na-mic.org/Bug/view.php?id=2338

lassoan commented 12 years ago
  1. In top level CMakeLists.txt, calling include(SlicerEnableExtensionTesting) is not needed anymore
  2. For sake of consistency, variable names, indent, comments .. in the module CMakeLists.txt could be updated to be similar to what has been done here

Thanks for the suggestions, fixed both.