Disconnect graph connections when ports are removed in visual shaders

[EddieBreeg]

For VectorCompose and VectorDecompose nodes, the 'changed' signal is connected to two new functions, which handle the deletion of connections from/to deleted ports.

Note: this revealed a new bug, described in #79417 (comment).
I don't see this new issue being directly caused by the code I wrote, and since this is my first ever contribution to this project, I am not too familiar with the code, so I decided to go ahead with the PR anyway.

Bugsquad edit, fixes #79417

[EddieBreeg]

Note: this PR is a re creation of another one I made two weeks ago, which you can find here. The original PR had changes applied directly on the master branch, instead of a separate one.

[YuriSizov]

Some notes about the style, but my main concern is the overall approach.

While the #79417 mentions a couple of specific nodes, isn't this a more general problem? A number of visual shader nodes can change the number of their input and output connections. So a solution should be generic as well.

We need a new signal, and maybe a more robust way to track if the number of ports was changed, or we can just emit the signal manually every time this happens in any node. Then you can connect to that signal instead. And you probably don't need 2 different methods, you can check both sides in one.

[YuriSizov]

This is incorrect for several reasons. First of all, the signal means that the resource has been changed for any reason, which is always true by this line since we rule out the new and the old op type being the same at the start of this method.

And second, your proposed change assumes that the order of enum values would somehow correspond to the number of available ports. While it's true in this case, it's a weak relation and such code can very easily lead to annoying mistakes and bugs. We could discuss the merits of it, but as mentioned above there is no need for this change at all.

[EddieBreeg]

The check here is not incorrect, although admittedly I should have chosen another name for the boolean. What I am testing specifically is whether the number of ports has decreased, not simply changed, because we do not need to do anything when the port count increased and we do not have any way to know which type of change it was when handing the signal

[YuriSizov]

Well, the signal is there to indicate that the resource has changed, whether the number of ports increased, decreased, or remained the same, it's not relevant. The signal is emitted because a new value for the op type was set.

[EddieBreeg]

Yeah as I said, I agree this is by no means ideal. I'd be happy to create a new signal instead, I just didn't know how and this one happened to be disconnected. Do you know how to do that by any chance?

[YuriSizov]

To add a new signal you need to use the ADD_SIGNAL macro in the _bind_methods method of the class that you want to add it to. In this case that would probably be VisualShaderNode. Then just emit it with emit_signal("your_signal_name") and connect to it normally.

[EddieBreeg]

Sorry about all the unnecessary includes I think clangd added them for some reason.

Normally I would agree that the issue is a bit more general, but on the other hand the contribution guidelines explicitly discourage to try implementing general solutions so I wasn't quite sure what to do. Which is also the reason I made the assumption about the enum values corresponding to the port count, both because it made the code simpler and because having any other way would arguably be weird.

I do agree adding another signal is the way to go, I tried and failed to do so (yes I still have much to learn about how the code base works), and since this signal was free I simply used it.

[YuriSizov]

the contribution guidelines explicitly discourage to try implementing general solutions

Yes, that's correct, but doesn't apply to this situation. General solution that is discouraged would be something that doesn't have a specific target in mind or has a very broad set of targets. In this case you are dealing with one thing — visual shader nodes, some of which can dynamically shed some of their ports, and we need to handle it properly. Fixing the issue for just the two nodes and leaving the problem open for others potentially affected doesn't make much sense.

[AThousandShips]

Suggested change

	if (!node.is_valid())
	return;
	if (!node.is_valid()) {
	return;
	}

Code style, we use brackets always for if-statements

[AThousandShips]

Suggested change

	const int inputs = node->get_input_port_count(),
	outputs = node->get_output_port_count();
	const int inputs = node->get_input_port_count();
	const int outputs = node->get_output_port_count();

Readability

[Chaosus]

@EddieBreeg I've checked it, currently, if you use undo (CTRL+Z) on that action leaves the changed connection disconnected. That should not be - the solution is to record the disconnection/connection actions by UndoRedo instead of doing it through signal.

[EddieBreeg]

Here the signal is required anyway but I'll add the UndoRedo record

[EddieBreeg]

I am very confused right now. Based on other examples in the code I tried creating an action and committing it, like so:

void VisualShaderEditor::_node_ports_removed(int p_id) {
	VisualShader::Type shader_type = visual_shader->get_shader_type();
	Ref<VisualShaderNode> node = visual_shader->get_node(shader_type, p_id);
	if (unlikely(!node.is_valid()))
		return;
	const int inputs = node->get_input_port_count();
	const int outputs = node->get_output_port_count();
	List<VisualShader::Connection> conns;
	visual_shader->get_node_connections(shader_type, &conns);
	EditorUndoRedoManager *undo_redo = EditorUndoRedoManager::get_singleton();
	undo_redo->create_action(TTR("Remove node ports"));
	for (const VisualShader::Connection &c : conns) {
		if (unlikely((c.from_node == p_id && c.from_port >= outputs) || (c.to_node == p_id && c.to_port >= inputs))) {
			// graph_plugin->disconnect_nodes(shader_type, c.from_node, c.from_port, c.to_node, c.to_port);
			// visual_shader->disconnect_nodes(shader_type, c.from_node, c.from_port, c.to_node, c.to_port);

			undo_redo->add_do_method(visual_shader.ptr(), "disconnect_nodes", shader_type, c.from_node, c.from_port, c.to_node, c.to_port);
			undo_redo->add_do_method(graph_plugin.ptr(), "disconnect_nodes", shader_type, c.from_node, c.from_port, c.to_node, c.to_port);
			undo_redo->add_undo_method(visual_shader.ptr(), "connect_nodes", shader_type, c.from_node, c.from_port, c.to_node, c.to_port);
			undo_redo->add_undo_method(graph_plugin.ptr(), "connect_nodes", shader_type, c.from_node, c.from_port, c.to_node, c.to_port);
		}
	}
	undo_redo->commit_action();
}

But the editor crashes as a result...

[Chaosus]

But the editor crashes as a result...

Suggested change

	vsnode->connect("node_ports_removed", callable_mp(editor, &VisualShaderEditor::_node_ports_removed).bind(p_id));
	vsnode->connect("node_ports_removed", callable_mp(editor, &VisualShaderEditor::_node_ports_removed).bind(p_id), CONNECT_DEFERRED);

I think you should try to use CONNECT_DEFERRED flag to prevent that crash.

[EddieBreeg]

I'll give it a shot thank you

[EddieBreeg]

It did fix the crash! I have another issue though: the op_type change and the connections removal are not in the same action, which means that hitting ctrl+z only recreates the connections without reverting to the previous op_type. As far as I can understand, handling removed ports should be done in _property_changed function?

[Chaosus]

Then unify it to a single action, (CONNECT_DEFERRED will not be needed in that case), remove create_action/commit_action from the _node_ports_removed and uses the same action as usual just call it before the commit_action of the op_type change function has been called.

[EddieBreeg]

Yes that's what I was proposing: the commit_action gets called in VisualShaderNodePluginDefaultEditor::_property_changed so all I need to do is check for invalid connections there.
It means that invalid connections will be checked everytime a property gets called whether this property has an impact on the number of ports or not. On the other hand, it also means that no matter what happens in the future invalid connections will always be removed as they should be

[Geometror]

Sorry for the long wait. It looks like this brach doesn't contain your most recent changes (undo/redo). Could you push those?

[akien-mga]

Superseded by #89810. Thanks for the contribution!