Hot Reload broken in Godot 4.3 beta 1

[m4gr3d]

Tested versions

• Reproducible in Godot 4.3 beta 1
• Not reproducible in Godot 4.3 dev 6

System information

Tested and reproduced on Windows 11 and Android 14

Issue description

Hot reload no longer works when running a project with Synchronize scene changes and Synchronize script changes enabled.

Steps to reproduce

• Create a project
• Enable Synchronize scene changes and Synchronize script changes
• Run the project and in the editor window toggle the visibility of node elements
• Notice that in Godot 4.3 beta 1, the running project is not updated, while it's in Godot 4.3 dev 6

Minimal reproduction project (MRP)

N/A: affects all projects

[matheusmdx]

Bisecting points to #92350 as the culprit:

[akien-mga]

CC @4d49 @KoBeWi

[matheusmdx]

Ok, i did some experiments here and what is happening:

First, hot reloading doesn't entire fails to work, if you change the properties in the inspector they will apply in game as expected. but if you drag and drop, rotate, change visibility, etc, outside the inspector, the hot reload fails

unknown_2024.06.02-20.14.mp4

Before #92350 UndoRedo used the Callable.get_method() as the do/undo_op.name and static_cast only as fallback for custom callables:

godot/core/object/undo_redo.cpp

Lines 162 to 166 in 83b916b

	do_op.name = p_callable.get_method();
	if (do_op.name == StringName()) {
	// There's no `get_method()` for custom callables, so use `operator String()` instead.
	do_op.name = static_cast<String>(p_callable);
	}

But thats made #90430 happens because lambdas raise a error if Callable.get_method() is used, so #92350 now checks for Callable.is_custom() and if is true, do a static cast to string of the callable as the do/undo_op.name instead the Callable.get_method() result

godot/core/object/undo_redo.cpp

Lines 162 to 167 in 705b7a0

	// There's no `get_method()` for custom callables, so use `operator String()` instead.
	if (p_callable.is_custom()) {
	do_op.name = static_cast<String>(p_callable);
	} else {
	do_op.name = p_callable.get_method();
	}

And as you can see in the video above, all the callables return true for is_custom() (the lines that start with "4") and that fails to work with hot reloading when the callable is from a Node (why i don't know)

That callables hot reload using the static cast result (the callables from editing properties from inspector):

Callable: EditorInspector::emit_signal
Get method result: emit_signal
Static cast result: EditorInspector::emit_signal

Callable: EditorInspector::_edit_request_change
Get method result: _edit_request_change
Static cast result: EditorInspector::_edit_request_change

That callables fail to hot reload using the static cast result (the callables from drag and drop, click the eye buton in scene tree, etc)

Callable: Node2D::set_visible
Get method result: set_visible
Static cast result: Node2D::set_visible

Callable: ColorRect::_edit_set_state
Get method result: _edit_set_state
Static cast result: ColorRect::_edit_set_state

So the options to solve this problem is:

1: Downgrade the error emited by Callable.get_method() when is called from a lambda to a warning and return StringName(), so we can revert #92350
2: Fix the problem that make the node callables fail, after all the inspector ones work as expected
3: If Callable.is_custom() is wrongly returning true when wasn't to be, fix that

If none of the above can be done, i have a totally not workaround solution:

	String callable_string = static_cast<String>(p_callable);
	// The lambda static cast result is lambda_name(lambda) or 
	// <anonymous lambda>(lambda) so...
	if (p_callable.is_custom() && callable_string.count("(lambda)") != 0) {
		do_op.name = callable_string;
	} else {
		do_op.name = p_callable.get_method();
	}
	// You can question my methods, but can't question my results, that works after all!

Probably this has another (better) solutions but that is what my limited knowledge allowed me to get, at least to give some direction of what is happening

[4d49]

Hello. First of all, I would like to apologize that my fix led to such a serious problem 😥

I've been researching the problem for a while and have come to some conclusions. The problem really is my fix. Debugger uses the method names from UndoRedo to call methods in the application instance. And my fix broke that... Sorry.

If we call bind on a Callable object, it starts to be considered as custom. So as a possible solution we can add a check if Callable is binded.

if (p_callable.is_custom()) {
	const CallableCustomBind *bind = dynamic_cast<CallableCustomBind *>(p_callable.get_custom());
	// Check if CallableCustom is bind.
	if (bind != nullptr) {
		do_op.name = bind->get_method();
	} else {
		do_op.name = static_cast<String>(p_callable);
	}
} else {
	do_op.name = p_callable.get_method();
}

[KoBeWi]

What if we add something like is_named() or has_name() to Callable? 🤔

[4d49]

Also I think the get_method function should be pure virtual functions like the other functions in the CallableCustom class.

godot/core/variant/callable.cpp

Lines 440 to 442 in 83b916b

	StringName CallableCustom::get_method() const {
	ERR_FAIL_V_MSG(StringName(), vformat("Can't get method on CallableCustom \"%s\".", get_as_text()));
	}

[4d49]

What if we add something like is_named() or has_name() to Callable? 🤔

I also think it would be nice to add an is_binded method to Callable.

[akien-mga]

Thanks for the investigation!

For now I'll revert #92350 which seems to be the safer approach for 4.3.

We can then explore the other approach to keep the improvement from #92350 while addressing the regression in a new PR, for 4.4.