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: added support for ScalarVolume inheritant #24

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kerim371
Copy link

Before that it was impossible to choose ScalarVolume derived as IO node.
There was a discoussion on that

@lassoan
Copy link
Collaborator

lassoan commented Mar 18, 2022

Allowing display of child nodes would make vector and label volumes show up. It would be better to avoid using custom volume type or just add your custom volume type to the list of allowed volume types (e.g., by adding a SimpleFilters configuration variable that would contain a list of additional scalar volume class names).

You may potentially need to change this flag in many other modules in Slicer if you want your custom volume type supported everywhere.
Therefore, it would better not have a new scalar volume class. You can use custom attributes, custom display and storage nodes, etc. to customize behavior of a volume node. Do you really need a custom scalar volume node type?

@kerim371
Copy link
Author

@lassoan thank you for feedback

Do you really need a custom scalar volume node type?
Yes, it is very attractive module and has sense for geoscience

Can we simply add checkbox enabling/disabling ScalarVolume inheritants and add a tooltip with the description?

Or if this concerns only me then I could fork the repo and build it SlicerCAT using it. I have some tool that helps me to do such things without breaking Superbuild so it should be doable.

@lassoan
Copy link
Collaborator

lassoan commented Mar 18, 2022

You can just add a module variable that lists all additional volume node classes that will be added to volume node selectors.

@kerim371
Copy link
Author

the problem is that I don't know how to acces this variable member from another module.
For example I can add a member variable nodeTypes to FilterParameters class. But how to modify this var from another module? I can get filter module with the command filt_module=slicer.modules.simplefilters but I don't see any method returning instance of that class

@lassoan
Copy link
Collaborator

lassoan commented Mar 18, 2022

You can do something like this: slicer.modules.SimpleFiltersInstance.scalarVolumeNodeClasses=['vtkMRMLScalarVolumeNode' ]

It is now possible to add custom volume types to node selectors:
`slicer.modules.SimpleFiltersInstance.scalarVolumeNodeClasses=['...']`
@kerim371 kerim371 force-pushed the enable-scalarvolume-inheritant branch from 2802284 to 8fec8bd Compare March 18, 2022 19:22
@kerim371
Copy link
Author

@lassoan thank you, that works!

Copy link
Contributor

@pieper pieper left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@kerim371
Copy link
Author

@jcfr please review

Copy link
Contributor

@jcfr jcfr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After reading the associated post, should the following attribute be set ?

inputSelector.showChildNodeTypes = True

@kerim371
Copy link
Author

After reading the associated post, should the following attribute be set ?

inputSelector.showChildNodeTypes = True

Probably this is the most elegant solution but as @pieper said this would allow to choose inapropriate vtkMRMLScalarVolumeNode successor nodes for computation.
Probably we should better prevent Slicer users from selecting inapropriate nodes for IO.
At the same time we could put information how to add custom volume nodes to Simple Filters somewhere in the developper documentation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants