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

Make EditorInterface.get_editor_scale() static #66109

Closed
wants to merge 1 commit into from

Conversation

Zylann
Copy link
Contributor

@Zylann Zylann commented Sep 19, 2022

To avoid having to do workarounds like this https://github.com/Zylann/godot_voxel/blob/41e364624fd5b038ad296eefeb7bfddebea9c697/util/godot/editor_scale.cpp#L10

EditorInterface is internally a singleton though, so if it is desired to expose it entirely as a singleton, it could be changed.

@RedMser
Copy link
Contributor

RedMser commented Sep 19, 2022

if it is desired to expose it entirely as a singleton, it could be changed.

I had to use a similar workaround to pass the EditorInterface.get_base_control() between scripts, to access theming properties. I feel that turning the EditorInterface into a singleton would simplify access a lot.

@YuriSizov
Copy link
Contributor

YuriSizov commented Sep 19, 2022

EditorInterface is internally a singleton though, so if it is desired to expose it entirely as a singleton, it could be changed.

Yes, I think this is the way to go.

Ideally, we'd expose some kind of Editor singleton (probably pointing to EditorNode) and pass everything through it. But for now we can just use EditorPlugin and make the get_editor_interface() method static. This will affect user code, and it seems that calling static methods on instances is not currently handled well in GDScript, but that's the case with the current state of the PR as well.

@YuriSizov
Copy link
Contributor

I had to use a similar workaround to pass the EditorInterface.get_base_control() between scripts, to access theming properties. I feel that turning the EditorInterface into a singleton would simplify access a lot.

You don't need to do that, your control already has access to the editor theme as long as it's inside of the tree (and if it's not, then theming doesn't really matter).

@akien-mga
Copy link
Member

This will affect user code, and it seems that calling static methods on instances is not currently handled well in GDScript, but that's the case with the current state of the PR as well.

Calling static methods on instances should work fine. It's the fact that it works fine which can be problematic for some static methods which we don't want to allow calling on instances (typically static methods that return an instance).

@YuriSizov
Copy link
Contributor

Ah, okay then. Even more reason to do it properly and not just for one use case.

@Zylann
Copy link
Contributor Author

Zylann commented Sep 21, 2022

Is it really possible to make a node also an API singleton? It's in the editor's scene tree, that also means there is a particular place it should be registered I guess, as the lifetime of the object isn't bound by register_types and the like. It has no member variables though, so... it could be changed to an Object maybe like other singletons, but then its functions might need to check for existence of EditorNode to be sure no mistake happens (unlike EDSCALE which is just a global float there was no risk)

@akien-mga
Copy link
Member

Superseded by #75694.

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.

5 participants