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

Sh terminology and transform improvements #1046

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@cpinter
Contributor

cpinter commented Nov 23, 2018

Two sets of changes in subject hierarchy:

Colors and terminologies:

  • Add new column for color between visibility and transform columns. It shows the color for segments, models, and markup fiducials (selected color)
  • Double-clicking the color brings up terminology selector where terminology and color can be selected. Terminology information is stored in segments as before (segment attributes), and for models and markups it is stored in MRML node attributes. When selecting a terminology type, color is overwritten but name is not by default
  • Terminology selector has been improved to accommodate setting terminologies to model and markup fiducial nodes, for which it's not mandatory to set terminology (unlike segments)
    • None type always appears in type list unless type search term is specified
    • Multi-select is enabled for categories, and by default all the categories are selected so that all the types show up in the types list and a search finds any type in the context
    • Expand button added to hide categories list
    • Both categories and anatomy lists are hidden by default
    • Type search box has the focus when opened and selection can be accepted by Enter press (so terminology can be selected very quickly by just typing search term then pressing Enter)
      Fixes https://issues.slicer.org/view.php?id=4658
      Re PerkLab/SlicerOpenAnatomy#4

Transforms:

  • Transform column now shows icon instead of the first few letters of the transform name. There is a separate icon for linear and deformable transforms
  • Double-clicking the icon brings up a menu instead of a node selector that had to be clicked again
  • Subject hierarchy reference highlights are now updated immediately after selecting a transforms, so the highlighted transform node is always correct
    Re https://issues.slicer.org/view.php?id=4401
ENH: Add color column and improve transform column in subject hierarchy
- Add new column for color between visibility and transform columns. It shows the color for segments, models, and markup fiducials (selected color)
- Double-clicking the color brings up terminology selector where terminology and color can be selected. Terminology information is stored in segments as before (segment attributes), and for models and markups it is stored in MRML node attributes. When selecting a terminology type, color is overwritten but name is not by default
Re PerkLab/SlicerOpenAnatomy#4

- Transform column now shows icon instead of the first few letters of the transform name. There is a separate icon for linear and deformable transforms
- Double-clicking the icon brings up a menu instead of a node selector that had to be clicked again
- Subject hierarchy reference highlights are now updated immediately after selecting a transforms, so the highlighted transform node is always correct
Re https://issues.slicer.org/view.php?id=4401

@cpinter cpinter requested review from pieper, lassoan and fedorov Nov 23, 2018

@cpinter

This comment has been minimized.

Contributor

cpinter commented Nov 23, 2018

Color column:
image

Simplified terminology selector:
image

Transform column:
image

@fedorov

This comment has been minimized.

Member

fedorov commented Nov 23, 2018

@cpinter thanks for mentioning me! Considering RSNA is around the corner - how urgent is this?

@cpinter

This comment has been minimized.

Contributor

cpinter commented Nov 23, 2018

This is related to the OpenAnatomy project, and I don't think this is super urgent. However, I'll need to keep working on things that build on this change, so it would make life easier if this could get integrated. If you and the others agree with what you see in the three images, then I guess this is safe to integrate. You'll use 4.10 stable at RSNA right?

@lassoan

This looks great, thank you! I have just a small comment about attribute names (see comment in code).

}
if (terminologyMetaData.contains(qSlicerTerminologyItemDelegate::NameAutoGeneratedRole))
{
modelNode->SetAttribute("NameAutoGenerated",

This comment has been minimized.

@lassoan

lassoan Nov 23, 2018

Contributor

Without any context, "NameAutoGenerated" is a bit obscure (not clear who generates it automatically, from what, when). It would clarify things if we included SubjectHierarchy and/or Terminology in the attribute name. For example:

  • SubjectHierarchy.NameAutoGenerated
  • SubjectHierarchy.UseTerminologyForName
  • Terminology.NodeNameAutoGenerated
  • Terminology.AutoUpdateNodeName

I think I like the last one the most.

This comment has been minimized.

@cpinter

cpinter Nov 23, 2018

Contributor

Sounds good to me

This comment has been minimized.

@cpinter

cpinter Nov 23, 2018

Contributor

The name of the module is Terminologies (plural). Shouldn't we use that instead of singular?

This comment has been minimized.

@lassoan

lassoan Nov 23, 2018

Contributor

Yes, good point, we should use the exact module name.

This comment has been minimized.

@lassoan

lassoan Nov 23, 2018

Contributor

It would be also good to define these attribute names somewhere (in Terminologies module?) to reduce chance of typos and have a place where these attributes can be documented.

@fedorov

This comment has been minimized.

Member

fedorov commented Nov 23, 2018

Csaba, the scope of changes sounds good to me. I am not concerned about breaking functionality for RSNA, but more about not having time to test the changes in the branch before RSNA. I know you will be maintaining this so, I don't worry too too much about something being broken, it can be fixed later, so I am fine if you merge.

A couple of (unrelated, sorry) feature suggestions I have, since I was playing with Segmentations/Segment Editor/Data modules in preparation to RSNA, and I am afraid I may forget:

Data module is very convenient to browse segmentations, since it shows the hierarchy, and is particularly useful when segmentations are stored in separate DICOM objects, loading as separate segmentation nodes. It would be very helpful if Data module provided:

  1. functionality to toggle visibility for selected items (it already provides multi-select and visibility toggle for individual items, so it is just an item in the context menu to streamline the process)
  2. "jump to slice" in the context menu, as in Segment Editor.
@cpinter

This comment has been minimized.

Contributor

cpinter commented Nov 23, 2018

@fedorov Thanks, I'll probably merge soon if nobody has conceptual issues with this. I think I did thorough testing, and honestly it will be much easier to test for everyone if it's in a nightly release. And of course I'll fix whatever issue arises.

The SH suggestions sound good to me.

  1. Let me think a bit about this. Currently only delete is in the multi-select pop-up menu, an dit wouldn't be a big deal adding this. However there is also a visibility context menu (right-click the eye), and I'm not sure if it should go there. In the meantime you can use the "hide all segments but this" option (if I remember correctly)
  2. I think we talked about this point with @lassoan recently, and it will be no problem implementing it.
@fedorov

This comment has been minimized.

Member

fedorov commented Nov 23, 2018

@cpinter

  1. Let me think a bit about this. Currently only delete is in the multi-select pop-up menu, an dit wouldn't be a big deal adding this. However there is also a visibility context menu (right-click the eye), and I'm not sure if it should go there.

I tested, and either it's a bug, or omission - if I have multiple items selected in the Data module, I do indeed see "Show only this segment" in the context menu when clicked over eye, but with multiple items selected, it has no effect, see below.

In the meantime you can use the "hide all segments but this" option (if I remember correctly)

Yes, this indeed is working, but it is in the "Segmentations" module. So in a way there is a discrepancy between Data, Segmentations and Segment editor:

  • Data does not support "Jump" or "Show selected only"
  • Segment Editor does not support multi-select, and does not allow to select across segmentations (and not providing both of those features make sense in Segment Editor)
  • Segmentations does not allow to select across segmentations (and this also makes sense)

I can definitely achieve the task, but I need to jump across the modules. Adding "show multiple" and "jump to slice" to the Data module I think would be a natural and much needed shortcut.

nov-23-2018 15-14-33

@cpinter

This comment has been minimized.

Contributor

cpinter commented Nov 23, 2018

Multi-select in SH is pretty basic. I think what you see is a bug, because on multi-select, the functions that come from the plugins (which work on one item at a time) shouldn't even be visible. I'll address this too.

Yes it makes a lot of sense to add the most used convenience functions so it's more consistent.

See issue: https://issues.slicer.org/view.php?id=4659
Feel free to edit it.

@pieper

This comment has been minimized.

Member

pieper commented Nov 24, 2018

Looks like great work! I agree testing in the nightly will be easiest. 👍

@lassoan

This comment has been minimized.

Contributor

lassoan commented Nov 24, 2018

Yes, since the stable is relatively recent, we can take use the nightly for testing.

However, CI reported a build error that may be related to the changes:

/opt/qt/5.11.2/gcc_64/include/QtCore/qvariant.h:484:9: error: ‘int QVariant::compare(const QVariant&) const’ is protected
int compare(const QVariant &other) const;
^
/usr/src/Slicer/Modules/Loadable/SubjectHierarchy/Widgets/qMRMLSubjectHierarchyModel.cxx:1064:47: error: within this context
if (item->data().compare(transformNodeId))

@cpinter

This comment has been minimized.

Contributor

cpinter commented Nov 24, 2018

Thanks! I'll fix the error.

ENH: Add none selection option to terminology selector
Terminology selector has been improved to accommodate setting terminologies to model and markup fiducial nodes, for which it's not mandatory to set terminology (unlike segments)
- None type always appears in type list unless type search term is specified
- Multi-select is enabled for categories, and by default all the categories are selected so that all the types show up in the types list and a search finds any type in the context
- Expand button added to hide categories list
- Both categories and anatomy lists are hidden by default
- Type search box has the focus when opened and selection can be accepted by Enter press (so terminology can be selected very quickly by just typing search term then pressing Enter)
Fixes https://issues.slicer.org/view.php?id=4658

Also, bug fixed in segment color update so that it's correct both in segments table widget (in Segment Editor) and subject hierarchy.

@cpinter cpinter force-pushed the cpinter:sh-terminology-and-transform-improvements branch from 0965704 to abb4874 Nov 26, 2018

@cpinter

This comment has been minimized.

Contributor

cpinter commented Nov 26, 2018

I did the fix and although CircleCI returned errors, they were about network timeouts not actual build errors, so I went ahead and integrated the change in
25ca991 and
3a5d29d

@cpinter

This comment has been minimized.

Contributor

cpinter commented Nov 26, 2018

I just noticed that I didn't include @lassoan 's request about the attribute names although I was pretty sure I did. I'll address this in a subsequent commit.

@cpinter

This comment has been minimized.

Contributor

cpinter commented Nov 26, 2018

Here it is: 3e7a41a

@cpinter cpinter closed this Nov 26, 2018

@lassoan

This comment has been minimized.

Contributor

lassoan commented Nov 27, 2018

Thanks a lot, it will be great to have these features. This makes terminologies much more accessible to users. I think I'll use terminologies more (even if I just want to set the colors, it will be easier to set the terminology).

@cpinter

This comment has been minimized.

Contributor

cpinter commented Nov 27, 2018

Exactly. The previous selector with the jet cockpit complexity could have been a barrier for many people. It is now much quicker to make a meaningful selection.

@cpinter

This comment has been minimized.

Contributor

cpinter commented Nov 29, 2018

@fedorov FYI "Show only this segment" was already present in subject hierarchy, see
image

For this you need to right-click the eye icon (there is a general context menu when you right-click on the name column and a visibility context menu over the eye icon).

I have implemented toggle visibility on multi-select because I think it's very useful regardless.

I'm thinking whether I should add "Jump to slices" to the general or the visibility context menu. I'm leaning towards the latter. @lassoan what do you think?

@cpinter

This comment has been minimized.

Contributor

cpinter commented Nov 29, 2018

@fedorov I implemented everything in the issue above (decided to do it as I'm working on SH anyway and these were easy): https://issues.slicer.org/view.php?id=4659
Whenever you have the chance please give it a go and let me know what you think.

I'll work on the issue of mandatory type modifiers and duplicate type items next.

@fedorov

This comment has been minimized.

Member

fedorov commented Nov 30, 2018

@cpinter

FYI "Show only this segment" was already present in subject hierarchy, see

Yes, I know that. What I was saying in #1046 (comment) is if I select multiple items, activate the menu, and select "show only", it had no effect.

@cpinter

This comment has been minimized.

Contributor

cpinter commented Nov 30, 2018

True. Thanks for the clarification.

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