I see what is happening.

The outputs of a CLI are ultimately read into Slicer using the vtkSlicerApplicationLogic::ProcessReadNodeData() method.

If there is a StorageNode attached to the node, then that is what is used to read in the data. If there not a StorageNode attached to the node, then the method tries to create one.

The create logic is hardcoded for the volume node types in the Slicer trunk. Since many of the volume node types are specializations of other volume node types, there is a precedence in the search for a compatible storage node type.

Unfortunately, MultiVolume is not in the Slicer trunk, so a case cannot be hard coded for it.

But I recall adding something to MRML a while back so that you could ask a node to create a storage node of the appropriate type. Perhaps a block of code with this hard coded specialization can be replaced with that.

Try this change

diff --git a/Base/Logic/vtkSlicerApplicationLogic.cxx b/Base/Logic/vtkSlicerAppl
index 41e7412..1351c62 100644
--- a/Base/Logic/vtkSlicerApplicationLogic.cxx
+++ b/Base/Logic/vtkSlicerApplicationLogic.cxx
@@ -917,12 +917,7 @@ void vtkSlicerApplicationLogic::ProcessReadNodeData(ReadDat
{
// Load a scalar or vector volume node
//

• // Need to maintain the original coordinate frame established by
• // the images sent to the execution model
• vtkMRMLVolumeArchetypeStorageNode *vin
• = vtkMRMLVolumeArchetypeStorageNode::New();
• vin->SetCenterImage(0);
• storageNode = vin;
• storageNode = storableNode->CreateDefaultStorageNode();
  }
  else if (dtvnd || dwvnd)
  {

The node is read correctly as multivolume, but the display node is incorrectly set to ScalarVolumeDisplayNode.

Okay. This next one it trickier.

Add a method CreateDefaultDisplayNodes() in MRMLMultiVolumeNode. See the MRMLFiberBundleNode for a reference. The method is defined in MRMLDisplayableNode.

Then apply this diff (assuming the previous diff was applied).

diff --git a/Base/Logic/vtkSlicerApplicationLogic.cxx b/Base/Logic/vtkSlicerAppl
index 41e7412..7030624 100644
--- a/Base/Logic/vtkSlicerApplicationLogic.cxx
+++ b/Base/Logic/vtkSlicerApplicationLogic.cxx
@@ -1112,6 +1107,12 @@ void vtkSlicerApplicationLogic::ProcessReadNodeData(ReadD
// Get the right type of display node. Only create a display node
// if one does not exist already
//

• vtkMRMLDisplayableNode *displayableNode = vtkMRMLDisplayableNode::SafeDownCas
• if (displayableNode)
• {
• displayableNode->CreateDefaultDisplayNodes();
• if (!displayabeNode->GetDisplayNode())
• {
  if ((svnd && !svnd->GetDisplayNode())
  || (vvnd && !vvnd->GetDisplayNode()))
  {
  @@ -1139,11 +1140,11 @@ void vtkSlicerApplicationLogic::ProcessReadNodeData(Read
  if (dtvnd)
  {
  vtkMRMLDiffusionTensorVolumeDisplayNode *dtvdn = vtkMRMLDiffusionTensorVo
  -// dtvdn->SetWindow(0);
  -// dtvdn->SetLevel(0);
  -// dtvdn->SetUpperThreshold(0);
  -// dtvdn->SetLowerThreshold(0);
  -// dtvdn->SetAutoWindowLevel(1);
• // dtvdn->SetWindow(0);
• // dtvdn->SetLevel(0);
• // dtvdn->SetUpperThreshold(0);
• // dtvdn->SetLowerThreshold(0);
• // dtvdn->SetAutoWindowLevel(1);
  disp = dtvdn; // assign to superclass pointer
  }
  else
  @@ -1237,6 +1238,8 @@ void vtkSlicerApplicationLogic::ProcessReadNodeData(ReadDa
  }
  disp->Delete();
  }
• }
• }

This is hack but it may work. Basically, the CreateDefaultDisplayNodes() does what we want but it didn't exist when we wrote ProcessReadNodeData() method. Unfortunately, CreateDefaultDisplayNodes() has not been propagated through the node types.

Ideally, we should be able to call

sn = storableNode->CreateDefaultStorageNode();
displayableNode->CreateDefaultDisplayNodes();

and eliminate almost all the code in ProcessReadNodeData().

Jim can you clarify;

1) CreateDefaultDisplayNodes() is defined on the MRMLNode, not on MRMLDisplayableNode - is this correct? ==>

• vtkMRMLDisplayableNode *displayableNode = vtkMRMLDisplayableNode::SafeDownCas
• if (displayableNode)
• {
• displayableNode->CreateDefaultDisplayNodes();

2) long lines in your patch are incomplete

1) CreateDefaultDisplayNodes() is defined on MRMLDisplayableNode NOT on MRMLNode. So you have to cast the node to a MRMLDisplayableNode before you can call it.

2) Sorry about the truncation in the patch. Here is a better one

diff --git a/Base/Logic/vtkSlicerApplicationLogic.cxx b/Base/Logic/vtkSlicerApplicationLogic.cxx
index 41e7412..96ac498 100644
--- a/Base/Logic/vtkSlicerApplicationLogic.cxx
+++ b/Base/Logic/vtkSlicerApplicationLogic.cxx
@@ -1112,6 +1107,12 @@ void vtkSlicerApplicationLogic::ProcessReadNodeData(ReadDataRequest& req)
// Get the right type of display node. Only create a display node
// if one does not exist already
//

• vtkMRMLDisplayableNode *displayableNode = vtkMRMLDisplayableNode::SafeDownCast(nd);
• if (displayableNode)
• {
• displayableNode->CreateDefaultDisplayNodes();
• if (!displayableNode->GetDisplayNode())
• {
  if ((svnd && !svnd->GetDisplayNode())
  || (vvnd && !vvnd->GetDisplayNode()))
  {
  @@ -1139,11 +1140,11 @@ void vtkSlicerApplicationLogic::ProcessReadNodeData(ReadDataRequest& req)
  if (dtvnd)
  {
  vtkMRMLDiffusionTensorVolumeDisplayNode *dtvdn = vtkMRMLDiffusionTensorVolumeDisplayNode::New();
  -// dtvdn->SetWindow(0);
  -// dtvdn->SetLevel(0);
  -// dtvdn->SetUpperThreshold(0);
  -// dtvdn->SetLowerThreshold(0);
  -// dtvdn->SetAutoWindowLevel(1);
• // dtvdn->SetWindow(0);
• // dtvdn->SetLevel(0);
• // dtvdn->SetUpperThreshold(0);
• // dtvdn->SetLowerThreshold(0);
• // dtvdn->SetAutoWindowLevel(1);
  disp = dtvdn; // assign to superclass pointer
  }
  else
  @@ -1237,6 +1238,8 @@ void vtkSlicerApplicationLogic::ProcessReadNodeData(ReadDataRequest& req)
  }
  disp->Delete();
  }
• }

• }

  // Cause the any observers to fire (we may have avoided calling
  // modified on the node)

Here is a patch ignoring space changes

diff --git a/Base/Logic/vtkSlicerApplicationLogic.cxx b/Base/Logic/vtkSlicerApplicationLogic.cxx
index 41e7412..96ac498 100644
--- a/Base/Logic/vtkSlicerApplicationLogic.cxx
+++ b/Base/Logic/vtkSlicerApplicationLogic.cxx
@@ -1112,6 +1107,12 @@ void vtkSlicerApplicationLogic::ProcessReadNodeData(ReadDataRequest& req)
// Get the right type of display node. Only create a display node
// if one does not exist already
//

• vtkMRMLDisplayableNode *displayableNode = vtkMRMLDisplayableNode::SafeDownCast(nd);
• if (displayableNode)
• {
• displayableNode->CreateDefaultDisplayNodes();
• if (!displayableNode->GetDisplayNode())
• {
  if ((svnd && !svnd->GetDisplayNode())
  || (vvnd && !vvnd->GetDisplayNode()))
  {
  @@ -1237,6 +1238,8 @@ void vtkSlicerApplicationLogic::ProcessReadNodeData(ReadDataRequest& req)
  }
  disp->Delete();
  }
• }

• }

  // Cause the any observers to fire (we may have avoided calling
  // modified on the node)

1) CreateDefaultDisplayNodes() is defined on MRMLDisplayableNode
NOT on MRMLNode. So you have to cast the node to a MRMLDisplayableNode
before you can call it.

Sorry about that, you had "DisplayableNode", but I was thinking "DisplayNode" ...

Thank you for the corrections.

This seems to work. Should I submit a pull request to Slicer trunk when done or will you integrate?

Will update MultiVolume MRML in the external module too.

We should probably coordinate with JC to have this backported to 4.3 - do you foresee any issue with that?

Fixes to MultiVolume MRML: https://github.com/fedorov/MultiVolumeExplorer/commit/ab7eb9411c5eaacacf81685b4c7e05cf1935d9a3

Corresponding update in Slicer r22420 (todo ask JC to backport).

Patched Slicer branch: https://github.com/fedorov/Slicer/tree/3382-fix-loading-multivolumes-from-CLIs

The issue is that the above is kind of a hack. I could see someone editting it out because it is not clear why it is needed.

For maintainability, I would prefer that fix the ProcessReadNodeData() method to transition all the special case code to method calls in the underlying nodes (e.g. CreateDefaultStorageNode(), CreateDefaultDisplayNodes(), whatever else we need). It is a pretty big change. Really should change all the DisplayableNodes to have a CreateDefaultDisplayNodes method. That would need a good bit of testing. Plus it looks like the Freesurfer path is little more complicated. Not sure I am comfortable doing it in a rush to backport to 4.3 in the next few days.

Sounds good. We should decide what to do with PkModeling. I think the recent patches add important functionality, but not sure they should be included in the 4.3 release of the extension if we know that running it from the GUI will crash Slicer.

Working on a new version of PrcessReadNodeData().

Patch will either

1. Rely on a node having CreateDefaultDisplayNode() and CreateDefaultStorageNode() methods, or

2. Use a factory to create nodes like in vtkSlicerVolumesLogic.

Thanks for the reminder. I had completely forgotten about it. I'll take some time to study it again and see if I still like the solution I had planned.

Thank you Jim. If there is something I can help with, let me know.

Shall we target the 4.4 release for this?

Jim, any updates? Do you think this will be fixed for 4.4?

Target 4.4

Jim says, we should try to apply the patch he suggested early in the history of this bug. I will try that and report if this fixes the problem.

Patch integrated with today's master, and is available in the pull request here: https://github.com/Slicer/Slicer/pull/225

Jim confirmed he is going to review and add comments before merging.

Merged with master in 23876 and 23877.