Skip to content

Conversation

@kleonc
Copy link
Member

@kleonc kleonc commented Dec 25, 2025

Fixes #114326.

Changes CONNECT_APPEND_SOURCE_OBJECT to bind the source object on signal emission. Previously it was done only on PackedScene instantiation, which led to bugs/inconsistencies.

  • CONNECT_APPEND_SOURCE_OBJECT did not work on runtime connections.
  • On runtime, duplicating a Node with the source object/Node binded during the scene instantiation would not replace the binded source object with the newly duplicated one.
  • The source object/Node binded during the scene instantiation might be tried to be saved in the scene file.
  • etc.

Modified MRP from #114326 with the examples below: duplicate-signal-extra-arg_v2.zip

Before (v4.6.beta2.official [551ce8d]) After (this PR)
Godot_v4 6-beta2_win64_wQKInKJUeV Sw5Z8VevPJ
Godot_v4 6-beta2_win64_DhLwm8jbzi 2cM7n4ITyY
Godot_v4 6-beta2_win64_XJHynYXQjd
gB2eqbazlf
MeJntvXswE
NczvKU27Bc

Relevant part of control.tscn from #114326 MRP:

  • Before (v4.6.beta2.official [551ce8d]):
[connection signal="resized" from="Parent/Child" to="Parent" method="_on_child_resized" flags=18]
[connection signal="resized" from="Parent2/Child" to="Parent2" method="_on_child_resized" flags=18 binds= [Object(Control,"unique_name_in_owner":false,"process_mode":0,"process_priority":0,"process_physics_priority":0,"process_thread_group":0,"physics_interpolation_mode":2,"auto_translate_mode":0,"editor_description":"","visible":true,"modulate":Color(1, 1, 1, 1),"self_modulate":Color(1, 1, 1, 1),"show_behind_parent":false,"top_level":false,"clip_children":0,"light_mask":1,"visibility_layer":1,"z_index":0,"z_as_relative":true,"y_sort_enabled":false,"texture_filter":0,"texture_repeat":0,"material":null,"use_parent_material":false,"clip_contents":false,"custom_minimum_size":Vector2(0, 0),"layout_direction":0,"layout_mode":0,"anchors_preset":0,"anchor_left":0.0,"anchor_top":0.0,"anchor_right":0.0,"anchor_bottom":0.0,"offset_left":0.0,"offset_top":0.0,"offset_right":40.0,"offset_bottom":40.0,"grow_horizontal":1,"grow_vertical":1,"rotation":0.0,"scale":Vector2(1, 1),"pivot_offset":Vector2(0, 0),"pivot_offset_ratio":Vector2(0, 0),"size_flags_horizontal":1,"size_flags_vertical":1,"size_flags_stretch_ratio":1.0,"localize_numeral_system":true,"tooltip_text":"","tooltip_auto_translate_mode":0,"focus_neighbor_left":NodePath(""),"focus_neighbor_top":NodePath(""),"focus_neighbor_right":NodePath(""),"focus_neighbor_bottom":NodePath(""),"focus_next":NodePath(""),"focus_previous":NodePath(""),"focus_mode":0,"focus_behavior_recursive":0,"mouse_filter":0,"mouse_behavior_recursive":0,"mouse_force_pass_scroll_events":true,"mouse_default_cursor_shape":0,"shortcut_context":null,"accessibility_name":"","accessibility_description":"","accessibility_live":0,"accessibility_controls_nodes":Array[NodePath]([]),"accessibility_described_by_nodes":Array[NodePath]([]),"accessibility_labeled_by_nodes":Array[NodePath]([]),"accessibility_flow_to_nodes":Array[NodePath]([]),"theme":null,"theme_type_variation":&"","script":null)
]]
  • After (this PR):
[connection signal="resized" from="Parent/Child" to="Parent" method="_on_child_resized" flags=18]
[connection signal="resized" from="Parent2/Child" to="Parent2" method="_on_child_resized" flags=18]

@kleonc kleonc added this to the 4.6 milestone Dec 25, 2025
@kleonc kleonc requested a review from a team as a code owner December 25, 2025 02:29
@kleonc
Copy link
Member Author

kleonc commented Dec 25, 2025

cc @Rindbee (#60143)

@kleonc kleonc force-pushed the node_duplicating_signal_source_node_fix branch from 1e219dc to bf302c5 Compare December 25, 2025 02:38
Copy link
Contributor

@Rindbee Rindbee left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@kleonc
Copy link
Member Author

kleonc commented Dec 25, 2025

I've realized this is not a proper fix, this would break for a runtime duplication of a Node with source binded using CONNECT_APPEND_SOURCE_OBJECT (which is broken even before this PR as binds including the source Node would be duplicated but without retargetting the binded source to be relative to the duplicated-new-node if possible).

CONNECT_APPEND_SOURCE_OBJECT seems to be broken / half-baked in general, as e.g. connecting to a signal with CONNECT_APPEND_SOURCE_OBJECT flag does not bind the source on runtime, only when saving to a PackedScene. This leads to bugs/inconsistencies.

@tool
extends EditorScript

class Signaler:
	signal sig

class Receiver:
	func callback(source: Object) -> void:
		print("%s.callback( source=%s )" % [self, source])

func _run() -> void:
	var source := Signaler.new()
	var receiver := Receiver.new()

	source.sig.connect(receiver.callback, CONNECT_APPEND_SOURCE_OBJECT)
	source.sig.emit() # ERROR: core/object/object.cpp:1310 - Error calling from signal 'sig' to callable: 'RefCounted::callback': Method expected 1 argument(s), but called with 0.

@kleonc kleonc marked this pull request as draft December 25, 2025 14:22
@Rindbee
Copy link
Contributor

Rindbee commented Dec 25, 2025

CONNECT_APPEND_SOURCE_OBJECT seems to be broken / half-baked in general, as e.g. connecting to a signal with CONNECT_APPEND_SOURCE_OBJECT flag does not bind the source on runtime, only when saving to a PackedScene.

CONNECT_APPEND_SOURCE_OBJECT is currently a flag primarily used for PackedScene and is somewhat redundant now. It was originally intended to enhance the functionality of the connection dialog, but the ability to add extra parameters of type NodePath (^".") now effectively replaces this functionality.

@kleonc kleonc force-pushed the node_duplicating_signal_source_node_fix branch from bf302c5 to 9f54dce Compare December 26, 2025 11:21
@kleonc kleonc marked this pull request as ready for review December 26, 2025 11:22
@kleonc kleonc requested review from a team as code owners December 26, 2025 11:22
@kleonc kleonc force-pushed the node_duplicating_signal_source_node_fix branch from 9f54dce to 49d9ffd Compare December 26, 2025 11:58
@kleonc
Copy link
Member Author

kleonc commented Dec 26, 2025

CONNECT_APPEND_SOURCE_OBJECT is currently a flag primarily used for PackedScene and is somewhat redundant now. It was originally intended to enhance the functionality of the connection dialog, but the ability to add extra parameters of type NodePath (^".") now effectively replaces this functionality.

I've moved handling CONNECT_APPEND_SOURCE_OBJECT to Object / removed special handling from PackedScene. PR description updated.

@kleonc kleonc changed the title Fix duplicating signal source Node binded with CONNECT_APPEND_SOURCE_OBJECT flag Fix CONNECT_APPEND_SOURCE_OBJECT to bind on signal emission Dec 26, 2025
}

if (flags & CONNECT_APPEND_SOURCE_OBJECT) {
callable_with_source_object_binded = callable_ptr->bind(this);
Copy link
Contributor

@Rindbee Rindbee Dec 27, 2025

Choose a reason for hiding this comment

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

Using bind() again (like a Russian nesting doll) will cancel the source's emission (not the original arguments) if unbinding is enabled (unbind > 0).

1

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.

Duplicating a node adds an extra empty argument to signal.

2 participants