Initial implementation checked into my github[1], need to do a bit more testing to make sure that batch processing the display toggle works well, as well as getting a bit of feedback about toggling the hierarchy display nodes[2].

[1] https://github.com/naucoin/Slicer/commit/d964bdb9ffdf249cd793e27917e5b9b7e706f5db

[2] Currently not toggling hierarchy display node visibility. This can leave a hierarchy node in a different state than the models under it, but if you click the apply to children check box, you can use that visibility. Or toggle the eye icon a couple of times to push that visibility down.

Commited to svn 21670 so I can get feedback from the nightly build.

http://viewvc.slicer.org/viewvc.cgi/Slicer4?view=revision&sortby=file&revision=21670

Let me know if you want the hierarchy node visibility also set.

Hi Nicole,

In commit r21670, Looking at the code "vtkSlicerModelsLogic.cxx", it seems the following piece of code:

// -------------
this->GetMRMLScene()->StartState(vtkMRMLScene::BatchProcessState);
for (int i = 0; i < numModels; i++)
{
vtkMRMLNode mrmlNode = this->GetMRMLScene()->GetNthNodeByClass(i, "vtkMRMLModelNode");
if (mrmlNode != NULL)
{
// rule out subclasses
if (strcmp(mrmlNode->GetClassName(), "vtkMRMLModelNode") == 0)
{
// rule out slice nodes
if (mrmlNode->GetName() != NULL &&
strstr(mrmlNode->GetName(), "Volume Slice") == NULL)
{
vtkMRMLModelNode modelNode = vtkMRMLModelNode::SafeDownCast(mrmlNode);
if (modelNode)
{
// have a "real" model node, set the display visibility
modelNode->SetDisplayVisibility(flag);
}
}
}
}
}
//-----------------

could be changed into something simpler

//-----------------

this->GetMRMLScene()->StartState(vtkMRMLScene::BatchProcessState);
for (int i = 0; i < numModels; i++)
{
vtkMRMLNode *modelNode = vtkMRMLModelNode::SafeDownCast(this->GetMRMLScene()->GetNthNodeByClass(i, "vtkMRMLModelNode"));
if (modelNode != NULL)
{
modelNode->SetDisplayVisibility(flag);
}
}
//-----------------

What are the node having the string "Volume Slice" in their name ? I loaded the SPL Atlas and couldn't see any node with such name.

Thanks
Jc

In the 3D scene the 2d slice planes are displayed using model nodes.

1) The string "Volume Slice" is indeed hardcoded here [1]

Please, consider adding a public member to "vtkMRMLSliceLogic.h":

const std::string SLICE_MODEL_NODE_NAME_SUFFIX set to "Volume Slice".

This member would then be re-used where appropriate.

2) In addition, a function named "IsSliceModelNode(vtkMRMLModelNode * node)" should probably be added and re-used where it applies.

3) Hardcoding such string will cause us more trouble moving forward.

4) I still think the code could be simplified, considering this previous remark, what do you think of:

//-----------------

this->GetMRMLScene()->StartState(vtkMRMLScene::BatchProcessState);
for (int i = 0; i < numModels; i++)
{
vtkMRMLNode *modelNode = vtkMRMLModelNode::SafeDownCast(this->GetMRMLScene()->GetNthNodeByClass(i, "vtkMRMLModelNode"));
if (modelNode != NULL && !vtkMRMLSliceLogic::IsSliceModelNode(modelNode))
{
modelNode->SetDisplayVisibility(flag);
}
}
//-----------------

5) The SetName function of vtkMRMLModelNode exposed in the public api should also check that user doesn't provide a name ending with the "reserved" suffix. Unexpected behavior may happen otherwise. Model slice node should be handled through a dedicated API.

[1] https://github.com/Slicer/Slicer/blob/master/Libs/MRML/Logic/vtkMRMLSliceLogic.cxx#L1332

4. The problem is that GetNthNodeByClass returns subclasses of the class given, and if there are annotations in the scene, they will be returned as well. That's why I had to do the combination of getting nodes by class and checking against the classname as well as the node name for the volume slices.

For the rest of your points, if we're going to make changes, I'd like to think about it a bit more to find a more elegant solution that keeping the reliance on a naming convention for volume slices. We could use an attribute, or subclass the model class.

Using an attribute or relying on the name suffix, I still think adding the method "IsSliceModelNode" to "vtkMRMLSliceLogic"makes sense.

Either way (attribute or based on the name) I would suggest we change the code to minimize confusion and ease future maintenance. If it take more than a day, we should probably go ahead and proceed to the easy change. It would be better that what we have.

To address the fact annotation can be returned, checking the classname make sense. That said, I would recommend a different approach. The "SetDisplayVisibility" of annotation could be a no-op, instead a method named "SetAnnotationVisibility" could be added.

Doing so, model would all be treated the same way. User deriving from ModelNode would also be ensure that their node would be considered as a model.

This is a good example illustrating the fact that when the code itself is not explicit enough, comment could help understanding the context :)

Thanks

Hrm, adding IsSliceModelNode to the logic means another step in the models module code to get that logic point, but I think it seems simple enough.
Changing the SetName call would need some testing to make sure that it's not disabling anything that it shouldn't be.
I'll strongly vote against changing SetDisplayVisibility for the annotations as changing to that unified call had a lot of fall out and would definitely take more than a day to sort out.

Got it, I guess the following could be a good compromise:

Approach number 1 - Exclude all vtkMRMLModelNode subclasses.

//-----------------
this->GetMRMLScene()->StartState(vtkMRMLScene::BatchProcessState);
for (int i = 0; i < numModels; i++)
{
vtkMRMLNode *modelNode = vtkMRMLModelNode::SafeDownCast(this->GetMRMLScene()->GetNthNodeByClass(i, "vtkMRMLModelNode"));
// Exclude vtkMRMLModelNode subclasses by comparing classname.
// Doing so will avoid to update annotation node visibility since
// vtkMRMLAnnotationNode derives from vtkMRMLModelNode.
// See http://www.na-mic.org/Bug/view.php?id=2576
if (modelNode != NULL
&& !vtkMRMLSliceLogic::IsSliceModelNode(modelNode)
&& strcmp(modelNode->GetClassName(), "vtkMRMLModelNode") == 0)
{
modelNode->SetDisplayVisibility(flag);
}
}
//-----------------

Approach number 2 - Exclude vtkMRMLAnnotationNode specifically. It has the disadvantage of including a "dependency" on the annotation module. This is a hack ...

//-----------------
this->GetMRMLScene()->StartState(vtkMRMLScene::BatchProcessState);
for (int i = 0; i < numModels; i++)
{
vtkMRMLNode *modelNode = vtkMRMLModelNode::SafeDownCast(this->GetMRMLScene()->GetNthNodeByClass(i, "vtkMRMLModelNode"));
// HACK - Exclude vtkMRMLAnnotationNode subclasses.
// See http://www.na-mic.org/Bug/view.php?id=2576
if (modelNode != NULL
&& !vtkMRMLSliceLogic::IsSliceModelNode(modelNode)
&& !modelNode->IsA("vtkMRMLAnnotationNode"))
{
modelNode->SetDisplayVisibility(flag);
}
}
//-----------------

Ideally, a general mechanism should be implemented. Both approach are hacks ..

svn 21687[1] incorporates some of the suggestions from this bug.

[1] http://viewvc.slicer.org/viewvc.cgi/Slicer4?view=revision&amp;sortby=file&amp;revision=21687

Thanks for working on this patch.

All models on/off buttons have been added to Models gui.