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

[GraphEdit] Correctly disconnect signal to connection_layer #94810

Merged
merged 1 commit into from
Jul 30, 2024

Conversation

maidopi-usagi
Copy link
Contributor

@maidopi-usagi maidopi-usagi commented Jul 26, 2024

Fix #94811

@maidopi-usagi maidopi-usagi requested a review from a team as a code owner July 26, 2024 17:35
@AThousandShips AThousandShips added this to the 4.4 milestone Jul 26, 2024
@AThousandShips AThousandShips added cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release and removed cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release labels Jul 26, 2024
@AThousandShips AThousandShips modified the milestones: 4.4, 4.3 Jul 26, 2024
@AThousandShips AThousandShips requested a review from Geometror July 26, 2024 17:48
@akien-mga akien-mga changed the title [GraphEdit] Correctly disconnect signal to connection_layer [GraphEdit] Correctly disconnect signal to connection_layer Jul 29, 2024
@akien-mga akien-mga merged commit 5271a39 into godotengine:master Jul 30, 2024
18 checks passed
@akien-mga
Copy link
Member

Thanks! And congrats for your first merged Godot contribution 🎉

@Naros
Copy link
Contributor

Naros commented Jan 31, 2025

@akien-mga and @Geometror are we sure this code change is correct?

I've built this locally against master, and I'm am noticing that this leads to this error.

ERROR: Attempt to disconnect a nonexistent connection from '[Wrapped:0]'. Signal: 'item_rect_changed', callable: 'CanvasItem::queue_redraw'.
   at: (core\object\object.cpp:1529)

In our extended GraphEdit implementation, we have two notification handlers, one for NOTIFICATION_READY and another for NOTIFICATION_THEME_CHANGED. In both of these cases, we have a handler that fires that adds/removes nodes from GraphEdit.

The theme changed event is what seems to create the issue where the remove_child_notify indicates that the item_rect_changed signal isn't connected yet to the CanvasItem::queue_redraw handler.

The handler that both READY and THEME_CHANGED call basically removes nodes like this:

for (GraphNode* node : node_list) {
  remove_child(node);
  node->queue_free();
}

It's this that creates an issue during theme changed which wasn't an issue in Godot 4.3.stable.

What's even more bizarre is this only manifests if the UI has two GraphEdit instances open with two different graphs. If there is only one GraphEdit open, there's no issue.

@Naros
Copy link
Contributor

Naros commented Jan 31, 2025

If I wrap the call in the theme notification as:

case NOTIFICATION_THEME_CHANGED: 
  if (is_node_ready()) {
    // call my custom synchronize met
    _synchronize_graph_with_script();
  }
  break;

Then I don't see the issue, its only if the implementation adds/removes nodes before the node is ready that the calls to the add/remove child notify handlers lead to the signal not yet being connected.

@maidopi-usagi
Copy link
Contributor Author

maidopi-usagi commented Jan 31, 2025

It's this that creates an issue during theme changed which wasn't an issue in Godot 4.3.stable.

@Naros Thanks for reporting! I'm wondering if the problem you're encountering might be related to this specific commit, as this PR has already been contained in 4.3.stable build. Would you mind opening a separate issue and provide additional details about your testing environment and reproduction steps?

@Naros
Copy link
Contributor

Naros commented Feb 1, 2025

Hmm @maidopi-usagi in that case, I wonder if the issue is some change in Godot 4.4 that breaks this commit. I nor none of our user community saw this on Godot 4.3.stable 🤔. Let me continue to check and once I know for sure, I'll open another issue.

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.

[GraphEdit] GraphElement's 'item_rect_changed' signal is not disconnected when leaving tree
5 participants