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

Comments

@slicerbot
Copy link
Collaborator

@slicerbot slicerbot commented Mar 12, 2020

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

This comment has been minimized.

Copy link
Contributor

@lassoan lassoan commented Mar 26, 2020

@cpinter is this issue still relevant?

@cpinter

This comment has been minimized.

Copy link
Member

@cpinter 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

This comment has been minimized.

Copy link
Contributor

@lassoan lassoan commented Mar 26, 2020

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

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked pull requests

Successfully merging a pull request may close this issue.

3 participants
You can’t perform that action at this time.