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

VR displayable manager should not show display node that are not referenced by displayable node #2628

Closed
slicerbot opened this issue Mar 12, 2020 · 3 comments · Fixed by #4776
Assignees
Labels
Type: Enhancement Improvement to functionality
Milestone

Comments

@slicerbot
Copy link
Collaborator

This issue was created automatically from an original Mantis Issue. Further discussion may take place here.

@slicerbot slicerbot added this to the Slicer 4.11.0 milestone Mar 12, 2020
@lassoan
Copy link
Contributor

lassoan commented Mar 26, 2020

@cpinter is this issue still relevant?

@cpinter
Copy link
Member

cpinter commented Mar 26, 2020

VR display nodes have the particularity to reference displayable node. Therefore they don't really need the displayable node to reference them. But it would be misleading to have display nodes without displayable node.

It is true that vol.ren. display nodes explicitly reference the volume node unlike other display nodes including the volume displayable node, which do an exhaustive search in the scene (but it has a one-item cache, see LastFoundDisplayableNode). And this reference is set in the logic by the UpdateDisplayNodeFromVolumeNode function (and I think this is part of the reason volume rendering doesn't work without having to call these update functions). It seems that this explicit reference has performance reasons, because it is faster to get the volume node from the display node in the displayable manager in functions like UpdateDisplayNodePipeline.

It may make sense to try and remove this extra complexity because it seems unlikely that performance would noticeably decrease.

For this reason, it would be better if the VR displayable manager hides VR display nodes if they are not referenced by at least one volume displayable node.

I'm not sure if this could be a problem or whether it is handled by the current code. However, doing the simplification would solve it for sure.

@lassoan lassoan assigned lassoan and unassigned cpinter Mar 26, 2020
@lassoan
Copy link
Contributor

lassoan commented Mar 26, 2020

Thanks for the analysis. I'll take care of this then.

@lassoan lassoan added Status: In Progress Person who the issue is assigned to currently works on this Type: Enhancement Improvement to functionality labels Mar 26, 2020
lassoan added a commit to lassoan/Slicer that referenced this issue Mar 26, 2020
…splay node

vtkMRMLVolumeRenderingDisplayNode::SetAndObserveVolumeNodeID method was removed, as display node base class already maintains a pointer to the displayed (volume) node.

Updated migration guide (https://www.slicer.org/wiki/Documentation/Nightly/Developers/Tutorials/MigrationGuide/Slicer#Volume_rendering).

Fixes Slicer#2628.
lassoan added a commit that referenced this issue Mar 27, 2020
…splay node

vtkMRMLVolumeRenderingDisplayNode::SetAndObserveVolumeNodeID method was removed, as display node base class already maintains a pointer to the displayed (volume) node.

Updated migration guide (https://www.slicer.org/wiki/Documentation/Nightly/Developers/Tutorials/MigrationGuide/Slicer#Volume_rendering).

Fixes #2628.
@lassoan lassoan removed the Status: In Progress Person who the issue is assigned to currently works on this label Jun 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Enhancement Improvement to functionality
Development

Successfully merging a pull request may close this issue.

3 participants