WIP: ENH: Relative dose representation for isolevels #168
Conversation
Isodose/Logic/vtkMRMLIsodoseNode.h
Outdated
| @@ -37,6 +37,7 @@ class vtkMRMLColorTableNode; | |||
| class VTK_SLICER_ISODOSE_LOGIC_EXPORT vtkMRMLIsodoseNode : public vtkMRMLNode | |||
| { | |||
| public: | |||
| enum DoseUnitsType { GY, RELATIVE, UNKNOWN_UNITS }; | |||
There was a problem hiding this comment.
Please see how enums are usually set up in Slicer and follow that convention
There was a problem hiding this comment.
In addition to my comments in the code I find it strange that you have a collpasible button on the UI for the Prescribed or reference dose, but then below you have the levels table. It is quite confusing what belongs where.
Not quite sure what is the problem with levels table? Do you want to get rid of it entirely?
Dose units will be used as follows: enum DoseUnitsType { Unknown = -1, Gy = 0, Relative = 1 };
Isodose/Logic/vtkMRMLIsodoseNode.h
Outdated
| protected: | ||
| vtkMRMLIsodoseNode(); | ||
| ~vtkMRMLIsodoseNode(); | ||
| vtkMRMLIsodoseNode(const vtkMRMLIsodoseNode&); | ||
| void operator=(const vtkMRMLIsodoseNode&); | ||
|
|
||
| void SetDoseUnits(int doseUnits = 0); |
There was a problem hiding this comment.
It's strange that there is a default value for the argument in a Set function. Why?
There was a problem hiding this comment.
There is no default value anymore.
| @@ -217,7 +216,7 @@ void qSlicerIsodoseModuleWidget::setMRMLScene(vtkMRMLScene* scene) | |||
| } | |||
| else | |||
| { | |||
| vtkSmartPointer<vtkMRMLIsodoseNode> newNode = vtkSmartPointer<vtkMRMLIsodoseNode>::New(); | |||
| vtkNew<vtkMRMLIsodoseNode> newNode; | |||
There was a problem hiding this comment.
Thanks for cleaning up the code as you go.
| image->GetScalarRange(valueRange); | ||
| if (!doseUnitName.compare("RELATIVE")) | ||
| { | ||
| paramNode->SetDoseUnits(vtkMRMLIsodoseNode::RELATIVE); |
There was a problem hiding this comment.
This is how you should namespace an enum (the class name instead of the enum type). Please fix the wrong ones.
| scalarBarTitle += " (" + doseUnitName + ")"; | ||
| case vtkMRMLIsodoseNode::DoseUnitsType::GY: | ||
| { | ||
| const char* str = relativeRepresentation ? " (%)" : " (Gy)"; |
There was a problem hiding this comment.
Wrong indentation. Please fix here and below
| relativeFlag = true; | ||
| } | ||
| std::string isodoseName = (relativeFlag) ? | ||
| vtkSlicerIsodoseModuleLogic::ISODOSE_RELATIVE_ROOT_HIERARCHY_NAME_POSTFIX : |
There was a problem hiding this comment.
Why do we need to have two different postfixes? Can we use both at the same time?
There was a problem hiding this comment.
Two isolevels at the same time don't work quite right in hierarchy node. If both absolute and relative isolevels with the same postfix in hierarchy you can hide only one isolevel folder node, other can't be hidden. So if use only one postfix => only one isolevels folder node.
No, obviously not. But from your screenshot it seems that you have a new collapsible button within the Input section. The problem with that button is that it's not obvious where that section ends, and that it is unusual to have a collapsible button within another collapsible button. I suggest reorganizing the UI so that it is less confusing. Maybe instead of a collapsible button, you could put the "Prescribed or reference dose value" in a groupbox, but other solutions could work too. |
I've changed UI a little with some other fixes.
|
|
I like this much better, thank you! @lassoan @gregsharp do you have anything to add? If not then I'll merge this on or after the weekend. |
MichaelColonel
force-pushed
the
relative-dose
branch
from
cefa6e6 to
d712ae3
Compare
Here is my understanding of relative dose isolevels, possible solution for #36, #96.
If RTDOSE type is "GY" then clicking on "Percentage dose isolevels" check box you can switch between absolute and relative isolevels representation. If RTDOSE type is "RELATIVE" then only relative representation is available.
Screenshot as an example:
