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

ENH: Use subject hierarchy in Models module #1054

Closed
wants to merge 1 commit into from

Conversation

@cpinter
Copy link

commented Dec 11, 2018

  • Only shows model and model hierarchy nodes (which are represented as SH folders)
  • All plugins are disabled except for Models, Folder, Opacity (and Default)
  • Apply color to branch action provided by the Folder plugin replaces the checkbox in model hierarchy
    • When the user changes the color of the folder then this option gets enabled automatically
    • Can be enabled/disabled using a checkable visibility action (right-click on eye icon or color)
    • If enabled, then visibility only applies to the folder, otherwise the default SH behavior is used (show/hide all items in whole branch)
ENH: Use subject hierarchy in Models module
- Only shows model and model hierarchy nodes (which are represented as SH folders)
- All plugins are disabled except for Models, Folder, Opacity (and Default)
- Apply color to branch action provided by the Folder plugin replaces the checkbox in model hierarchy
  - When the user changes the color of the folder then this option gets enabled automatically
  - Can be enabled/disabled using a checkable visibility action (right-click on eye icon or color)
  - If enabled, then visibility only applies to the folder, otherwise the default SH behavior is used (show/hide all items in whole branch)

@cpinter cpinter requested review from pieper, jcfr, lassoan, fedorov and ihnorton Dec 11, 2018

@cpinter

This comment has been minimized.

Copy link
Author

commented Dec 11, 2018

In the past weeks I added many features to subject hierarchy to support this PR. Now we have a column for colors, we have an opacity plugin, etc.

This PR replaces the model hierarchy tree in the Models module. I tried to do everything so that it's as close as possible to the old way, but there are of course changes.
I made this video that demoes the new tree:
https://drive.google.com/open?id=1n1w5mTWJu0nN89XioGPe3Em2Yb3sra94
What I forgot to include in the video is the opacity. There is no column for opacity in the new SH tree (the 1.00s looked strange and it was buggy too - flickering etc), but instead there is a plugin for it, and it can be set as before in the Display section. This is how the plugin looks like:
image

Please let me know what you think and I'd appreciate if someone familiar with the fiber related features could test it.
Thanks!

@cpinter

This comment has been minimized.

Copy link
Author

commented Dec 11, 2018

And also why I made all this :)

Other than "fixing" the bugs in the original scene model, this basically gives better support for atlases:

  • Terminology can be associated to models
  • Reorder is possible
  • Saving branch expanded state is possible
@jcfr

This comment has been minimized.

Copy link
Member

commented Dec 11, 2018

Thanks for tackling this 👍

I am curious to see how more complex atlas (e.g SPL atlas) would load.

@mhalle Since you have been working on the open anatomy browser, would you have any comment ?

@jamesjcook This may be relevant for your application

@jamesjcook

This comment has been minimized.

Copy link

commented Dec 11, 2018

Thanks for the Ping JC. After some moderate growing pains, and some help from the discourse and other devs, I'm using the subject hierarchy now for our model and tractography display protocol.

@cpinter

This comment has been minimized.

Copy link
Author

commented Dec 11, 2018

I made sure before that SH handles such atlases fine, but you're right I'll do tests with this one as well.

@pieper

This comment has been minimized.

Copy link
Member

commented Dec 11, 2018

makes sense to me - thanks for working on this 👍

@jcfr

This comment has been minimized.

Copy link
Member

commented Dec 11, 2018

I am also wondering if performance are improved with large atlases

@cpinter cpinter referenced this pull request Dec 12, 2018
7 of 7 tasks complete
@lassoan

This comment has been minimized.

Copy link
Contributor

commented Dec 12, 2018

@cpinter

This comment has been minimized.

Copy link
Author

commented Jan 4, 2019

@jcfr I loaded the SPL abdominal atlas and did some show/hiding, opacity and other property changing, with and without applying the folder property on the nodes under it. In both 4.10 and in my branch, the results were instantaneous, so it was hard to see any difference. Do you have any suggestion about how to gauge it better?

@mhalle

This comment has been minimized.

Copy link

commented Jan 4, 2019

@cpinter

This comment has been minimized.

Copy link
Author

commented Jan 4, 2019

The folder show/hide operation is much slower with the SH-based one, because by default it turns on slice intersections as well. Without slice intersections (with Apply color to branch turned on and slice intersection off for the folder) it is much faster though (0s vs 1s).

What do you think about an option in Application Settings for visibility of slice intersections? By default it could be off. However I'm not sure how to make it easy for the user to realize that there is a setting for this. I personally hated that in the Models module it was only possible to turn on slice intersections and that's why I implemented SH visibility so that it sets Visibility and also SliceIntersectionVisibility. Or maybe it's time for the separate 2D and 3D visibility columns? It wouldn't be hard to implement.
@lassoan @pieper @jcfr @mhalle @ihnorton

@pieper

This comment has been minimized.

Copy link
Member

commented Jan 4, 2019

I like the slice intersections being on by default since it's good for most uses - the brain atlas is more of a stress test. Having the 2D and 3D controls unified is very convenient so that should be the default so if you do add separate controls it would be good for them to be linked by default, but I agree there are times when you want to control them separately.

@lassoan

This comment has been minimized.

Copy link
Contributor

commented Jan 4, 2019

I like how it is configurable for Segmentations: you specify 2D and 3D display options in various flags and enums (visibility, color, projection mode, intersection thickness, etc.) and you have an additional global visibility flag to control overall visibility. If this was a backward-incompatible change, I think we could still do it (in Slicer5).

@cpinter

This comment has been minimized.

Copy link
Author

commented Feb 4, 2019

Now that 4.10.1 is out, I can integrate this change soon I think.

The only outstanding issue is the slowness of showing huge branches due to the slice intersections. @pieper @lassoan Have you had the chance to talk about this in Las Palmas?
I can either add an application setting for the slice intersections, or do a reform of the visibility controls in SH, whichever seems the best to everyone.

@lassoan

This comment has been minimized.

Copy link
Contributor

commented Feb 4, 2019

I think the best solution would be to have overall, slice, and 3D visibility options. The eye icon in the visibility column in SH would only change the overall visibility flag (therefore it would not turn on slice intersections).

@cpinter

This comment has been minimized.

Copy link
Author

commented Feb 4, 2019

So you'd add two more visibility columns? I guess we can have them hidden by default and have a checkbox for their visibility, similarly to the ID column.

@lassoan

This comment has been minimized.

Copy link
Contributor

commented Feb 4, 2019

For now, we could keep the single visibility column in the SH table (it would control the overall visibility), and add expose all 3 visibilities (overall, slice, 3D) in Models module.

Later we could reconsider how to make the 3 visibility flags conveniently accessible in SH.

@pieper

This comment has been minimized.

Copy link
Member

commented Feb 4, 2019

@cpinter

This comment has been minimized.

Copy link
Author

commented Feb 13, 2019

The solution we're proposing:

  • Change SH behaviour so that it does not set slice intersection visibility
  • Change display nodes (base class and up) so that they have a 2D and 3D visibility
  • Add a simple Visibility plugin in SH that offers toggling 2D and 3D visibility separately from the visibility context menu (right-click eye)

If this sounds good then I'll start doing it today.

@jcfr

This comment has been minimized.

Copy link
Member

commented Feb 13, 2019

Thanks for the detailed summary.

Change display nodes (base class and up) so that they have a 2D and 3D visibility

What would be the plan to implement this ?

Is the idea to leverage SetViewNodeIDs ?

@cpinter

This comment has been minimized.

Copy link
Author

commented Feb 13, 2019

ViewNodeIDs should work the same as before.

The idea is to do it similarly to how it works for segmentations
https://github.com/Slicer/Slicer/blob/master/Modules/Loadable/Segmentations/MRMLDM/vtkMRMLSegmentationsDisplayableManager3D.cxx#L174

@lassoan

This comment has been minimized.

Copy link
Contributor

commented Feb 14, 2019

The display node's "overall visibility" would be decoupled from the "2D visibility" and "3D visibility", as it is already done for segmentations:

  • The node is visible in 2D if and only if both overall visibility and 2D visibility are enabled.
  • The node is visible in 3D if and only if both overall visibility and 3D visibility are enabled.

The advantage of this decoupling is that we can show/hide of a node without changing its 2D/3D visibility flags.

@cpinter

This comment has been minimized.

Copy link
Author

commented Feb 15, 2019

Would it be possible to merge this PR so that I have an easier time with doing the display node and DM rework? Models module would work pretty well in general during this one week (I hope) period, except that atlas folder show/hide would be slower.

@cpinter

This comment has been minimized.

Copy link
Author

commented Feb 15, 2019

@jcfr @pieper what do you think about integrating this? (see previous comment)

@jcfr

This comment has been minimized.

Copy link
Member

commented Feb 15, 2019

Would it be possible to merge this PR
I have an easier time with doing the display node and DM rework?

Sounds good to me. I don't have the cycle to do thorough testing but the outlined plan seems very reasonable.

@cpinter

This comment has been minimized.

Copy link
Author

commented Feb 15, 2019

Thanks, Jc! If there are no objections I'll merge this over the weekend. In the meantime I'm making progress with the display nodes and the 2D/3D visibilities.

@cpinter

This comment has been minimized.

Copy link
Author

commented Feb 18, 2019

Integrated in e9f25f8

@cpinter cpinter closed this Feb 18, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.