Closed MichaelColonel closed 3 years ago
Thanks for working on this!
I cannot review the code in depth but after a quick glance it looks alright to me and I'm OK with merging it if all the tests pass and things look good in general.
One thing that I would like to see though is documentation. This is a lot of change, and there is no description from which one can understand how these modules work.
@Sunderlandkyl do you have any observations, etc about this PR?
I will add more explicit description soon. Basic idea is the CLI module handles all the calculation and image processing. The loadable module has a little bit better GUI and shows imager boundary, normal and view up vectors and some points as a markups in 3D.
I will let you know when it be ready for merging.
Failed tests after adding drr modules:
1 - qSlicerBeamsModuleGenericTest (Failed)
2 - qSlicerBeamsModuleWidgetGenericTest (Failed)
17 - qSlicerDoseVolumeHistogramModuleWidgetGenericTest (Failed)
39 - vtkSlicerSegmentComparisonModuleLogicTest_EclipseProstate_Transformed (Failed)
47 - py_nomainwindow_PlmProtonDoseEngineTest (Failed)
These definitely should not fail: 39 - vtkSlicerSegmentComparisonModuleLogicTest_EclipseProstate_Transformed (Failed) 47 - py_nomainwindow_PlmProtonDoseEngineTest (Failed)
By the way what Slicer version do you use for this work?
4.13.0-2020-10-05 r29411 / ff8bc2b
Just asking because SlicerRT apparently doesn't build on the factory machines for weeks now. See the dashboard for the day you mention: http://slicer.cdash.org/index.php?project=SlicerPreview&date=2020-10-05&filtercount=1&showfilters=1&filtercombine=or&field1=label&compare1=63&value1=SlicerRT
I'm using VTK8.
py_nomainwindow_PlmProtonDoseEngineTest failed because of different values of SAD in test (old issue since beam sequence node have been added).
vtkSlicerSegmentComparisonModuleLogicTest_EclipseProstate_Transformed failed test is something new.
It looks like vtkSlicerSegmentComparisonModuleLogicTest_EclipseProstate_Transformed is failing on the VTK8 build, along with some other tests: http://slicer.cdash.org/testDetails.php?test=10179043&build=2036597
I'll take a look at it today.
- This new module needs a documentation page. Right now this is where the SlicerRT documentation is: https://www.slicer.org/wiki/Documentation/Nightly/Extensions/SlicerRT . @lassoan do you think the new module documentation should go here, or do you have another suggestion? (thinking about the big migration work in general to rst)
Now the recommendation is to use markdown format and store the documentation in the same repository as the source code.
To not increase the burden of migrating documentation from wiki to github, it makes sense to add new documentation on github. For now, you could just dump all new pieces of documentation in the top-level readme file (change the extension to .md). The content can later be split into several pages.
A digitally reconstructed radiograph (DRR) is a synthetic radiograph which can be generated from a computed tomography (CT) scan. It is used as a reference image for verifying the correct setup position of a patient prior to radiation treatment.
The process of DRR calculation consists of two modules: command line interface module and graphical user interface loadable module.
CLI module "slicer_plastimatch_drr" takes a CT image as input, and generates one DRR image according to the parameters of detector orientation, source distance, isocenter position and image resolution and spacing. Most of these parameters are based on plastimatch drr ones.
Loadable GUI module "DRR Image Computation" uses CLI module's logic and nodes data to calculate and visualize DRR image. It also shows basic detector elements such as: detector boundary, detector normal vector, detector view-up vector, detector image origin (0,0) pixel, image subwindow boundary as markups data on a slice and 3D views.
Markups data is only for WYSIWYG purpose.
Arguments for plastimatch drr program are generated using loadable module parameters for testing and debugging purposes.
Initial documentation for DRR computation modules.
Looks great, thank you!
Can you please make this an md file and add it to your commit? Similarly to how it is done for Slicer: https://github.com/Slicer/Slicer/tree/master/Docs/user_guide/modules
@lassoan Correct me please if you have a different idea
Do you mean on top level, along with readme.txt?
Similarly to how it is in Slicer, I'd put it in Docs/user_guide/modules
. We'll take care of the rest when migrating the documentation.
Thanks a lot! It look s good to me. Ideally, the images should be uploaded in a more persistent way (as I see the md file uses the images commented here in the PR), but as far as I'm concerned it looks good.
@lassoan @Sunderlandkyl if you agree, then we can integrate this.
As i understand the markdown description, a directory can be used as an image storage.
![GitHub Logo](/images/logo.png)
. It can add image either in the same directory or in some relative path, for example [images/drr_cli_module.png]
, [images/drr_loadable_module.png]
.
Yes, exactly. However I am also not sure about uploading doc images into the repo, because they will take lots of space after a while. I'd wait for @lassoan 's opinion
If documentation images don't add more than a 5-10MB to the repository size (in the total lifetime of the project, including updated versions of the images) then they can be stored in git.
If you need more storage space then it is better to store them as assets in a release (see for example in Slicer repository: https://github.com/Slicer/Slicer/releases/tag/docs-resources). Test data can be also stored as release assets (see for example here: https://github.com/lassoan/SlicerOrthodonticAnalysis/releases/tag/TestingData).
It's not yes ready to merge. I'm trying to fix a crash when previous scene is closed and a new data are loaded.
Most of the bugs are fixed, and i think it's ready to merge. Tests are the same.
Thanks a lot for the contribution! We can fix arising problems in subsequent issues/commits.
I already have a question: why did you change the Plastimatch repo to your own fork?
I've made a PR in plastimatch repository for proper support of DRR computation in SlicerRT. Plastimatch reconstuct library and drr_compute function are unavailable in SlicerRT without it. I will return original plastimatch repo as soon as this PR will be merged.
Excellent, thanks! @gregsharp have you had a chance to take a look at the mentioned PR?
Assets for a new PR
Modules compute a digitally reconstructed radiograph (DRR) from a computed tomography (CT) scan. It is used as a reference image for verifying the correct setup position of a patient prior to radiation treatment. Modules support only single image mode, normal RT image plane orientation, and zero x-ray image receptor angle.