Exported script properties and sub-resource scripts will now be properly treated as Scripts #30738
Conversation
…nd will include the property name in the path if appropriate)
| } else { | ||
|
|
||
| // Based upon editor/scene_tree_dock.cpp#_tool_selected (case TOOL_ATTACH_SCRIPT); edited | ||
| // to account for exported script properties, resources, and sub-resources |
There was a problem hiding this comment.
I would want to try to merge this again, but instead of merging it back into the mentioned function (as I tried with my first attempt), I would prefer to merge this and the above-mentioned function into editor/editor_node.cpp (or another editor class that's a bit more "global", if you get what I mean) in some manner.
That said, the reason I have not done so already is that I don't think that's within the scope of this PR -- but if it can be considered within the scope, I might try my hand at it.
| // 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.
It would be nice if it could get a NodePath to the current property. For example, if the path to the property is MyCSGMesh:material:albedo_texture:script, the path could generated as "res://scenes/MyCSGMesh.material.albedo_texture.gd". Preliminary testing seems to show that this would be a bit harder than I initially thought, however, and might be better served as its own PR anyways (since from what I can tell, I'd need to implement additional functionality into the Inspector or something).
| path = String("res://").plus_file(edited_res->get_name()); | ||
| } else { | ||
| path = root_path.get_base_dir().plus_file(edited_res->get_name()); | ||
| } |
There was a problem hiding this comment.
Would it be more appropriate to put this section of code (as well as the copies for Node and the else statement) in a macro, for re-use in all three cases? I would normally just go ahead and do so, but I'm not sure if such a move would be appreciated (considering the only macros I ever see in the engine are the error macros), or if there is perhaps a better way of doing that.
LikeLakers2
changed the title
|
By the way, I should mention that even with this change, you still have to instantiate the scripts using I don't quite know how I would deal with this, but I don't know if I need to either. |
|
Whenever someone gets to this PR, please let me know if I should make a proposal over at the godot-proposals repo. I've asked about what to do with PRs like this one over at godotengine/godot-proposals#10 (comment), seeing as how it could be considered both a bugfix and an enhancement, each of which entails different prerequisites, but received no response. (if you want an explanation as to how it could be considered both, see the linked comment) |
|
@LikeLakers2 Is this still desired? If so, it needs to be rebased on the latest master branch. |
|
@aaronfranke I have no desire for this PR, personally. So I won't be updating this PR. Closing. |
|
Also: If someone else wants to implement this PR, they are free to base it off of this PR -- though they should check out the PR comments above to see what I was stuck on. |
…and will include the property name in the path if appropriate
This is my second attempt at solving this, and what I feel is a much better attempt than #30272 was.
This could still be a bit better (see my review comments below; plus the code could maybe be a tiny bit tidier, but I'm not sure how I'd go about it), but it is in a state that I'm happy with, and in a state that solves the issue at hand.