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

BUG: Fix nodes not getting removed in vtkMRMLSequenceNode::SetDataNodeAtValue #7335

Merged
merged 2 commits into from Nov 7, 2023

Conversation

Sunderlandkyl
Copy link
Member

When SetDataNodeAtValue is called, the new data node is deep-copied into the internal sequence scene, and replaces the old node in the index list if one exists. The old node however is not removed from the scene, causing the number of nodes to grow larger every time SetDataNodeAtValue was called for a value with an existing data node.

Fixed by removing the old node from the scene if one already exists for the specified value. This commit also changes the node pointer in IndexEntryType to a vtkWeakPointer.

Fixes #7328

lassoan
lassoan previously approved these changes Nov 7, 2023
Copy link
Contributor

@lassoan lassoan left a comment

Choose a reason for hiding this comment

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

This looks perfect, thank you!

Would be nice to add some tests in vtkMRMLSequenceNodeTest1.cxx but it can be done in a separate pull request.

…eAtValue

When SetDataNodeAtValue is called, the new data node is deep-copied into the internal sequence scene, and replaces the old node in the index list if one exists. The old node however is not removed from the scene, causing the number of nodes to grow larger every time SetDataNodeAtValue was called for a value with an existing data node.

Fixed by removing the old node from the scene if one already exists for the specified value.
This commit also changes the node pointer in IndexEntryType to a vtkWeakPointer.

Fixes Slicer#7328
…de scene

Adds additional tests to vtkMRMLSequenceNodeTest1 to check internal scene behavior.
Check that nodes are correctly added and removed from the internal scene in vtkMRMLSequenceNode.
@Sunderlandkyl
Copy link
Member Author

Added new tests in 280625b.

Copy link
Contributor

@lassoan lassoan left a comment

Choose a reason for hiding this comment

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

Great, thank you!

@lassoan lassoan enabled auto-merge (squash) November 7, 2023 20:53
@lassoan lassoan merged commit 17d23da into Slicer:main Nov 7, 2023
5 checks passed
@jcfr jcfr added the backport:5.x Identify pull request expected to be backported to the current 5.x release branch. label Nov 8, 2023
@Sunderlandkyl Sunderlandkyl deleted the seq_set_data_node_remove_node branch November 8, 2023 15:29
@jcfr jcfr removed the backport:5.x Identify pull request expected to be backported to the current 5.x release branch. label Nov 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Potential memory leak in Sequences module when items are deleted
3 participants