-
-
Notifications
You must be signed in to change notification settings - Fork 21.6k
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
Fix add_root_node()
being no-op
#90136
Conversation
I'm not sure it's a great idea to start revitalizing the EditorScript class since it's borderline entirely deprecated by other classes. This functionality seems like it would fit better as part of EditorInterface. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After adding the root node, the tab for the scene will still show as [empty].
Fixed.
The base functionality of this class is running utility scripts in the editor, which is still relevant. Methods other than |
Would this method still be under the purview of compatibility after not having worked for nearly a decade haha. It has been marked unimplemented since 3.0. I can see a rational for the answer being yes, so I won't press the matter. I still think it makes sense at least for a similar function to be implemented in EditorInterface, and this one to be marked as deprecated (along with |
When using this method with scenes, would it make sense for this method to add an inherited instance of a scene instead of just opening a scene? IMO, having it open the scene, seems wrong and can lead to confusion, and accidentally modifying the original scene, especially when you have other options like vs I could see this as being supplementary to: #90057, but instead of simultaneously opening a new tab and creating a new inherited scene, it allows you to build plugins to do stuff like this after pressing 'Add new scene' |
This PR is supposed to make the method functional again the way it was designed, any additional changes can be done later.
It does not open a scene, you need to be already in empty scene to use it. |
Functional to what standard? The way the engine is used is pretty different from version 1.1 (the actual last time this method worked) to version 4. We'd have to understand first why the method was commented out in the first place to have full understanding of its intent. I would argue at this point you might as well treat it as if you are adding a new function, and its use-cases should be examined as such.
The empty scene gets replaced with the loaded scene, as if you opened it. If you begin to make modifications, you are now modifying the original scene. This is typically not what people want when adding scenes as nodes into the tree and could lead to issues. |
I made it create inherited scene when adding an instance. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tab should probably be marked as unsaved similar to as if you were creating a new scene through the GUI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works great now.
Along with #90057 this will fully implement:
godotengine/godot-proposals#3907, giving users the option to create a new inherited scene either over an empty scene or straight into a new tab.
Thanks! |
The method was non-functional since e9bbb97, for no reason.