Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for multi-leaf collimators #115

Open
cpinter opened this issue Aug 8, 2019 · 39 comments
Open

Add support for multi-leaf collimators #115

cpinter opened this issue Aug 8, 2019 · 39 comments

Comments

@cpinter
Copy link
Member

cpinter commented Aug 8, 2019

@cpinter cpinter added this to the External Beam Planning milestone Aug 8, 2019
@cpinter
Copy link
Member Author

cpinter commented Oct 23, 2019

@MichaelColonel This is where the MLC related work should be tracked.

So what exists in SlicerRT right now is a way to set the leaf positions
https://github.com/SlicerRt/SlicerRT/blob/master/Beams/MRML/vtkMRMLRTBeamNode.h#L101-L105
However it uses double array nodes, and I think it would be much better using table nodes (vtkMRMLTableNode). Do you know how are MLC positions stored in DICOM?

There is also some code that generates the beam model from the MLC positions, see
https://github.com/SlicerRt/SlicerRT/blob/master/Beams/MRML/vtkMRMLRTBeamNode.cxx#L483-L574
I think this should be OK, just need to change all to use table nodes, and add importing MLCs from DICOM (or whatever feature you need around MLCs)
(https://github.com/SlicerRt/SlicerRT/blob/master/DicomRtImportExport/Logic/vtkSlicerDicomRtReader.cxx#L637-L640)

@gregsharp
Copy link
Member

MLC positions are stored as a sequence of control points. MLC positions may change between control points. Each control point also specifies the gantry angle, collimator angle, jaw positions, and cumulative MUs (and several other things as well).

The plan also contains information about the number of MLC leaves and the widths of those leaves. However, it does not contain all needed information, for example leaf position limits.

@cpinter
Copy link
Member Author

cpinter commented Oct 23, 2019

Thanks, @gregsharp ! What is the format of the MLC positions? Just a number for each leaf for each control point?

@gregsharp
Copy link
Member

Correct. A single number for each leaf position at each control point.

Regarding using a vtkMRMLTableNode, that would only be for the "Current leaf position"? Or do you envision a sequence of vtkMRMLTableNode for multiple control points?

@cpinter
Copy link
Member Author

cpinter commented Oct 23, 2019

You're probably right and we should put all this in a sequence.

@lassoan
Copy link
Member

lassoan commented Oct 23, 2019

Storing in a sequence node would have the advantage that you would get browsing between time points for free. You could add any other nodes (markup fiducials, transforms, etc) to change in sync.

Potential inconvenience is that each table would contain information for only one time point, so if you want to view/edit multiple time points then you would need to switch between them, add multiple browser nodes, or temporarily export to a merged table.

@MichaelColonel
Copy link
Collaborator

MichaelColonel commented Oct 23, 2019

I worked with MLC in DICOM a couple of times.
Here from dcmdump:

(300a,00b6) SQ (Sequence with undefined length #=3)     # u/l, 1 BeamLimitingDeviceSequence
  (fffe,e000) na (Item with undefined length #=4)         # u/l, 1 Item
    (300a,00b8) CS [MLCX]                                   #   4, 1 RTBeamLimitingDeviceType
    (300a,00ba) DS [538.0]                                  #   6, 1 SourceToBeamLimitingDeviceDistance
    (300a,00bc) IS [60]                                     #   2, 1 NumberOfLeafJawPairs
    (300a,00be) DS [-200.0\-190.0\-180.0\-170.0\-160.0\-150.0\-140.0\-130.0\-120.0\-11... # 354,61 LeafPositionBoundaries
  (fffe,e00d) na (ItemDelimitationItem)                   #   0, 0 ItemDelimitationItem
(fffe,e0dd) na (SequenceDelimitationItem)               #   0, 0 SequenceDelimitationItem

MLC has 60 leaves, 61 leaf boundary values as pairs (-200., -190.) (-190., -180.) ....

Position in one control point:

    (300a,011a) SQ (Sequence with undefined length #=3)     # u/l, 1 BeamLimitingDevicePositionSequence
      (fffe,e000) na (Item with undefined length #=2)         # u/l, 1 Item
        (300a,00b8) CS [MLCX]                                   #   4, 1 RTBeamLimitingDeviceType
        (300a,011c) DS [-105.0\-105.0\-105.0\-105.0\-105.0\-105.0\-105.0\-105.0\-105.0\-10... # 786,120 LeafJawPositions
      (fffe,e00d) na (ItemDelimitationItem)                   #   0, 0 ItemDelimitationItem
    (fffe,e0dd) na (SequenceDelimitationItem)               #   0, 0 SequenceDelimitationItem

There are 120 positions, one for each leaf, first 60 for one side, last 60 for the other side.

@MichaelColonel
Copy link
Collaborator

Importing MLCs from DICOM in vtkSlicerDicomRtReader. Some general questions.

  1. BeamEntry class members are only for control point sequence data (MLC positions) or not?
    https://github.com/SlicerRt/SlicerRT/blob/master/DicomRtImportExport/Logic/vtkSlicerDicomRtReader.cxx#L93-L131
  2. Where should i store MLC data from BeamLimitingDeviceSequence? Should it be static members in BeamEntry class, or separate class defining MLC parameters?
  3. What public methods must be added into the vtkSlicerDicomRtReader header to get access to the MLC parameters and MLC positions within control points?
  4. Can STL containers be used in the header?

Best regards,
Mikhail

@cpinter
Copy link
Member Author

cpinter commented Oct 25, 2019

  1. Since we haven't supported multiple control point yet, it's the parameters of the beam for that one control point. Maybe I don't fully understand the question.
  2. Neither I think. Static members should only be used for application-wide single data objects such as attribute names and such. Also, MLCs positions is not complex data enough to warrant a separate class (just a vector of pairs). During loading, I think it should go to the BeamEntry, and then the import logic should set it in a member of the vtkMRMLRTBeamNode class (exactly where it is currently, but instead of a double array node it should be table node, as I suggested above)
  3. I'd say GetBeamMultiLeafCollimatorPositions(unsigned int beamNumber, std::vector)
  4. Yes, but instead of returning it, I think it's best to have it as an argument that is cleared and filled in the function.

@MichaelColonel
Copy link
Collaborator

The reason for the first question is that one multi leaf collimator definition from BeamLimitingDeviceSequence, "MLCX" for example, can have a number of positions or openings within ControlPointSequence.

These members will be added to the BeamEntry
double SourceToBeamLimitingDeviceDistance;
unsigned int NumberOfLeafJawPairs;
std::vector< double > LeafPositionBoundary; // from BeamLimitingDeviceSequence
std::vector< double > LeafJawPositions; // from BeamLimitingDevicePositionSequence

What is a better way to store boundaries and positions?

  1. Raw std::vector of values from DICOM;
  2. Formatted std::vector< std::pair< double, double > > of leaf pairs.

@cpinter
Copy link
Member Author

cpinter commented Oct 25, 2019

It is hard to tell before starting implementation. I think you can start with either, and if you see that there are more disadvantages than advantages, then we can always switch. This is a small enough detail that it does not need to start as final. I think what is very important is to decide what data goes to which class, but it seems we have an agreement in that.
If others have a preference, please chime in. Thanks!

@gregsharp
Copy link
Member

My 2 cents:

The below are not part of a control point (they do not change).

double SourceToBeamLimitingDeviceDistance;
unsigned int NumberOfLeafJawPairs;
std::vector< double > LeafPositionBoundary; // from BeamLimitingDeviceSequence

But the below is part of the control point. You probably want a separate class to describe what is contained in a control point, because eventually you will have a vector of control points.

std::vector< double > LeafJawPositions; // from BeamLimitingDevicePositionSequence

@cpinter
Copy link
Member Author

cpinter commented Oct 25, 2019

@gregsharp True, however, if we include adding the control points feature to this topic, then it will reach too far. If @MichaelColonel does not need more than one control point, then I suggest keeping it simple for now, and building on it later when generalizing for multiple control points.

@MichaelColonel
Copy link
Collaborator

I don't want to break compatibility to the code that works, since i'm quite new in SlicerRT. But in the end, each beam must have multiple control points.

Something like

class ControlPointEntry {
public:
// Collimators positions, openings: "X", "Y", "ASYMX", "ASYMY", "MLCX", "MLCY"
unsigned int ControlPointIndex;
double LeafJawPositions[2][2]; // "X", "Y"
double LeafJawPositionsASYM[2][2]; // "ASYMX", "ASYMY"
std::vector< double > LeafJawPositionsMLCX;
std::vector< double > LeafJawPositionsMLCY;
};

class BeamEntry {
public:
// Collimators definitions: "X", "Y", "ASYMX", "ASYMY", "MLCX", "MLCY"
int NumberOfControlPoints;
std::vector< ControlPointEntry > ControlPointSequenceVector;
};

std::vector< BeamEntry > BeamSequenceVector;

@MichaelColonel
Copy link
Collaborator

MichaelColonel commented Oct 28, 2019

I've mostly done with loading MLC data in DicomRtReader for both RTPlan and RTIonPlan. Only one control point per beam for start.

Where the vtkMRMLTableNode for MLC opening data should be created?

@cpinter
Copy link
Member Author

cpinter commented Oct 28, 2019

Thanks for keeping us in the loop, I really appreciate it!

Creating the table node should be where you read in the MLC data, in ImportExportLogic. It should be set to the beam right there too.
How about you make a commit with the single control point MLCs working, I take a look at it, and suggest a way to implement multiple control points without you having to do the refactoring, but in a way that makes it easier for us to do that later?

@MichaelColonel
Copy link
Collaborator

I don't have the permission to push commit.

@cpinter
Copy link
Member Author

cpinter commented Oct 28, 2019

Let's follow the usual GitHub workflow:

  1. Fork the SlicerRT repository
  2. Create a branch (something like "multi-leaf-collimators"
  3. Do the commits there

When ready for integration, issue a pull request.

@MichaelColonel
Copy link
Collaborator

@cpinter I opened a pull request.

@cpinter
Copy link
Member Author

cpinter commented Oct 30, 2019

Please issue a pull request to SlicerRT/SlicerRT not cpinter/SlicerRT (which is also a fork).

If the PR is not about ion plans, please rename the branch so that it is expressive to what it contains. Thanks!

@cpinter
Copy link
Member Author

cpinter commented Oct 30, 2019

I see now why you issued the PR in my fork, it's because you want to integrate it to ion plans.

For future reference, it's important that we don't do many things under an umbrella, because it will be very hard to track why things happened and when. If you can do the MLC related commits separately which by the way should have been committed to this ticket and not the ion one, for the same reason), then we could integrate the MLCs and the ion plans separately. It would be much cleaner. I hope it's doable. Thanks!

@MichaelColonel
Copy link
Collaborator

MichaelColonel commented Oct 31, 2019

When the RTBeamNode creates beam polydata it doesn't use parameters from a beam limiting device sequence. It had been done just for demonstration?

@cpinter
Copy link
Member Author

cpinter commented Oct 31, 2019

It does, just above the code you highlighted:
https://github.com/SlicerRt/SlicerRT/blob/master/Beams/MRML/vtkMRMLRTBeamNode.cxx#L484-L574

@MichaelColonel
Copy link
Collaborator

WIP.

MLCX data are only data available tor testing.

MLCX -- example1
testMLC

vtkMRMLTableNode -- example1
mlcTableNode

MLCX -- example2
testMLC2

vtkMRMLTableNode -- example2
mlcTableNode2

@cpinter
Copy link
Member Author

cpinter commented Nov 8, 2019

Looks amazing, thanks a lot!

@MichaelColonel
Copy link
Collaborator

The process of handling MLCs in beam planning, and UI for that purpose.

Should it be just editing of selected MLC positions from DICOM RT file, or the user could be able to create additional MLC and edit their positions as well?

If only editing of selected positions, which is better for user interface: plugin widget or dialog window?

@cpinter
Copy link
Member Author

cpinter commented Nov 14, 2019

I think it's enough in the plugins if we can reference the MLC table node (by node ID), and the user should edit the MLC in the Tables module.

@MichaelColonel
Copy link
Collaborator

In that case could it be guaranteed, that the modified positions data would be valid (no intersection between leaves in pair, no meaningless values, etc).

@MichaelColonel
Copy link
Collaborator

@cpinter i'm done with initial implementation and pull request is ready for review. The beam vtkPolyData is displayed correctly when any value in MLC position table node is changed, or clicked "Edit properties..." action on a beam node. That's the only bug i have noticed.

I hope somebody could test not only MLCX but also MLCY.

@Sunderlandkyl
Copy link
Collaborator

Thanks for working on this! Csaba is in the process of moving, so I can review this pull request for you tomorrow.

@Sunderlandkyl
Copy link
Collaborator

@MichaelColonel I've merged the pull request in this commit 357d9db.

What organization are you a member of? We would like to include you in our list of collaborators 🙂.

@MichaelColonel
Copy link
Collaborator

Institute for High Energy Physics (IHEP)

Short Name: NRC «Kurchatov Institute» – IHEP
Full Name: Institute for High Energy Physics named by A.A.Logunov of National Research Center «Kurchatov Institute»

@MichaelColonel
Copy link
Collaborator

MLC representation like XiO.

Is it possible to create 3D or 2D not just a MLC modulated beam but a MLC itself? The MLC data could be used from the beam node.

I hope there is an easy way without making a custom node for that purpose.

MLC

@MichaelColonel
Copy link
Collaborator

It was relatively easy as @lassoan have said, but only as a static model.
MLC_Position_Model

@lassoan
Copy link
Member

lassoan commented Jan 18, 2020

Looks nice! You can automatically update the model node whenever any parameter is changed.

@MichaelColonel
Copy link
Collaborator

I can remove DoubleArrayNode node from MLC support and store all the data within single TableNode, because of #142.

New MLC data storage in table node will be: first column - leaf pairs boundary data, second column - leaf pairs position on side 1, third column - leaf pairs position on side 2.

@cpinter
Copy link
Member Author

cpinter commented May 1, 2020

@MichaelColonel This would be excellent, thank you for offering to help with this! The columns seem fine with me. Please make sure you document this clearly in comments in the code.

@MichaelColonel
Copy link
Collaborator

* Allow handling MLCs in beam planning

I've made a small module to calculate MLC position for a given RTBeamNode, target volume, and table node with defined leaf pair boundaries. Some of my colleagues want to try it without compilation and other stuff.

If such feature is needed in SlicerRT how could i integrate it with minimum problems (separate module, integrate in "Beams" module, GUI for MLC leaf pair boundaries calculation and so on)?

@cpinter
Copy link
Member Author

cpinter commented Jun 8, 2020

When you say module what do you mean exactly? I suppose not Slicer module, otherwise you wouldn't ask this question.

I think the best would be to add it as a new logic class in the Beams module's logic folder.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants