`GraphEdit` minimap is not updated when `GraphNode` is deleted from the scene

allkhor commented 3 years ago

Godot version

3.3.4.stable, 3.4.rc3

System information

Ubuntu 16.04.7 LTS, GLES2, GLES3

Issue description

GrapEdit minimap not updated when graphnodes deleted from scene while not scroll or zoom graphedit.

Steps to reproduce

Open Mrp and run it. Press Delete Button. Look at minimap, graph nodes is gone, but on minimap you can see graph nodes.

Minimal reproduction project

minimap_mrp.zip

YuriSizov commented 3 years ago

Probably something to do with deferred execution. It should work if you actually remove it from the parent right there instead of relying on queue_free doing it for you later, I think.

allkhor commented 3 years ago

@pycbouh replacing queue_free with free solved this issue.

jakefriend-dev commented 2 years ago

Noting that if the trigger to remove the GraphNode is the GraphNode - IE, the on_close_request() signal is the node removing trigger as intended - you can't queue_free() because the GraphNode is still locked mid-emit.

This actually might be why this bug exists in the first place. I have a clear_graph() function that just loops through all GraphNode children and removes then from the scene, then queue_frees them. Minimap updates correctly. But my remove_graphnode() function triggered by a signal on the GraphNode does not update the minimap. My blind guess would be that the minimap also sees the GraphNode is locked, and moves on without a console message or similar.

jakefriend-dev commented 2 years ago

Pretty sure my theory here ^ is correct. I regret to inform you that this workaround works 😅

SirQuartz commented 1 year ago

Upon investigation, it would appear this issue can be resolved by calling minimap->queue_redraw() in GraphEdit when the remove_child_notify() function is called in the source code. This seems to already be done, but it's called inside a disconnect() method via a Callable.

https://github.com/godotengine/godot/blob/e13fae1414b0369fdd3f51b4e3529fd3f272b0e1/scene/gui/graph_edit.cpp#L444-L445

Not sure if somehow that's deferred or something, but it would explain why it doesn't immediately update on that frame. What I do know is that if you call queue_redraw() on minimap directly it will immediately update every time and seems to entirely solve this issue:

if (minimap != nullptr && minimap->is_inside_tree()) { 
    gn->disconnect("item_rect_changed", callable_mp((CanvasItem *)minimap, &GraphEditMinimap::queue_redraw)); 
        minimap->queue_redraw();

Feels a little redundant to do it that way though, since you're technically calling the function twice. My best guess is that the disconnect() function is deferred, thereby making the queue_redraw() deferred.

godotengine / godot