0004040: fix vtkMRMLSceneImportIDModelHierarchyConflictTest - MantisBT

ID	Project	Category	View Status	Date Submitted	Last Update

0004040	Slicer4	Core: MRML	public	2015-09-04 06:49	2018-03-02 11:06

Reporter	nicole	Assigned To	nicole
Priority	normal	Severity	minor	Reproducibility	always
Status	closed	Resolution	fixed
Product Version
Target Version	Slicer 4.5.0-1	Fixed in Version	Slicer 4.5.0-1

Summary	0004040: fix vtkMRMLSceneImportIDModelHierarchyConflictTest
Description	After updating the MRML infrastructure to use a mini scene to load volumes and setting the MRML scene earlier on parsing nodes, the vtkMRMLSceneImportIDModelHierarchyConflictTest started failing. Test is failing as of: http://viewvc.slicer.org/viewvc.cgi/Slicer4?view=revision&revision=24402 First commit on this issue was: http://viewvc.slicer.org/viewvc.cgi/Slicer4?view=revision&revision=24370
Steps To Reproduce	ctest -R vtkMRMLSceneImportIDModelHierarchyConflictTest -VV
Additional Information	From email: I¹ve been digging into this one but haven¹t been able to isolate the change. The model display node¹s polydata is being reset to NULL during the node reference updates during ReadXMLAttributes, but comparing this version versus the branch I have from before the changes were merged, it looks like the processing is getting to the same place. The nodes in question only have a minor difference of the SceneRootDir having been removed, but the parser was updated to take that into account. The parser does set the MRML scene on the node earlier now, so some new/earlier events may be getting triggered, I¹m exploring that avenue now. I tried using an updated MRML snippet (the one in the test is versioned and from over 5400 commits ago, so it doesn¹t have the references entry), but that didn¹t fix it.
Tags	No tags attached.

nicole 2015-09-04 10:03 administrator ~0013256	Used git bisect to find the commit that broke the test: http://viewvc.slicer.org/viewvc.cgi/Slicer4?view=revision&revision=24393 That commit sets the scene on the storage node in the XML parser before reading it and moves the root dir information onto the scene from the node.

nicole 2015-10-06 11:21 administrator ~0013345 Last edited: 2015-10-06 11:24	Debugging with JC: we found that the MRML Parser was setting the scene on all nodes in StartElement while it was only needed on the storage nodes to resolve paths relative to the scene root directory. Having the scene set on the model node while parsing the xml, the reference id that points to the conflicting model display node id triggered a series of calls that tried to update the reference node id and cleared out the poly data on the original scene model display node. The larger problem is that the node references are being set/updated at the end of the vtkMRMLNode reading XML before the ID conflicts are resolved. We need to take another look at AddNodeReferenceID. References: ReadXMLAttributes called from vtkMRMLParser::StartElement: https://github.com/Slicer/Slicer/blob/0779d413e2957f809dda566d814eaba559304836/Libs/MRML/Core/vtkMRMLParser.cxx#L106-L110 AddNodeReferenceID called from ReadXMLAttributes: https://github.com/Slicer/Slicer/blob/0779d413e2957f809dda566d814eaba559304836/Libs/MRML/Core/vtkMRMLNode.cxx#L376-L395

jcfr 2015-10-06 20:37 administrator ~0013350	Fixed in r24608 See http://viewvc.slicer.org/viewvc.cgi/Slicer4?view=revision&revision=24608

Import 2017-06-07 23:51:09: master c56baf5f 2015-10-06 22:55:56 jcfr Details Diff	BUG: Do not reset reference when importing scene. Fixes 0004040 This commit fixes a regression introduced in r24393 [1] where a node and its referenced nodes would be modified if node with the same ID were imported. The issue is due to the MRML scene being set to the node just before the function "ReadXMLAttributes()" is called in "vtkMRMLParser::StartElement". Indeed, since within "ReadXMLAttributes()", the method "AddNodeReferenceID()" is called, when the ID of a node being imported conflicts with an existing one in the scene, it causes the existing node to be updated. For example, this problem is observed in test where a conflicting "vtkMRMLDisplayableNode" is imported. Other approach could be to ensure no events are invoked in the method "vtkMRMLDisplayableNode::OnNodeReferenceAdded" when scene is being imported. [1] r24393 - BUG: remove node SceneRootDir, use RootDirectory on MRML scene Co-authored-by: Nicole Aucoin <nicole@bwh.harvard.edu> git-svn-id: http://svn.slicer.org/Slicer4/trunk@24608 3bd1e089-480b-0410-8dfb-8563597acbee
	mod - Libs/MRML/Core/Testing/vtkMRMLSceneImportIDConflictTest.cxx		Diff File
	mod - Libs/MRML/Core/Testing/vtkMRMLSceneImportIDModelHierarchyConflictTest.cxx		Diff File
	mod - Libs/MRML/Core/vtkMRMLParser.cxx		Diff File

Date Modified	Username	Field	Change
2015-09-04 06:49	nicole	New Issue
2015-09-04 06:49	nicole	Status	new => assigned
2015-09-04 06:49	nicole	Assigned To	=> nicole
2015-09-04 10:03	nicole	Note Added: 0013256
2015-10-06 11:21	nicole	Note Added: 0013345
2015-10-06 11:24	jcfr	Note Edited: 0013345
2015-10-06 20:37	jcfr	Note Added: 0013350
2015-10-06 20:37	jcfr	Status	assigned => resolved
2015-10-06 20:37	jcfr	Fixed in Version	=> Slicer 4.5.0-1
2015-10-06 20:37	jcfr	Resolution	open => fixed
2017-06-10 08:51	jcfr	Changeset attached	=> Slicer master c56baf5f
2018-03-02 11:06	jcfr	Status	resolved => closed