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

Fix GraphEdit layering #61783

Conversation

Geometror
Copy link
Member

This PR fixes/improves the ordering of comment nodes as children of GraphEdit as well as the position of the connections layer.
Split from #61414.

Detailed changes:

  • the order of comment nodes is now calculated similar to a 2D BVH: when a comment node encloses other comment nodes, the index of the enclosing node is kept below
    • the order is updated when resizing a comment node
  • the connections layer is now kept between the comment nodes and the normal graph (due to technical limitations it is no longer an internal node)

Before:
before

This PR:
after

@Geometror Geometror requested review from a team as code owners June 7, 2022 16:49
@Geometror Geometror added this to the 4.0 milestone Jun 7, 2022
@Geometror Geometror force-pushed the graphedit-layering-improvements branch 2 times, most recently from ee5a92f to 6c8b44c Compare June 7, 2022 17:34
@fire
Copy link
Member

fire commented Jun 8, 2022

The nesting in GraphEdit::_top_layer_input is very deep. will review more.

@reduz
Copy link
Member

reduz commented Aug 6, 2022

This looks good to me, what is it missing?

@akien-mga
Copy link
Member

This looks good to me, what is it missing?

Mostly a contributor that feels confident enough with GraphEdit to approve the feature and implementation :)

@fire
Copy link
Member

fire commented Aug 6, 2022

I'll take a look. Can it be rebased and ready to merge?

@Geometror Geometror force-pushed the graphedit-layering-improvements branch from 6c8b44c to 2368202 Compare August 7, 2022 10:32
@Geometror
Copy link
Member Author

@fire Rebased.
Just for reference: For now(4.0) I think this will do, but since it is a bit hacky I would like to refactor the current GraphNode/GraphEdit system including the restructuring(#61414) and implementation of layers (or something similar) in the next few months after I opened a proper proposal. A problem might be compatibility breakage so that needs to be discussed.

@fire
Copy link
Member

fire commented Aug 27, 2022

There's some conflicts.

@Geometror Geometror force-pushed the graphedit-layering-improvements branch from 2368202 to 1ff9363 Compare September 6, 2022 13:54
Comment on lines +411 to +412
_update_comment_enclosed_nodes_list(gn, comment_enclosed_nodes);
_graph_node_raised(p_gn);
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't it use the new method?

Comment on lines 1294 to +1295
_set_drag_comment_enclosed_nodes(gn, comment_enclosed_nodes, false);
_reorder_comment_nodes(gn);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a bit confused by the order of operations. In the below snippet we update enclosed nodes after the reorder, and here we do before. It's a bit hard to follow the logic, so can you tell if this is intentional?

@@ -399,7 +430,18 @@ void GraphEdit::add_child_notify(Node *p_child) {
gn->connect("selected", callable_mp(this, &GraphEdit::_graph_node_selected).bind(gn));
gn->connect("deselected", callable_mp(this, &GraphEdit::_graph_node_deselected).bind(gn));
gn->connect("slot_updated", callable_mp(this, &GraphEdit::_graph_node_slot_updated).bind(gn));

if (gn->is_comment()) {
background_nodes_separator_idx++;
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we actually need to track this? Can't we update connections layer's index along the comments? And in _graph_node_raised we can probably just refer to the connection layer's index.

I'm just concerned that adding a tracker like that would make the code too fragile.

@YuriSizov
Copy link
Contributor

So, from some quick testing this doesn't seem to work completely as expected. For a new shader the comments are still in front of the connections:
image

But closing and reopening the file fixes it, even for the nodes added afterwards:
image

Am I doing something wrong?

@akien-mga
Copy link
Member

Superseded by #67152.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants