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.

Sign up for GitHub

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

Disconnect graph connections when ports are removed in visual shaders #80312

Closed
wants to merge 1 commit into from
Closed

Disconnect graph connections when ports are removed in visual shaders #80312

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 25 additions & 0 deletions editor/plugins/visual_shader_editor_plugin.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -440,6 +440,10 @@ void VisualShaderGraphPlugin::add_node(VisualShader::Type p_type, int p_id, bool
custom_node->_set_initialized(true);
}

if (!p_just_update) {
vsnode->connect("node_ports_removed", callable_mp(editor, &VisualShaderEditor::_node_ports_removed).bind(p_id));
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll give it a shot thank you

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

}

// Create graph node.
GraphNode *node = memnew(GraphNode);
graph->add_child(node);
Expand Down Expand Up @@ -2661,6 +2665,27 @@ void VisualShaderEditor::_port_edited(const StringName &p_property, const Varian
undo_redo->commit_action();
}

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 (!node.is_valid())
return;
Comment on lines +2671 to +2672
Copy link
Member

Choose a reason for hiding this comment

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

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

Code style, we use brackets always for if-statements

const int inputs = node->get_input_port_count(),
outputs = node->get_output_port_count();
Comment on lines +2673 to +2674
Copy link
Member

Choose a reason for hiding this comment

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

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

List<VisualShader::Connection> conns;
visual_shader->get_node_connections(shader_type, &conns);
for (const VisualShader::Connection &c : conns) {
if (unlikely(c.from_node == p_id && c.from_port >= outputs)) {
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);

} else if (unlikely(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);
}
}
}

void VisualShaderEditor::_edit_port_default_input(Object *p_button, int p_node, int p_port) {
VisualShader::Type type = get_current_shader_type();
Ref<VisualShaderNode> vs_node = visual_shader->get_node(type, p_node);
Expand Down
2 changes: 2 additions & 0 deletions editor/plugins/visual_shader_editor_plugin.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -387,6 +387,8 @@ class VisualShaderEditor : public VBoxContainer {
void _edit_port_default_input(Object *p_button, int p_node, int p_port);
void _port_edited(const StringName &p_property, const Variant &p_value, const String &p_field, bool p_changing);

void _node_ports_removed(int p_id);

int to_node = -1;
int to_slot = -1;
int from_node = -1;
Expand Down
2 changes: 2 additions & 0 deletions scene/resources/visual_shader.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -406,6 +406,8 @@ void VisualShaderNode::_bind_methods() {
ADD_PROPERTY(PropertyInfo(Variant::ARRAY, "default_input_values", PROPERTY_HINT_NONE, "", PROPERTY_USAGE_NO_EDITOR | PROPERTY_USAGE_INTERNAL), "set_default_input_values", "get_default_input_values");
ADD_PROPERTY(PropertyInfo(Variant::ARRAY, "expanded_output_ports", PROPERTY_HINT_NONE, "", PROPERTY_USAGE_NO_EDITOR | PROPERTY_USAGE_INTERNAL), "_set_output_ports_expanded", "_get_output_ports_expanded");

ADD_SIGNAL(MethodInfo("node_ports_removed"));

BIND_ENUM_CONSTANT(PORT_TYPE_SCALAR);
BIND_ENUM_CONSTANT(PORT_TYPE_SCALAR_INT);
BIND_ENUM_CONSTANT(PORT_TYPE_SCALAR_UINT);
Expand Down
8 changes: 8 additions & 0 deletions scene/resources/visual_shader_nodes.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4936,7 +4936,11 @@ void VisualShaderNodeVectorCompose::set_op_type(OpType p_op_type) {
default:
break;
}
bool ports_removed = p_op_type < op_type;
op_type = p_op_type;
if (ports_removed) {
emit_signal("node_ports_removed");
}
emit_changed();
}

Expand Down Expand Up @@ -5100,7 +5104,11 @@ void VisualShaderNodeVectorDecompose::set_op_type(OpType p_op_type) {
default:
break;
}
bool ports_removed = p_op_type < op_type;
op_type = p_op_type;
if (ports_removed) {
emit_signal("node_ports_removed");
}
emit_changed();
}

Expand Down