Improve drag and drop into array property editors by ttencate · Pull Request #102534 · godotengine/godot

ttencate · 2025-02-07T14:50:03Z

This makes a couple of things work that you'd expect to work:

Dragging a node now highlights properties accepting that node's type as valid drop targets. The drop was already possible, it just wasn't indicated like it was for other types of properties.
Custom resources can now be dropped onto array property editors that accept that resource.
Nodes with a script can now be dropped onto array property editors that accept that type of script.
Items can be added to an array property by dropping them on the + Add Element button. The button is highlighted to indicate this.

Further details and rationale for implementation can be found in the individual commit messages.

Fixes godotengine/godot-proposals#6862, which currently has 33 upvotes and no objections.

CC @YuriSizov who brought this up on Mastodon.

Demo video:

dragdrop.mp4

Test project:

dragdroptest.zip

KoBeWi · 2025-02-07T23:42:44Z

Custom Resource Array gets highlighted for CustomResource.gd, even though it doesn't (and shouldn't) accept it.

O8sr887BVQ.mp4

ttencate · 2025-02-08T09:28:10Z

@KoBeWi Great find! But that bug is also present in the current master, not caused by this PR 😁 In fact, you can also drag CustomResource.gd onto the custom_resource property, which even accepts it despite being the wrong type. (You can also drag it onto resource and resource_array, but that makes sense because Script inherits from Resource.)

I'll see if I can find out why that happens and contribute a fix~~, but I don't think it needs to block this PR~~.

ttencate · 2025-02-08T10:24:51Z

Correction: it does need some work in this PR because I copied the faulty logic into EditorPropertyArray for the case of dropping a "resource" type (e.g. from another property) onto the array itself.

The cause is this bit here:

godot/editor/editor_resource_picker.cpp

Lines 689 to 692 in 36d90c7

    
           StringName custom_class = EditorNode::get_singleton()->get_object_custom_type_name(res.ptr()); 
        
           if (_is_type_valid(custom_class, allowed_types)) { 
        
           	return true; 
        
           }

I'm not sure what the purpose of that code is. _is_type_valid already checked whether the type inherits from one of the allowed types, both for built-in and custom classes. But I'll assume that it's good for something, and not just remove it.

The bug happens because EditorNode::get_object_custom_type_name() returns the global_class name of the script set on the object, but also if the object is itself a Script, it returns its global_class. No idea why that is useful; it seems like conflating two different things. This method was introduced in 4f72178 for use in a tooltip in SceneTreeEditor, where it's only called for Nodes so the object can never be a Script anyway. Since then, usage has been added in e8f15f7 for the EditorResourcePicker, where it's explicitly only used to get the global_class of Script objects directly. And this was subsequently changed in d32f270, now calling it with the resource itself rather than the resource's script, introducing the bug.

For now, I'll add another commit to this PR that fixes the bug in the least invasive way possible, and also modify the commit where I copied the bug to the EditorPropertyArray. But I think all this logic is too convoluted and too scattered, and should be cleaned up at some point.

ttencate · 2025-02-08T10:42:38Z

Should all be fixed now, please take another look.

FYI, I would rather not squash these commits together, because each of them can stand on its own. If any regressions are introduced, bisecting and focused reverting is easier this way.

This was already done for Resource properties; this just makes things consistent.

See discussion on godotengine#102534. Before this fix, it was possible to drag a file `CustomResource.gd` with `class_name CustomResource` onto a property of type `CustomResource`. Of course, the intention is that only `Resource`s with the `CustomResource` script attached are accepted. (Actually accepting the script, but creating a resource from it automatically, is left as a future improvement.)

This duplicates some of the logic in EditorResourcePicker, but that's unavoidable: EditorResourcePicker works on a single, loaded resource, whereas we'd like this to be efficient and not need to load all (potentially many) dragged resources. Instead, we use the EditorFileSystem cache to get the information we need. Note that EditorResourcePicker also supports some automatic conversions, such as from Shader to ShaderMaterial. This change does not attempt to make that work for arrays as well.

For example, an Array[MyScript] property now accepts any node with the script MyScript on it. This works for Node and NodePath arrays alike, and also with the @export_node_path annotation.

The most common scenario is dragging a file from the filesystem dock, which drags it as type "file", not "resource". This already worked. This change also makes it possible to drag a resource from another property editor.

Instead of having to first click the button, then drag the item into the new slot, this allows just dragging the item onto the button directly. The button is highlighted as a valid drop target to signal this.

ttencate · 2025-03-08T18:35:42Z

Rebased onto master, fixed conflicts with b9a6057. Now that the dust has settled on the 4.4 release, I guess this is ready to merge?

Repiteo · 2025-03-13T13:39:26Z

Needs squash before this can be merged

ttencate · 2025-03-13T13:54:47Z

@Repiteo As mentioned above:

FYI, I would rather not squash these commits together, because each of them can stand on its own. If any regressions are introduced, bisecting and focused reverting is easier this way.

Each of them could even have been a separate PR, but the overhead didn't seem worth it.

Still want me to squash?

Repiteo

On second thought, nah. The squashing isn't a hard-rule, and doesn't need to be enforced when you've done such a good job at making atomic commits. We'll ship it as-is!

Repiteo · 2025-03-14T15:13:18Z

Thanks!

azur-wolve · 2025-03-22T15:00:18Z

adding a red tone on fields that cant accept it would be a plus

ttencate · 2025-03-22T18:16:56Z

adding a red tone on fields that cant accept it would be a plus

I think it's better to draw attention to properties that can be interacted with, but you can always open a proposal!

AThousandShips added enhancement topic:editor usability labels Feb 7, 2025

AThousandShips added this to the 4.x milestone Feb 7, 2025

AThousandShips requested a review from a team February 7, 2025 14:50

ttencate force-pushed the feature/drag_drop_into_array branch from 4e6d4fc to 7e84479 Compare February 8, 2025 10:40

KoBeWi reviewed Feb 9, 2025

View reviewed changes

Comment thread editor/editor_properties_array_dict.cpp Outdated

KoBeWi reviewed Feb 9, 2025

View reviewed changes

Comment thread editor/editor_properties_array_dict.cpp Outdated

KoBeWi approved these changes Feb 9, 2025

View reviewed changes

KoBeWi reviewed Feb 9, 2025

View reviewed changes

Comment thread editor/editor_properties_array_dict.cpp Outdated

ttencate force-pushed the feature/drag_drop_into_array branch 2 times, most recently from 5a09650 to 3ec3d2c Compare February 10, 2025 08:04

ttencate added 6 commits March 8, 2025 19:24

Highlight Node and NodePath properties as valid drop targets

36c1b01

This was already done for Resource properties; this just makes things consistent.

Allow dragging custom node types onto array property editor

11b3900

For example, an Array[MyScript] property now accepts any node with the script MyScript on it. This works for Node and NodePath arrays alike, and also with the @export_node_path annotation.

Allow dragging items onto array property editor Add Element button

93d342b

Instead of having to first click the button, then drag the item into the new slot, this allows just dragging the item onto the button directly. The button is highlighted as a valid drop target to signal this.

ttencate force-pushed the feature/drag_drop_into_array branch from 3ec3d2c to 93d342b Compare March 8, 2025 18:34

KoBeWi modified the milestones: 4.x, 4.5 Mar 8, 2025

Repiteo approved these changes Mar 14, 2025

View reviewed changes

Repiteo merged commit 409fe3c into godotengine:master Mar 14, 2025

Rudolph-B mentioned this pull request Jun 1, 2025

Can't drag and drop assign multiple Custom Resources in the editor the same way built-in resources can #92666

Closed

Uh oh!

Conversation

ttencate commented Feb 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

KoBeWi commented Feb 7, 2025

Uh oh!

ttencate commented Feb 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ttencate commented Feb 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ttencate commented Feb 8, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ttencate commented Mar 8, 2025

Uh oh!

Repiteo commented Mar 13, 2025

Uh oh!

ttencate commented Mar 13, 2025

Uh oh!

Repiteo left a comment

Choose a reason for hiding this comment

Uh oh!

Repiteo commented Mar 14, 2025

Uh oh!

azur-wolve commented Mar 22, 2025

Uh oh!

ttencate commented Mar 22, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ttencate commented Feb 7, 2025 •

edited

Loading

ttencate commented Feb 8, 2025 •

edited

Loading

ttencate commented Feb 8, 2025 •

edited

Loading