Closed MichaelColonel closed 4 years ago
MichaelColonel
commented
4 years ago Problem 2: The code planNode->AddBeam(beamNode); does a lot of things:
In case of sequence node, the transformation must be generated in beams logic without using plan node, only beam node and isocenter data.
lassoan
commented
4 years ago I'm not sure if you have a question or problem that you need help with. If yes, then I would need more specific details.
MichaelColonel
commented
4 years ago I've fixed the problem 2, and now sequence node for dynamic RTBeam (not ion beam) is generally working.
The last major problem is how to modify or even check a control point data for such beam? The Beams module shows only default values when is selected a proxy beam node: no correct jaws position, no gantry angle, no patient support angle.
The MLC table proxy node data is OK, and one can see the difference in the data.
lassoan
commented
4 years ago You are making nice progress!
how to modify or even check a control point data for such beam?
I'm not sure what you mean by "check a control point". With each sequence browser node, you can get one selected item from a sequence and show it in the scene. You can enable synchronization of proxy nodes with the corresponding data node in the sequence. It means that if the proxy node is changed then the changes are automatically saved into the sequence. You can also implement functions that process sequence nodes and display results: for example, you can create a curve node from point positions stored in a sequence node to show it as a trajectory.
MichaelColonel
commented
4 years ago Correction. My previous post about modification is irrelevant.
For some reason when i add beam node to beam sequence beamSequenceNode->SetDataNodeAtValue( beamNode, std::to_string(controlPointIndex));
the node copy doesn't have modified values. It looks like that the default constructor is used instead of copy constructor, and vtkMRMLRTBeamNode::Copy method hasn't been used.
How can i check that the beam node was copied correctly?
MichaelColonel
commented
4 years ago I've done what i could. All the tests are OK. Could you check and test this pull request?
cpinter
commented
4 years ago I built and tested the branch and loading (of the Eclipse ENT dataset I used) indeed seems to work well. Although I cannot judge if the beams that are shown are valid or not, because they don't encompass the target, but it may just be the nature of the dynamic beam. @MichaelColonel can you please check this?
A bigger issue, however, is that there is a crash when I browse the beam sequence.
All I had to do is select the sequence you can see in the screenshot and drag the slider. It did change control point successfully a few times but it crashed quite soon.
The log doesn't show anything interesting. There was this error
[CRITICAL][Stream] 02.06.2020 13:19:28 [] (unknown:0) - W: BeamDoseVerificationControlPointSequence (300a,008c) absent in ReferencedDoseReferenceSequence (type 1)
hundreds of times (which should be fixed), then this one seven times
[ERROR][VTK] 02.06.2020 13:19:43 [vtkTransformPolyDataFilter (00000234F49D9550)] (E:\e\ScR\VTK\Filters\General\vtkTransformPolyDataFilter.cxx:85) - No input data
and the log ends right there.
MichaelColonel
commented
4 years ago As an experiment could you select only first three sequence nodes in playback, and do not use the ScanSpot data which is for IonBeamNode anyway. The screenshot below. Will the program crash in that case?
Under linux there is no crash neither playback nor manual selection of the of node in the sequence.
cpinter
commented
4 years ago I tried this. It took longer to crash, but finally it did after replaying the sequence the third time. If you cannot reproduce it, then I could debug into it on my computer.
MichaelColonel
commented
4 years ago Two commits that set one sequence node for beam, one sequence node for transform and one sequence node for table data (MLC data for RTBeam, ScanSpot data for RTIonBeam).
cpinter
commented
4 years ago Thanks @MichaelColonel ! Have you had a chance to investigate the crash? I did a new build but it still crashes right after moving the slider.
MichaelColonel
commented
4 years ago I have tested almost all SlicerRtData under linux, not a single crash. There are couple of major bugs that must be fixed (for some reason RTBeamNode can't be copied into sequence node, only default constructor), but everything else works quite right. I can't tested it under windows since i don't have a VisualStudio.
cpinter
commented
4 years ago I was going to debug into this, but unfortunately my debug builds don't work (https://github.com/Slicer/Slicer/issues/4991)
cpinter
commented
4 years ago Now the debug builds work, so I was able to try this branch in debug mode in the hope to catch the crash. Unfortunately, there is no crash in debug mode. I tried again in release mode, but the crash is still there. Given that most of our users use Windows, it would be important not to merge something known to cause crashes. Debug/Release discrepancies are sometimes caused by uninitialized variables (which I have seen many in the new PR). Maybe you should start there.
Any suggestions or thoughts about this @lassoan or @Sunderlandkyl ? I would like to be able to merge this soon, because with time this PR gets stale, and also @MichaelColonel keeps working and he should be able to do that on top of the master.
cpinter
commented
4 years ago I did manage to get a call stack, however it's not obvious where is the issue:
MichaelColonel
commented
4 years ago When the loaded beam node is added to sequence node, it couldn't be copied for some reason. Members of the proxy node (e.g. X1Jaw, X2Jaw, Y1Jaw, Y2Jaw, SAD, etc) have default values, not the values of the original beam node. I think it's one of the reasons why it crashes!
https://github.com/SlicerRt/SlicerRT/pull/147/files#diff-fbeee7fdba9b07188d70b50a9dad6bf7R1238-R1242
// Add beam to beam sequence node
if (beamNode)
{
beamSequenceNode->SetDataNodeAtValue( beamNode, std::to_string(controlPointIndex));
}
cpinter
commented
4 years ago @cpinter this issue has been fixed more than a week ago. You just need to remove python library cmake variables and run a top-level build (or rebuild everything from scratch).
@lassoan This comment was two weeks old...
lassoan
commented
4 years ago @lassoan This comment was two weeks old...
Yes, I realized that and immediately deleted the comment, but apparently not quickly enough.
This kind of crash is typically due to running some filters or locators on an empty input mesh.
Since Sequences module has been merged into Slicer core, there is not separate node sequencer class that takes care of copying nodes to/from internal sequence scenes, but MRML node API has been updated with a new "CopyContent" method. You can see how a number of nodes were updated to this new API here: https://github.com/Slicer/Slicer/commit/f88c110430ac818329b2806dbe3e42eeb9e0b503. You need to implement CopyContent for all classes that you want to put in a sequence.
MichaelColonel
commented
4 years ago I have added a CopyContent method to vtkMRMLRTBeamNode and vtkMRMLRTIonBeamNode, and minor change in vtkMRMLRTBeamNode::CreateBeamPolyData method so the beam poly data was always created.
@cpinter can you test this PR with a latest commit, will it crash or not?
cpinter
commented
4 years ago Unfortunately it still crashes :(
MichaelColonel
commented
4 years ago Final attempt with the latest commit with some debug output.
If it still crashes, then i don't know what to do in that case. I can't find neither empty input mesh nor uninitiated value that bugged and cause the crash.
cpinter
commented
4 years ago The crash is gone!
If everyone agrees (@lassoan and @Sunderlandkyl) then let's merge this one.
@MichaelColonel can you please confirm one more time that the tests pass with the master rebased onto this branch? Then squash the commits into one, and then I'll rebase and merge.
Again, thanks a lot!
MichaelColonel
commented
4 years ago There are some failed tests that i can't understand:
1:qSlicerBeamsModuleGenericTest 2:qSlicerBeamsModuleWidgetGenericTest 17:qSlicerDoseVolumeHistogramModuleWidgetGenericTest
These are auto generated i suppose? It's difficult to say why these tests are failed.
cpinter
commented
4 years ago Yes these are generic tests that may fail due to too wide minimum GUI size. I'd say go ahead with integration.
cpinter
commented
4 years ago In order for that to happen please squash commits and rebase to master. Thanks!
MichaelColonel
commented
4 years ago Tests 1 and 2 have strange output:
Start testing: Jul 02 15:37 MSK
----------------------------------------------------------
1/70 Testing: qSlicerBeamsModuleGenericTest
1/70 Test: qSlicerBeamsModuleGenericTest
Command: "/tmp/Slicer/Slicer-SuperBuild-Release/Slicer-build/Slicer" "--launcher-additional-settings" "/tmp/SlicerRT/SlicerRT-Release/AdditionalLauncherSettings.ini" "--launch" "/tmp/SlicerRT/SlicerRT-Release/bin/qSlicerBeamsModuleGenericCxxTests" "qSlicerBeamsModuleGenericTest"
Directory: /tmp/SlicerRT/SlicerRT-Release/Beams
"qSlicerBeamsModuleGenericTest" start time: Jul 02 15:37 MSK
Output:
----------------------------------------------------------
Failed to read launcher settings /tmp/Slicer/Slicer-SuperBuild-Release/Slicer-build/qSlicerBeamsModuleGenericTestLauncherSettings.ini
Number of registered modules: 151
Number of instantiated modules: 151
The module "Beams" has not been registered.
The following modules have been registered: ("ACPCTransform", "AddManyMarkupsFiducialTest", "AddScalarVolumes", "Annotations", "AtlasTests", "BRAINSDWICleanup", "BRAINSFit", "BRAINSFitRigidRegistrationCrashIssue4139", "BRAINSLabelStats", "BRAINSROIAuto", "BRAINSResample", "BRAINSResize", "BRAINSStripRotation", "BRAINSTransformConvert", "BordersOut", "CLI4Test", "CLIROITest", "Cameras", "CastScalarVolume", "Charting", "CheckerBoardFilter", "Cleaner", "Colors", "ColorsScalarBarSelfTest", "CompareVolumes", "Connectivity", "CreateDICOMSeries", "CropVolume", "CropVolumeSelfTest", "CropVolumeSequence", "CurvatureAnisotropicDiffusion", "DICOM", "DICOMPatcher", "DICOMScalarVolumePlugin", "DICOMSlicerDataBundlePlugin", "DICOMVolumeSequencePlugin", "DMRIInstall", "DWIConvert", "Data", "DataProbe", "DataStore", "Decimation", "DiffusionTensorTest", "DoubleArrays", "DynamicModeler", "Editor", "Endoscopy", "EventBroker", "ExecutionModelTour", "ExpertAutomatedRegistration", "ExtensionWizard", "ExtractSkeleton", "FiducialLayoutSwitchBug1914", "FiducialRegistration", "FillHoles", "GaussianBlurImageFilter", "GradientAnisotropicDiffusion", "GrayscaleFillHoleImageFilter", "GrayscaleGrindPeakImageFilter", "GrayscaleModelMaker", "HistogramMatching", "ImageLabelCombine", "JRC2013Vis", "LabelMapSmoothing", "LabelStatistics", "LandmarkRegistration", "MC2Origin", "Markups", "MarkupsInCompareViewersSelfTest", "MarkupsInViewsSelfTest", "MarkupsWidgetsSelfTest", "MaskScalarVolume", "MedianImageFilter", "MergeModels", "Mirror", "ModelMaker", "ModelToLabelMap", "Models", "MultiVolumeExplorer", "MultiVolumeImporter", "MultiVolumeImporterPlugin", "MultiplyScalarVolumes", "N4ITKBiasFieldCorrection", "NeurosurgicalPlanningTutorialMarkupsSelfTest", "Normals", "OrientScalarVolume", "PETStandardUptakeValueComputation", "PerformMetricTest", "PerformanceTests", "Plots", "PlotsSelfTest", "ProbeVolumeWithModel", "PyCLI4Test", "RSNA2012ProstateDemo", "RSNAQuantTutorial", "RSNAVisTutorial", "Reformat", "ResampleDTIVolume", "ResampleScalarVectorDWIVolume", "ResampleScalarVolume", "RobustStatisticsSegmenter", "SampleData", "ScenePerformance", "SceneViews", "ScreenCapture", "SegmentEditor", "SegmentStatistics", "Segmentations", "SelfTests", "Sequences", "ShaderProperties", "SimpleFilters", "SimpleRegionGrowingSegmentation", "SliceLinkLogic", "Slicer4Minute", "SlicerBoundsTest", "SlicerMRBMultipleSaveRestoreLoopTest", "SlicerMRBMultipleSaveRestoreTest", "SlicerMRBSaveRestoreCheckPathsTest", "SlicerOrientationSelectorTest", "SlicerTransformInteractionTest1", "Smoothing", "SubjectHierarchy", "SubjectHierarchyCorePluginsSelfTest", "SubjectHierarchyGenericSelfTest", "SubtractScalarVolumes", "SurfaceToolbox", "Tables", "TablesSelfTest", "Terminologies", "TestGridTransformRegistration", "Texts", "ThresholdScalarVolume", "ThresholdThreadingTest", "Transforms", "Units", "UtilTest", "VectorToScalarVolume", "ViewControllers", "ViewControllersSliceInterpolationBug1926", "VolumeRendering", "VolumeRenderingSceneClose", "Volumes", "VotingBinaryHoleFillingImageFilter", "WebEngine", "Welcome", "relaxPolygons", "scaleMesh", "sceneImport2428", "slicerCloseCrashBug2590", "translateMesh")
error: [/tmp/SlicerRT/SlicerRT-Release/bin/qSlicerBeamsModuleGenericCxxTests] exit abnormally - Report the problem.
<end of output>
Test time = 52.78 sec
----------------------------------------------------------
Test Failed.
"qSlicerBeamsModuleGenericTest" end time: Jul 02 15:38 MSK
"qSlicerBeamsModuleGenericTest" time elapsed: 00:00:52
----------------------------------------------------------
Especially the line The module "Beams" has not been registered.!
cpinter
commented
4 years ago There are strange issues around the generic tests. I think I have seen this one too before, but couldn't determine its source. Since the Beams module is obviously loaded fine, I'd say we ignore these for now and go ahead with integration. I may look at this later.
MichaelColonel
commented
4 years ago Commits are squashed.
Final failed tests: 2 - qSlicerBeamsModuleWidgetGenericTest (Failed) 17 - qSlicerDoseVolumeHistogramModuleWidgetGenericTest (Failed) 55 - py_nomainwindow_PlmProtonDoseEngineTest (Failed)
2, 7 besause of GUI size (widget has a minimum size hint width of 555px) 55 because of SAD value
cpinter
commented
4 years ago Thanks! There are conflicts. Have you rebased on the master?
MichaelColonel
commented
4 years ago Now should be fine.
cpinter
commented
4 years ago Merged! Thanks for bearing with us here! I'll now do a clean build with the master and check if everything's alright. Hopefully yes, but in any case now we can follow up in individual tickets.
WIP:
There are several problems:
If number of beams is more than one i have an output is console:
vtkMRMLHierarchyNode::SetIndexInParent() index 1, outside the range 0-0vtkMRMLHierarchyNode::SetIndexInParent() index 2, outside the range 0-0vtkMRMLHierarchyNode::SetIndexInParent() index 3, outside the range 0-0vtkMRMLHierarchyNode::SetIndexInParent() index 4, outside the range 0-0vtkMRMLHierarchyNode::SetIndexInParent() index 5, outside the range 0-0I don't know how to add beam proxy node to plan node, for correct isocenter position. Now transformation node is in wrong position.