Exported Script-type resources will now be treated properly as Scripts in EditorPropertyResource #30272
Conversation
…ot just when it's the `script` property
…r any exported Script-type property besides the `script` property
| EditorNode::get_singleton()->get_scene_tree_dock()->open_script_dialog(Object::cast_to<Node>(get_edited_object())); | ||
| } else { | ||
| // Copied from editor/scene_tree_dock.cpp#L380 and edited for our purposes | ||
| // There's a good chance we could merge this back into that code with some modifications |
There was a problem hiding this comment.
As mentioned here, there's a good chance we might be able to merge this back into SceneTreeDock's _script_created code, though it'll require some modifications that I'm not sure would really be wanted (I'd be happy to explain if need be). Should I try to merge it?
There was a problem hiding this comment.
It depends on how much modification is needed. If the final result is less or ~same amount of code, that's fine, but if it implies adding a lot of abstraction just to reduce 15 lines of code duplication, it's likely not worth it.
What are your ideas to merge this?
There was a problem hiding this comment.
My idea to merge this is to edit _script_created and _tool_selected (both from SceneTreeDock) to take a "property" parameter, which will be provided as a signal bind, and will be the property that we'll be attaching the script to. When it is set, the script will be attached to that property after creation (for _script_created) or used to generate the "inherits" and "path" bits (for _tool_selected).
Ideally, this method will be used whether we're attaching the script to the script property, or to another Script-type property. (taking a look at object.cpp, it seems it updates the script instance when the script property is updated via set(), but I will test that).
That said, the reason I didn't do it in the first place is because it felt a little weird using properties in the SceneTreeDock, where they wouldn't be relevant.
editor/editor_properties.cpp
Outdated
|
|
||
| ScriptCreateDialog *script_create_dialog = EditorNode::get_singleton()->get_scene_tree_dock()->get_script_create_dialog(); | ||
| script_create_dialog->connect("script_created", this, "_script_created"); | ||
| script_create_dialog->connect("popup_hide", this, "_script_creation_closed"); |
There was a problem hiding this comment.
I realized a couple things shortly after making my other PR that changed the SceneTreeDock to only listen for script creation when it's the one that opened the dialog.
First off, I probably should have marked the popup_hide signal connection as CONNECT_ONESHOT. This would of saved a single line of code at most, though. Should I do that here (and maybe change it to CONNECT_ONESHOT in SceneTreeDock's code too)?
Second, and this is related to the review comment I made about merging this into scene_tree_dock.cpp, I initially brushed off the idea of adding the property name that the script should be attached to, as a signal bind, but now I'm wondering if maybe I should have done that to try to prevent this extra code from needing to be used.
Thoughts?
There was a problem hiding this comment.
First off, I probably should have marked the popup_hide signal connection as CONNECT_ONESHOT. This would of saved a single line of code at most, though. Should I do that here (and maybe change it to CONNECT_ONESHOT in SceneTreeDock's code too)?
Sounds good.
Second, and this is related to the review comment I made about merging this into scene_tree_dock.cpp, I initially brushed off the idea of adding the property name that the script should be attached to, as a signal bind, but now I'm wondering if maybe I should have done that to try to prevent this extra code from needing to be used.
I'm not familiar enough with this code to say, but if it makes the code simpler/cleaner, that would be great.
There was a problem hiding this comment.
I'm not sure how clean I'll be able to make it, so... I will merge this code into scene_tree_dock.cpp in its own commit, and then we can decide if the way it's been handled is better or not.
| // The extra period at the end is to prevent the property name from being seen as the type by ScriptCreateDialog | ||
| String path_property_name = get_edited_property(); | ||
| path_property_name = path_property_name.replace(":", ".").replace("/", ".").replace("\\", "."); | ||
| path = path + "." + path_property_name + "."; |
There was a problem hiding this comment.
As a bit of justification for why I did it this way: I can't just add the .replaces to the end of get_edited_property(), otherwise the compiler complains that I'm trying to use .replace on a StringName, which doesn't have a .replace method despite being fully capable of being cast to a String. If there's a better way to do this, please let me know.
That said, depending on how this gets merged into scene_tree_dock.cpp later, I may not need this weird set of three lines anyways. Or at least, not all of them.
|
Note for myself: I'll need to set the inheritence base type to Object when the ScriptCreateDialog is being opened from the inspector/EditorPropertyResource. |
|
@akien-mga I'm having a very weird issue while trying to merge the code back into SceneTreeDock, one that's resulting in a crash, and I'm not sure how to continue. What's happening is that, because of how I was attempting to get the property, the editor would crash when attempting to add a script after clearing it. The Inspector wouldn't have an object selected after clearing the script, and because I'm querying it for its current object but it doesn't have one, it crashes upon trying to get the property from it. I tried seeing if I could fix the issue by having the node get de-selected when the inspector goes to another resource (so that it would be re-selectable when right-clicking), but apparently that seems to cause subresources to not work properly. Additionally, right-clicking on the node when it's already selected, even on a subresource, doesn't seem to reselect it, and I'm not sure how to fix that. The easy fix while still merging it into SceneTreeDock is to add another parameter for the object we're attaching to (alongside the parameter I already added, which tells it what property we're attaching a script to), but then that feels a little hacky, and at that point I might just want to give the inspector its own ScriptCreateDialog for properties other than Whenever you get a chance, please let me know what I should do. I'm not sure where to go from here. (P.S. testing branch with the commits where i attempted to merge is here: https://github.com/LikeLakers2/godot/tree/exported-script-resources-testing) |
|
Closing, as I'm working on a new implementation of this from scratch, and will open a new PR for that once I'm finished. |
This makes the EditorPropertyResource work properly with exported Script-type resources (that is, properties marked with
export(Script)).By default, the ScriptCreateDialog will default "Inherits" to
Resource. Except, of course, when the property already has a script, where it will default "Inherits" to that script.By default, the ScriptCreateDialog will default "Path" to
<node_or_scene_name>.<property_name>.(with a trailing period to prevent the property name from being seen as an extension), with the proper extension being added onto the end by the ScriptCreateDialog (as it already does).This is a draft because I'd like to make sure everything is in order before it's merged. Please read the review comments below and give me any responses to them that you might have! When the PR is ready for review, I'll merge all the commits and remove the comments I've left behind.