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

Revert "Let user know about dead instances in deferred calls" #80081

Merged
merged 1 commit into from
Jul 31, 2023

Conversation

clayjohn
Copy link
Member

@clayjohn clayjohn commented Jul 31, 2023

This reverts commit 3a6527d. (PR:#78987)

reduz suggested reverting this in #78987 (comment). As it is currently causing many unexpected problems.

We still need to fix the underlying issues in another way though, although how to do so will require some discussion

CC @reduz @RandomShaper

Production edit: Closes #80002

@clayjohn clayjohn added this to the 4.2 milestone Jul 31, 2023
@clayjohn clayjohn requested a review from a team as a code owner July 31, 2023 08:48
@reduz
Copy link
Member

reduz commented Jul 31, 2023

After thinking about it for a while, to me right solution to this problem is as follows:

  • Document in Object::call_deferred, and Callable::call_deferred that this function will not print an error if the receiver object has been removed because, in many cases, this is entirely intended. This avoids us breaking compatibility, having to change the whole Godot codebase, and running into cases where users intend this behavior, even if they are not thinking about it, then ending up with an error they don't know how to fix.
  • Add to that documentation that, if intending to see an error when the receiver object was removed, an alternate form of founction should be used, such as Object::call_deferred_valid_instance and Callable::call_deferred_valid_instance (or find a new name).

@RandomShaper
Copy link
Member

Go ahead. To be honest, I can't understand now which kind of issue I found that way alleviated by that PR. If the object is gone, there would be no crashes anyway.

@YuriSizov
Copy link
Contributor

The issue that we were trying to solve with the original PR is crashes in exported projects which cannot be caught in editor/debug builds. I don't think changes to the documentation, however useful in principle, are sufficient to address that.

Ideally, under no circumstances should the engine crash in production but be completely fine in dev mode. If there are performance implications for extra checks, that's fine. We have dev asserts that can run only in debug, and be compiled out for export templates.

@YuriSizov YuriSizov merged commit 3fa8fad into godotengine:master Jul 31, 2023
@YuriSizov
Copy link
Contributor

Thanks!

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.

set_deferred method is invalid
4 participants