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

Signals trigger in-editor, leads to confusing behavior, crashes #82704

Closed
vpellen opened this issue Oct 3, 2023 · 3 comments
Closed

Signals trigger in-editor, leads to confusing behavior, crashes #82704

vpellen opened this issue Oct 3, 2023 · 3 comments
Labels

Comments

@vpellen
Copy link

vpellen commented Oct 3, 2023

Godot version

v4.1.1.stable.official [bd6af8e]

System information

Windows 11

Issue description

So this is a general permutation of several existing bug reports which point to a larger underlying issue. I'm on the fence regarding if this is an issue or a proposal, but given that it's causing editor crashes and multiple bug reports going back several years, I'm going to go out on a limb and suggest that this is undesired behavior that should be treated as its own specific issue, even if its fix may require some manner of executive decision on how to resolve it.

Here's a summation of the issue: In the editor, you can connect signals to methods. If the method is a part of a script, the method will be called iff the script is a tool script. If, however, the method is innate to the node, the method will be called even if there's no script attached at all. This leads to incredibly easy crashes in response to fairly innocuous behavior, such as hooking up queue_free to an animation_finished signal and then trying to play a non-looping animation in the editor.

There are multiple ways to fix this. If I had to suggest one off the top of my head, it'd be to disable firing node signals in the editor entirely unless there's a tool script attached. Alternatively, editor signal connections could be given a toggle to disable or enable triggering within the editor on an individual basis. The specific crashes caused by queue_free in the editor is a wider issue that could be addressed, but given that there are actual uses for calling queue_free within the editor, the actual logic for screening such things could be more trouble than it's worth.

Related bug reports (that I could find - there may be others):
#73771 (comment)
#56230 (comment)
#27125 (comment)

Steps to reproduce

There are multiple ways to go about this.
The most trivial is to create a Node2D and tie a method to its visibility_changed or hidden signals. If you're looking for a benign example, print_tree dumps a period in the output. If you're looking for a less benign example, try queue_free.

Minimal reproduction project

signal_crash.zip
I hesitated to even bother creating an MVP for this given that it skirts the fringes between bug and undesired behavior, but I think there's value in demonstrating just how utterly trivial it is to create something which crashes the editor. Although the example is slightly contrived, even a more realistic example (e.g. queue_free on animation_finished) is simple enough that the average user could run into it without knowing why.

@Jordyfel
Copy link
Contributor

Jordyfel commented Oct 3, 2023

Seems to me like it's reasonable to only allow editor connections to script functions.

@vpellen
Copy link
Author

vpellen commented Oct 3, 2023

Seems to me like it's reasonable to only allow editor connections to script functions.

The thing is, there are still very practical cases where you'd want to connect signals to built-in methods. The animation_finished callback isn't a contrived example - it's how I actually ran into this issue in the first place. It's just that signals probably shouldn't be triggered in the editor unless you're explicitly running a tool script.

@KoBeWi
Copy link
Member

KoBeWi commented Nov 1, 2023

Duplicate of #67864
#74796 will fix this.

@KoBeWi KoBeWi closed this as not planned Won't fix, can't repro, duplicate, stale Nov 1, 2023
@KoBeWi KoBeWi added the archived label Nov 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants