godotengine / godot

Godot Engine – Multi-platform 2D and 3D game engine
https://godotengine.org
MIT License
89.15k stars 20.21k forks source link

Copying a node with signals to another scene can cause crash. #96318

Open OffsetMOSFET opened 3 weeks ago

OffsetMOSFET commented 3 weeks ago

Tested versions

4.3.1.rc

System information

Ubuntu 22.04.4 LTS 64-bit

Issue description

Copying a node with signal into another scene will cause common_parent is null errors and similar errors.

Trying to edit the signal in the new scene via the context menu causes a segfault.

ERROR: Index p_line = 14 is out of bounds (get_line_count() = 12).
   at: unfold_line (scene/gui/code_edit.cpp:1655)
ERROR: Cannot get path of node as it is not in a scene tree.
   at: get_path (scene/main/node.cpp:2257)
ERROR: Parameter "common_parent" is null.
   at: get_path_to (scene/main/node.cpp:2192)
ERROR: Node not found: "" (relative to "/root/@EditorNode@16886/@Panel@13/@VBoxContainer@14/DockHSplitLeftL/DockHSplitLeftR/DockHSplitMain/@VBoxContainer@25/DockVSplitCenter/@VSplitContainer@52/@VBoxContainer@53/@PanelContainer@98/MainScreen/@CanvasItemEditor@9272/@VSplitContainer@9094/@HSplitContainer@9096/@HSplitContainer@9098/@Control@9099/@SubViewportContainer@9100/@SubViewport@9101/PopupDemo/HSplitContainer/VBoxContainer/LineEdit").
   at: get_node (scene/main/node.cpp:1792)

================================================================
handle_crash: Program crashed with signal 11
Engine version: Godot Engine v4.3.1.rc.custom_build (ff9bc0422349219b337b015643544a0454d4a7ee)
Dumping the backtrace. Please include this when reporting the bug to the project developer.
[1] /lib/x86_64-linux-gnu/libc.so.6(+0x42520) [0x799864042520] (??:0)
[2] ConnectDialog::_update_warning_label() (/home/leonard/Git/godot/editor/connections_dialog.cpp:455)
[3] ConnectDialog::_tree_node_selected() (/home/leonard/Git/godot/editor/connections_dialog.cpp:178)
[4] void call_with_variant_args_helper<ConnectDialog>(ConnectDialog*, void (ConnectDialog::*)(), Variant const**, Callable::CallError&, IndexSequence<>) (/home/leonard/Git/godot/./core/variant/binder_common.h:309 (discriminator 4))
[5] void call_with_variant_args<ConnectDialog>(ConnectDialog*, void (ConnectDialog::*)(), Variant const**, int, Callable::CallError&) (/home/leonard/Git/godot/./core/variant/binder_common.h:419)
[6] CallableCustomMethodPointer<ConnectDialog>::call(Variant const**, int, Variant&, Callable::CallError&) const (/home/leonard/Git/godot/./core/object/callable_method_pointer.h:104)
[7] Callable::callp(Variant const**, int, Variant&, Callable::CallError&) const (/home/leonard/Git/godot/core/variant/callable.cpp:57)
[8] Object::emit_signalp(StringName const&, Variant const**, int) (/home/leonard/Git/godot/core/object/object.cpp:1188)
[9] Node::emit_signalp(StringName const&, Variant const**, int) (/home/leonard/Git/godot/scene/main/node.cpp:3895)
[10] Error Object::emit_signal<>(StringName const&) (/home/leonard/Git/godot/./core/object/object.h:936)
[11] SceneTreeEditor::set_selected(Node*, bool) (/home/leonard/Git/godot/editor/gui/scene_tree_editor.cpp:1023)
[12] ConnectDialog::set_dst_node(Node*) (/home/leonard/Git/godot/editor/connections_dialog.cpp:526)
[13] ConnectDialog::init(ConnectDialog::ConnectionData const&, Vector<String> const&, bool) (/home/leonard/Git/godot/editor/connections_dialog.cpp:646)
[14] ConnectionsDock::_open_edit_connection_dialog(TreeItem&) (/home/leonard/Git/godot/editor/connections_dialog.cpp:1157)
[15] ConnectionsDock::_handle_slot_menu_option(int) (/home/leonard/Git/godot/editor/connections_dialog.cpp:1254)
[16] void call_with_variant_args_helper<ConnectionsDock, int, 0ul>(ConnectionsDock*, void (ConnectionsDock::*)(int), Variant const**, Callable::CallError&, IndexSequence<0ul>) (/home/leonard/Git/godot/./core/variant/binder_common.h:309 (discriminator 4))
[17] void call_with_variant_args<ConnectionsDock, int>(ConnectionsDock*, void (ConnectionsDock::*)(int), Variant const**, int, Callable::CallError&) (/home/leonard/Git/godot/./core/variant/binder_common.h:419)
[18] CallableCustomMethodPointer<ConnectionsDock, int>::call(Variant const**, int, Variant&, Callable::CallError&) const (/home/leonard/Git/godot/./core/object/callable_method_pointer.h:104)
[19] Callable::callp(Variant const**, int, Variant&, Callable::CallError&) const (/home/leonard/Git/godot/core/variant/callable.cpp:57)
[20] Object::emit_signalp(StringName const&, Variant const**, int) (/home/leonard/Git/godot/core/object/object.cpp:1188)
[21] Node::emit_signalp(StringName const&, Variant const**, int) (/home/leonard/Git/godot/scene/main/node.cpp:3895)
[22] Error Object::emit_signal<int>(StringName const&, int) (/home/leonard/Git/godot/./core/object/object.h:936)
[23] PopupMenu::activate_item(int) (/home/leonard/Git/godot/scene/gui/popup_menu.cpp:2438)
[24] PopupMenu::_input_from_window_internal(Ref<InputEvent> const&) (/home/leonard/Git/godot/scene/gui/popup_menu.cpp:637)
[25] PopupMenu::_input_from_window(Ref<InputEvent> const&) (/home/leonard/Git/godot/scene/gui/popup_menu.cpp:447)
[26] Window::_window_input(Ref<InputEvent> const&) (/home/leonard/Git/godot/scene/main/window.cpp:1675)
[27] void call_with_variant_args_helper<Window, Ref<InputEvent> const&, 0ul>(Window*, void (Window::*)(Ref<InputEvent> const&), Variant const**, Callable::CallError&, IndexSequence<0ul>) (/home/leonard/Git/godot/./core/variant/binder_common.h:304 (discriminator 4))
[28] void call_with_variant_args<Window, Ref<InputEvent> const&>(Window*, void (Window::*)(Ref<InputEvent> const&), Variant const**, int, Callable::CallError&) (/home/leonard/Git/godot/./core/variant/binder_common.h:419)
[29] CallableCustomMethodPointer<Window, Ref<InputEvent> const&>::call(Variant const**, int, Variant&, Callable::CallError&) const (/home/leonard/Git/godot/./core/object/callable_method_pointer.h:104)
[30] Callable::callp(Variant const**, int, Variant&, Callable::CallError&) const (/home/leonard/Git/godot/core/variant/callable.cpp:57)
[31] Variant Callable::call<Ref<InputEvent> >(Ref<InputEvent>) const (/home/leonard/Git/godot/./core/variant/variant.h:876)
[32] DisplayServerX11::_dispatch_input_event(Ref<InputEvent> const&) (/home/leonard/Git/godot/platform/linuxbsd/x11/display_server_x11.cpp:4063)
[33] DisplayServerX11::_dispatch_input_events(Ref<InputEvent> const&) (/home/leonard/Git/godot/platform/linuxbsd/x11/display_server_x11.cpp:4040)
[34] Input::_parse_input_event_impl(Ref<InputEvent> const&, bool) (/home/leonard/Git/godot/core/input/input.cpp:775)
[35] Input::flush_buffered_events() (/home/leonard/Git/godot/core/input/input.cpp:1056)
[36] DisplayServerX11::process_events() (/home/leonard/Git/godot/platform/linuxbsd/x11/display_server_x11.cpp:5200)
[37] OS_LinuxBSD::run() (/home/leonard/Git/godot/platform/linuxbsd/os_linuxbsd.cpp:960)
[38] /home/leonard/Git/godot/bin/godot.linuxbsd.editor.dev.x86_64(main+0x190) [0x5acb59cb3539] (/home/leonard/Git/godot/platform/linuxbsd/godot_linuxbsd.cpp:85)
[39] /lib/x86_64-linux-gnu/libc.so.6(+0x29d90) [0x799864029d90] (??:0)
[40] /lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0x80) [0x799864029e40] (??:0)
[41] /home/leonard/Git/godot/bin/godot.linuxbsd.editor.dev.x86_64(_start+0x25) [0x5acb59cb32e5] (??:?)
-- END OF BACKTRACE --

Steps to reproduce

Create a node. Connect one of its signals to the scene.

Create a second scene.

Copy the node to the second scene. Error should start appearing.

Try to edit the signal connection on the copied node. This should segfault.

Minimal reproduction project (MRP)

N/A

CreatedBySeb commented 2 weeks ago

What would the expected behaviour here be? It seems like the signal is copied with an empty path which is invalid. The signal is then not persisted to the .tscn file when the scene is saved. Since the signal is not persisted, closing and re-opening the scene clears the invalid signal from the Editor UI, so the issue disappears. Based on the fact the signal is not persisted, it seems like the correct behaviour would potentially be to not copy this signal over at all, since it isn't going to be persisted anyway.

However, an alternative expected behaviour I could envision is that the signal is copied preserving the relative path (i.e. to its immediate parent), but it is unclear how this would work in more complex cases. Given that it seems harder to reason about, going with the more obvious fix of just not copying this signal seems like it may be the better option.

The actual segfault however seems to be caused by the logic displaying the warning labels in the connect dialog (ConnectDialog::_update_warning_label) which does not account for the fact that the destination path could be invalid (attempts to fetch the script for the target node without checking if the node is a nullptr). Since this method already has checks to continue gracefully if the script is not present, it may be beneficial to also check if the destination path is invalid and allow the dialog to continue gracefully in case it doesn't exist for some other reason.