What are the properties of such "ModelHierarchy" node ?
E.g.
What is its classname ?
What is the value of GetHiddenFromEditors?
Does it have an associated node ?
Does it have children ?
Why do you think it should not be visible in Models module ?

It's a vtkMRMLModelHierarchyNode
HideFromEditors is 1
Yes
No
Because it wasn't before. :) It's what I've called the 1:1 hierarchy node that just provides the hook for the model node to be plugged into a hierarchy.

It's visible because the node is Hidden but the tree view show hidden nodes (https://github.com/Slicer/Slicer/blob/master/Modules/Loadable/Models/qSlicerModelsModuleWidget.cxx#L142-143).

So why do you think the Models module shouldn't display the node?
Should all hierarchy nodes with no children but with associated nodes be hidden to the view?

For the 1:1 model hierarchy nodes (no children, just associated nodes), they've always been set as Hidden but they weren't shown before in the Models GUI. (see ModelsModuleHierarchyBug-AbdominalAtlasBefore.jpg)
This is a recent change to the GUI that makes the model hierarchies look confusing (see second attached image from the abdominal atlas ModelsModuleHierarchyBug-AbdominalAtlas.jpeg). I can drag either the model heirarchy node or the model node to move models into a new hierarchy, but when I drag the one, the other moves automatically and the reason isn't clear.

For the models module, yes, if the model heirarchy node has the hidden flag on, it has an associated node id and no other hierarchy nodes have it as a parent, it shouldn't be visible in the tree. If someone drags a model to be a child of another model, creating a child on the 1:1 model heirarchy node, I'm fine with it staying hidden and just having the visible tree view structure reflect the hierarchy.
The hierarchy nodes added by right clicking should probably not be hidden, that can be changed so that hidden is used to really mean don't show it in the tree views anywhere (the hide from editors flag tends to trip me up as it's on by default).

If it is the case: "all hierarchy nodes with no children but with associated nodes are hidden to the view", then let's take the following example:
A hierarchy model node

• has the hidden flag
• has an associated node
• has 1 child
  it is then visible, now we remove its child:
• it becomes hidden.

I think we should simply work with the hidden flag.
All the nodes that must be visible by the Models module view should not have the HiddenFromEditors flag.
What do you think ?

I'm fine with the example as given, but we should check with Marianna and Ron to be sure that works for them.
I think that using the hidden flag makes sense going forward, but most (all?) of the legacy scenes that we use for the atlases will then display incorrectly.

We can do something similar to:
https://github.com/Slicer/Slicer/blob/master/Libs/MRML/Core/vtkMRMLParser.cxx#L116-121

I had a quick chat with Ron, he said that since the scenes that we're talking about aren't that numerous (only the atlases), we could fix them on a case by case basis. He also says that "simpler is better" in terms of the display and having those extra hierarchy nodes showing isn't good.
With the path of fixing the old mrml scenes open, the change I'd want to make in the models module is to set the tree to hide hierarchy nodes that have the hide from editors flag set to true and fix any fall out that results in the tree not looking/working as expected.

I chatted with Marianna, she needs drag and drop to work and no extra model hierarchy nodes cluttering up the UI.

Changes needed to fix the models gui starting from current code:

Libs/MRML/Core/vtkMRMLModelHierarchyNode.cxx - set hide from editors to false
Modules/Loadable/Models/Resources/UI/qSlicerModelsModuleWidget.ui - set showHidden to false
Modules/Loadable/Models/qSlicerModelsModuleWidget.cxx - take out call to setShowHiddenForTypes
Libs/MRML/Widgets/qMRMLSceneHierarchyModel.cxx - set hidden true on new 1:1 hierarchy nodes
Modules/CLI/ModelMaker/ModelMaker.cxx - ditto
Base/QTCLI/qSlicerCLIModuleUIHelper.cxx - showHidden is set to true if multiple && aggregate, but it's also used to set a nodeType. Setting show hidden to false and setting the node type by directly checking multiple and aggregate.

Alternately: keep the default being hide from editors is true on the hierarchy node and explicitly set it to false when necessary. Either way, it would probably best to be explicitly setting hide from editors on new hierarchy nodes.

The changes above seem to work, checking them in... see svn 21657.

Marianna will test the nightly build on Monday and leave feedback.

I agree with Nicole, the low level hierarchy nodes serve as "links" from hierarchy to models, and have no meaning to the user. They should not be visible to the user.

Following r21657, should this issue be resolved ?
See http://viewvc.slicer.org/viewvc.cgi/Slicer4?view=revision&revision=21657

I've had feedback from Marianna: the head and neck atlas as well as the brain atlas are showing no models in the Models gui after r21657. The abdominal atlas is okay. The knee atlas is also fine.
I'm writing a python script to see if I can automate the resetting of the hide from editors flag on model hierarchy nodes so that we can quickly fix the head&neck and brain atlas files.

How about we add a fix when loading those scenes instead ?
Right now, MRML scenes are saved with a "version" attribute for the "MRML" element. Such value is "Slicer4" but I think it should instead be "4.2.2" or anything more precise than Slicer4.
That way, we can turn the visibility on for all the hierarchy nodes in scenes older than 4.2.3 that are not a 1:1 hierarchy node.

@Julien: Make sens. Thanks for pointing this out.
@Nicole: What would it take to follow the approach mentioned by Julien ?

Julien's proposal requires changing the way the mrml files are versioned as well as putting in a special case in the parser. If the versioning can be tied to the "official" version so that it only has to be updated in one place each time, I'm fine with going that route. Otherwise I'm fine with updating the mrml files on a case by case basis.
(we used to version the mrml files by svn revision)

I found an odd thing as I was making the script: after running this in the python terminal, the Models GUI didn't update to show the new settings. I had to save the .mrml file and reload it to see that it had worked:
numNodes = slicer.mrmlScene.GetNumberOfNodesByClass("vtkMRMLModelHierarchyNode")
for h in range(numNodes):
mh = slicer.mrmlScene.GetNthNodeByClass(h, "vtkMRMLModelHierarchyNode")
if mh.GetNumberOfChildrenNodes() > 0:
print "Showing", mh.GetName()
mh.SetHideFromEditors(0)
if mh.GetNumberOfChildrenNodes() == 0 and mh.GetAssociatedNodeID() != None:
print "Hiding", mh.GetName()
mh.SetHideFromEditors(1)

The odd behavior is due to an optimization of the models.
https://github.com/Slicer/Slicer/blob/master/Libs/MRML/Widgets/qMRMLSortFilterProxyModel.cxx#L223
When the node has the flag HideFromEditors to 1, then the model doesn't observe the node to see if such flag has changed.
My understanding was that such property are not meant to change within the entire life of a node.
If it appear to be the case, then line 223 could be changed to RejectButPotentiallyAcceptable (however it would have to be further refined. Don't return RejectButPotentiallyAcceptable if the node type is wrong).

@Julien/nicole: If the property is expected to be read-only, no setter should exist. If a setter exist, we should assume it can be updated using the python console.

@nicole: Updating the mrml on case by case is counter productive. Yes, the versioning of MRML should be associated with a specific version set at one single location. I would vote for a MRML specific version that we would set in the Libs/MRML/Core/CMakeLists.txt

It's more of a developer action to change that flag while debugging a module. I'm a bit worried that with the optimisations that you can get into a state where you want the tree to update but it's not possible to do. Maybe add a hot key or only in python "force update" operation to the trees?

for the tree views, there is the undocumented '!' key :-)

If such change are made only from python, waiting time shouldn't be an issue. I would suggest we keep things simple and avoid "forceupdate", "hot key", ... etc

The optimization done by Julien was done with an implicit knowledge, we should either:

• document the optimization + (add warning in the setter ?)
• remove the optimization
• document + enforce the optimization by construction/design (removing the setter)

If such case is reserved to a handful of developer, documenting should be enough.

The main reason I'm hesitating over removing the Set call is that the mrml nodes have hide from editors set to true by default. Every time a developer makes a new mrml node, they have to decide if they want their node to show up in drop down menus and in mrml tree views and in any other node lists. Sometimes they want them in one but not the other(s) and use the show hidden flag. The change that triggered this bug report has led to a change in Slicer's default behaviour in the trees that resulted in my changes to work around those changes, so I'd like to add this topic to the developer hangout tomorrow, I'd like to get Steve and Alex's input on it before we make even more changes.

Documentation has been added in r21661.
I believe nodes should be visible by default (HideFromEditors = 0).
Maybe the behavior of HideFromEditors could be extended. (a list of tags instead of being only a on/off value).

Is this bug fixed ? I saw Nicole fixed the parent issue.

Looks like the original bug is fixed, can't reproduce it in the current svn trunk.

Closing resolved issues that have not been updated in more than 3 months

Hide from editors default change commited as svn 22937:
http://viewvc.slicer.org/viewvc.cgi/Slicer4?view=revision&revision=22937

Quick test to instatiate all but slice nodes:
numNodeClasses = slicer.mrmlScene.GetNumberOfRegisteredNodeClasses()
for i in range(numNodeClasses):
className = slicer.mrmlScene.GetNthRegisteredNodeClass(i).GetClassName()
print className
if className != 'vtkMRMLSliceNode':
newNode = slicer.mrmlScene.CreateNodeByClass(className)
newNode.SetName('test node ' + className)
slicer.mrmlScene.AddNode(newNode)

See NotHiddenFromEditorsDataGUI attachment.

Pull request to change Slicer MRML core nodes:
https://github.com/Slicer/Slicer/pull/428
Pull request to change OpenIGTLinkIF nodes:
https://github.com/openigtlink/OpenIGTLinkIF/pull/59

OpenIGTLinkIF fix is in:
https://github.com/openigtlink/OpenIGTLinkIF/commit/32fd763272f728b8885f0f01b7ecae4ff6cebe44
Slicer MRML core nodes fix is in:
http://viewvc.slicer.org/viewvc.cgi/Slicer4?view=revision&revision=24995

Based on the comments, the issue seems to have been fixed.