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

Differentiate between core and editor-only singletons #80962

Merged
merged 1 commit into from
Sep 25, 2023

Conversation

YuriSizov
Copy link
Contributor

@YuriSizov YuriSizov commented Aug 24, 2023

Fixes #80704. Supersedes #80722.

The issues comes from the fact that during debug runs the engine technically has all editor-only classes registered, because we are using the "tools" (editor) version of the executable. In exported games this is not the case, so the reported crash doesn't affect templates. However, this does mean that code that is not guarded with is_editor_hint will cause issues when running scenes from the editor, including crashes.

On the one hand, this should've been an issue before, since nothing stops you from writing this in a stable version of Godot:

EditorPlugin.new().get_editor_interface().restart_editor()

But it doesn't happen, because ClassDB has guards against these situations. GDScript is not very helpful with its error messages (see #80921), but at least it doesn't crash:

godot windows editor dev x86_64_2023-08-24_16-44-09

Unfortunately, ClassDB has nothing to do with the engine singletons. So I had to make changes to the Engine class. The Singleton struct already tracks user-created singletons, so it makes sense to add yet another flag for editor-only singletons, to differentiate between them and normal, always accessible core singletons.

So this no longer causes a crash:

EditorInterface.restart_editor()

The error message for GDScript still leaves a lot to be desired, but, once again, it doesn't crash at least.

godot windows editor dev x86_64_2023-08-24_16-43-51

This is a core change, but I think that's something that is justified. EditorInterface is the first editor-only singleton that we have, but it may not be the last. We didn't need these checks before, but we do now.

This makes sure that running scenes in debug mode
(from the editor) does not crash Godot.
In export mode it should already work correctly, because
editor-only singletons are never registered in the first place.
Copy link
Member

@adamscott adamscott left a comment

Choose a reason for hiding this comment

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

I'm not from the core team, but the PR seems OK!

@Mickeon
Copy link
Contributor

Mickeon commented Aug 25, 2023

The error absolutely needs to be adjusted later down the line. Something along the lines "Invalid call. 'EditorInterface' singleton is only available in the editor."

@akien-mga
Copy link
Member

Looks good to me overall.

I haven't looked into details, but isn't it possible to simply not register the singleton when not is_editor_hint()? Or related, why does register_editor_classes run when not using the editor itself?

@YuriSizov
Copy link
Contributor Author

Or related, why does register_editor_classes run when not using the editor itself?

That's a good question. It seems possible to change main's setup2() to check for the editor hint (we should set it before this step AFAIU), but I have no idea what the repercussions of this might be.

isn't it possible to simply not register the singleton when not is_editor_hint()?

I think if we call register_editor_types() at all, it should do everything it's supposed to do, and the responsibility to check for conditions should be on the caller. So I'd prefer your second option, if we go this route.

@YuriSizov
Copy link
Contributor Author

YuriSizov commented Sep 11, 2023

I haven't looked into details, but isn't it possible to simply not register the singleton when not is_editor_hint()? Or related, why does register_editor_classes run when not using the editor itself?

I just had an idea why this may be useful. This way we should be able to show more useful error messages to the user if they try to access editor-only classes from project code. Granted, we don't do that particularly well for now, but if we don't register those classes at all, we won't be able to differentiate them from classes that just don't exist. So at least that's a possibility that we can explore.

Edit: @reduz said that it was probably done for C# API hashes to work correctly. Which is no longer a thing in Godot 4, so maybe that's not needed to happen anymore. I do think though that extra error hinting is a worthy goal.

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Yolo.

@akien-mga akien-mga merged commit 9750876 into godotengine:master Sep 25, 2023
16 checks passed
@akien-mga
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Executing EditorInterface.restart_editor function crashes Godot
4 participants