godotengine
Add drag and drop export variables
Duplicate of https://github.com/godotengine/godot/pull/106186, had some git trouble and couldn't figure how to revert 😅 Closes #12394
Think this is pretty much ready for merge on my end. Was reviewed already a bit in the older pr.
@MarianoGnu
All of the GDScript specific code will need to be abstracted out in the future, but it matches the current behavior so its fine for now. I don't like how this yet adds another mode where there is no indication that it is possible until after it is dropped. But that existed before, so it can be fixed separately.
Yup, that's what I was afraid of, doing too much things that aren't related to the general script text editor, but rather to the GDScript text editor specifically. That's why I attempted to add as little as possible to the actual class structure, like the suggested DraggedExport struct and class level variable, and instead trying to keep it scoped inside the drop_data_fw. But if you say this is fine, I will change the implementation. Thanks for the review!
Changed some things to accommodate for @kitbdev suggestions. Some notes:
- I opted to use
mark_scene_as_unsavedinstead of using undo-redo, since there can pretty easily be a situation where the undo does not do anything if the@exportline was deleted/changed from the script. - I wasn't sure where to put all the new class level things in the header file. Let me know if there's a better place to put everything.
- Some crash happened when changing idle parse delay in between dragging in export variables and them showing on the inspector. Couldn't recreate it.
EDIT: realized there's some bug I introduced by doing safety check with checking scene_root->get_script() == script, ignore till fixed
- I opted to use mark_scene_as_unsaved instead of using undo-redo, since there can pretty easily be a situation where the undo does not do anything if the @export line was deleted/changed from the script.
This can easily happen anyways because the undo history of scripts and scenes are separated
- Create an export variable
- Go to the scene and assign it
- Go to the script and delete the line
- Go to scene view (2d or 3d) and undo, output will some something like "Undo: set somevar" this is because the uno simply uses the
set(property, value)method, and the property could simply be ignored
- I opted to use mark_scene_as_unsaved instead of using undo-redo, since there can pretty easily be a situation where the undo does not do anything if the @export line was deleted/changed from the script.
This can easily happen anyways because the undo history of scripts and scenes are separated
* Create an export variable * Go to the scene and assign it * Go to the script and delete the line * Go to scene view (2d or 3d) and undo, output will some something like "Undo: set somevar" this is because the uno simply uses the `set(property, value)` method, and the property could simply be ignored
Yeah, I understand. I just don't see the value in it that much and it also seems a little weird since its only undoing half an action. Not sure about it.
Fixed everything mentioned except undo redo. If it has support from you guys I don't mind implementing it.
Still have a bug because of checking scene_root->get_script() and regarding trying to assign variable when having multiple script instances of the same script in one scene. I have some ideas on how to handle it, working on it.
Yeah, I understand. I just don't see the value in it that much and it also seems a little weird since its only undoing half an action. Not sure about it.
I have the same mixed feelings about adding that to the undo/redo, for one side is an action the user is doing in a different editor (the text editor) in the other side, you could undo other previous actions done in the scene without unassigning this value in the inspector, leaving you in a different state than the one you really was in the history of the scene
@kitbdev raised an important thing I didn't realize, but fixed now, regarding using Script vs ScriptInstance. When we assign export variables, we want to do so on the ScriptInstance itself. This particularly also matters when you have a scene with multiples of the same script, since previously it would just pick the first node it finds in the hierarchy with a matching script.
Beforehand, we couldn't really know which script instance we want to assign the export variable to. So I added the _get_scripts_for_export_assignment method to decide on which script instance to operate on. I did this using EditorInspector->get_edited_object(). I also added support for drag-n-drop export variables on multiple scripts at a time, if there are multiple nodes with the given script selected in the inspector. If there aren't any selected nodes, I just use the first script of the type that is found in the hierarchy(same behavior as before).
This seems to work well for me, Didn't find any bugs. Only slightly annoying behavior is that you cant assign multiple export node variables to multiple scripts at a time because we don't really have a way to have multiple multi-selects in the same hierarchy, but I can't really do anything about that 😅 But this is a niche case anyways, don't think it matters that much.
If you don't have multiples of the same script in the same scene, this should work the same as before.
Demonstration:
https://github.com/user-attachments/assets/01328d45-4837-4582-ad3a-74a298c1b021
Switched to use ObjectID to keep track of which ScriptInstance *s we need to assign to, instead of just storing raw ScriptInstance *. Also fixed everything else, plus added warnings in case the assignment didn't go through in case of scene closed/object deletion/script detachment before script was updated in idle parse.
I really like this feature. I could see myself using this this over onready vars all the time. However, I've noticed an issue where the assignment of the export variable doesn't work when there are errors in the script. For drag and drop of onready vars it doesn't make a difference if there are errors in the script. I don't think this is that big of a deal though.
I think a good addition to this feature would be an option to create an exported array, say when holding shift + alt. For example, when you have many markers in a scene and drag and drop them into the script using shift + alt, it creates a populated Array[Marker2D]. I don't this this functionality exists for onready vars.
I really like this feature. I could see myself using this this over onready vars all the time. However, I've noticed an issue where the assignment of the export variable doesn't work when there are errors in the script. For drag and drop of onready vars it doesn't make a difference if there are errors in the script. I don't think this is that big of a deal though.
Yes, that also bothers me a bit. However it goes a bit outside the scope of this PR imo. My idea for this fix is to keep track of export variables in general, in order to handle all kinds of problems with them losing references. So for example the problem you mentioned with drag and dropping while the script has an error, but I also noticed this happens when cutting and pasting the line back in somewhere else in the script, or when renaming the variable. These can also happen one after the other, WHILE the script has errors. So while I can fix that one problem for when the script has errors, I don't think that's the root of the problem.
I think a good addition to this feature would be an option to create an exported array, say when holding shift + alt. For example, when you have many markers in a scene and drag and drop them into the script using shift + alt, it creates a populated Array[Marker2D]. I don't this this functionality exists for onready vars.
I also like this, however this also adds functionality for @onready vars as well, so I would think I would rather add this as a separate PR for both @export and @onready. I wouldn't want to add this functionality just to @export in this PR since that would feel a bit weird to have @export have that option and @onready not.
EDIT: The shift key seems to be taken for switching between file dropping by UID and by path. Alternate suggestion for the key will need to be inside the feature proposal.
Two really good points though, I've thought about those too. I would love to make separate PRs for those once this gets merged :) Btw, another small thing is to improve variable name generation in case you already have one with the same name, but I would also say that should be a separate PR(I believe @kitbdev also said that).
However, I've noticed an issue where the assignment of the export variable doesn't work when there are errors in the script.
One way to workaround this could be to not clear pending_dragged_exports if file failed to compile, but this could lead to problems, because that queue is tied to the editor, not to the edited file, so you you change to anotehr file and modify something it would try to assign thoseexports to the wrong object.
Maybe it could work with enough caution (like mapping correctly the uid of the script where the assignment should happen, and search for a function that returns if parsing failed or not)
Alternatively you could just show a toast "Assignment of export variables failed" when a ScriptInstance was found, but script failed to parse, that way at least the user is aware
Btw, another small thing is to improve variable name generation in case you already have one with the same name
This code automatically generates variable names without any convenions, not respecting any ProjectSettings, but in example, first thing i do when adding an @onready variable is adding a "_" prefix, that is something i would like to configure in the ProjectSettings. I personally dont bother, because i use snake_case, but i guess people would like to have camelCase or PascalCase here too
In any case this PR is already good as is. If someone wants to try to improve it that could be a followup PR
Fixed a bug where dropping when multiple nodes are selected does not assign export variable/s. EDIT: Updated code comment
When you undo/redo adding variables, they are not getting assigned again 🤔
We have talked about this here a little, the problem is that the histories are separate - the script text editor history for generating the text line is not the same history as for assigning the variable. How would you suggest to go about this?
Well I'm not sure if there is a reasonable fix. You'd have to keep the drop data info in script editor's history, so it's applied every time you redo the variables. I think it's not worth worrying about for now.
Looks fine overall.
Though it's another hidden modifier, it would be nice if it was displayed somewhere. Maybe we could even have a preview of dropped line? idk (that's something for later)
Yeah, that could be cool 🙂
Also, fixed all StringName() comparisions in drop_data_fw() with !is_empty().
looks like CICD wants you to sort your includes alphaberically https://github.com/godotengine/godot/actions/runs/16449155799/job/46489364925?pr=106341
looks like CICD wants you to sort your includes alphaberically https://github.com/godotengine/godot/actions/runs/16449155799/job/46489364925?pr=106341
Oh woops, didn't notice my clang format died for some reason. Fixing
What do you guys think about updating the selection of the inspector to be the node with the script that the @export variable was assigned to after dropping the line/s? Might be more intuitive to show the user what happened.
i dislike the idea of performing actions that wasn't done by the user. As an user i appreciate software to do what i tell it to do, and not take their own desitions.
