Skip to content

Fix Node deletion is too greedy #2619

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Daniel53245
Copy link

@Daniel53245 Daniel53245 commented Apr 25, 2025

Closes #2606

As mentioned in #2606 , in node editor mode, deleting nodes sometime results unwanted deletion of parent nodes.

This PR examines the algorithm for deletion and modified its behavior. Now, it will not cause the deletion in #2606, but I haven't tested it for other cases. I will put this as a draft PR till I am sure it works for other conditions

@Daniel53245
Copy link
Author

Fix Node Deletion is too greedy

Original Code

https://github.com/GraphiteEditor/Graphite/blob/d39308c048cc439715943ff487294ffccaf12c70/editor/src/messages/portfolio/document/utility_types/network_interface.rs#L4177//

The original code for greedy node deletion was a graph search algorithm in rust. It iterates each upstream node for the nodes marked for deletion.

For one of the upstream node it checks all the child it connected to

  1. the child is marked for deletion, search in this branch is stopped (will result deletion of upstream node if all the childs falls in this category)
  2. the child is not marked for deletion, continue search the grandchildren

Cause of the original bug

As mentioned in the original issue, following images describes one of the condition that the algorithm fails and deleted "Identity" node while it is used by other nodes
435906647-59440fc0-815d-4419-b154-5b823d1eb711
435906694-03cc8024-4192-4e85-a163-bca175cc3f21

During the deletion, the original algorithm check all the downstream node for the "Identity", it confirms that "equals is marked for deletion by the user. The algorithm searches through "Greater Than" and "Switch". A the point it reaches "Switch", the issue happened.

The line

let Some(downstream_nodes) = outward_wires.get(&current_node) else { continue };

"Switch" node does not have any child, this line continues the loop. The algorithm ignores "Switch" and mistake that all downstream nodes of "Identity" are marked for deletion

Modification

The original algorithm does a recursive search through the graph. I modified that algorithm such that it would mark the upstream node not for deletion once it reach a non-deletion downstream node. This is achieve by modifying line

if !delete_nodes.contains(downstream_id) {
	stack.push(downstream_node_output);
}

to

if !delete_nodes.contains(downstream_id) {
	can_delete = false;
	break;
}

Test of the modified code

I tested modified code in many situations

image
Deletion of the node will not cause deletion of any other node

image
Deletion of "Switch" cause both upstream node deleted

image
Deletion of "Logical Or" will not delete any other node

image
Sandwiched deletion will not cause the middle node to be deleted

image
Deleting last node of the chain will cause the deletion of all upstream nodes

@Daniel53245 Daniel53245 marked this pull request as ready for review April 27, 2025 09:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Node deletion is too greedy
1 participant