Yet another node copy-paste PR #34892

KoBeWi · 2020-01-07T17:09:48Z

Following the failure of #31616, here's another attempt to close #3720 (counting all PRs so far, isn't it 5th one already?). I'll be angry if this one isn't merged eventually :/

In my previous PR, the problem was that copy-paste wasn't working like if you duplicated the node, so now I literally used duplicate(). The PR introduces node_clipboard variable, which is a list of copied nodes. Basically you can select multiple nodes and they will be duplicated into the list. If you copy, but the list already has elements, they are freed.

Paste makes yet another copy of these nodes and puts them under the node you have selected. The fact that I used duplicate (duplicate_from_editor() to be precise) ensures that all references and everything is correctly kept. If you copy a node with resource, the pasted node will still reference the same resource. What's more interesting, you can copy a scene root and paste it into another scene, which will create an instance (xd). Paste has already anti-cyclic-reference safeguards.

The Cut operation might be arguable though. When you cut-paste a file, the original file is moved to the new location. I tried to do that, but I've ran into infinite number of reference-related problems, so my Cut operation works like in text editors. It's the same as Copy, but also deletes the node.

YeldhamDev · 2020-01-07T17:13:43Z

Chin up, @KoBeWi! 5th time is the charm! 😉

KoBeWi · 2020-03-12T22:42:36Z

I rebased the PR, because there were some conflicts. One line was lost as a result of that and I don't know what it was :I

Anyways, I've been testing this for few months and seems to work very well. Copy-pasting on the same scene is like duplicate + move, while between scenes works like "Merge From Scene" (including all it's problems, i.e. unexpected resource sharing). IMO this should be fine to merge already >_>

pouleyKetchoupp

Great work! Thanks for implementing this feature. I've started to use it on my custom branch and it generally works fine. I'll keep sending you feedback as I get to do more tests in different scenarios.

I've left a few comments on the code itself, and here are the two issues I've spotted so far:

When pasting a node with connected signals:
The path to node is set to "." in the pasted node and it shows this error:

ERROR: Condition "!common_parent" is true. Returning: NodePath()
   at: Node::get_path_to (scene\main\node.cpp:1645)
Callstack:
 	Node::get_path_to(const Node * p_node) Line 1645	C++
 	Node::_duplicate_signals(const Node * p_original, Node * p_copy) Line 2264	C++
 	Node::duplicate_from_editor(Map<Node const *,Node *,Comparator<Node const *>,DefaultAllocator> & r_duplimap) Line 2165	C++
 	SceneTreeDock::_tool_selected(int p_tool, bool p_confirm_override) Line 475	C++
 	SceneTreeDock::_unhandled_key_input(Ref<InputEvent> p_event) Line 98	C++

Deleting a node from the tree doesn't work anymore, it triggers this error:

ERROR: Error calling from signal 'confirmed' to callable: SceneTreeDock::_delete_confirm : Method expected 1 arguments, but called with 0..
   at: Object::emit_signal (core\object.cpp:1181)

Can be fixed by setting a default value in the method binding:

ClassDB::bind_method(D_METHOD("_delete_confirm"), &SceneTreeDock::_delete_confirm, DEFVAL(false));

editor/scene_tree_dock.cpp

pouleyKetchoupp · 2020-06-20T13:59:36Z

editor/scene_tree_dock.cpp

+				for (Map<const Node *, Node *>::Element *E2 = duplimap.front(); E2; E2 = E2->next()) {
+					Node *d = E2->value();
+					editor_data->get_undo_redo().add_do_method(d, "set_owner", owner);
+				}


This process is different from what is currently done in the TOOL_DUPLICATE case, where it only changes the owner on nodes that were already owned by the one being copied. I'm not sure what cases are covered this way, but should it be the same here?

In general, could duplicate and paste share most of the code since they are similar processes? (apart from the fact paste also handles separate scenes).

Well, they do share much of the code. Though the most important part is duplicate_from_editor(). I just tried to make a common method that would be called in both cases and well, it didn't work. The method I use in paste is much simplified and adjusted to work in scenarios that duplicate doesn't happen (like, copying scene root or pasting across the scenes).

Is the difference you spotted important? The code never failed me (and I've been using it since it was PRed here), but maybe there's some edge case I didn't predict and it breaks everything (not like it's possible to make the code perfect anyways).

I don't know if it's important, and in the duplicate code it was there from the beginning so it's difficult to confirm the reason why it's done this way. I guess time will tell :)

My main point was that it might be safer and cleaner to use a common method, but if the two cases are too different to do that it's fine.

editor/scene_tree_dock.cpp

KoBeWi · 2020-06-22T15:54:41Z

When pasting a node with connected signals:
The path to node is set to "." in the pasted node and it shows this error:

Not sure how to avoid this error, other than removing the error on non-common parent and just aborting.

Deleting a node from the tree doesn't work anymore, it triggers this error:

It works for me, the binding you quoted no longer exists. Are you testing on master?

pouleyKetchoupp · 2020-06-23T07:02:34Z

When pasting a node with connected signals:
The path to node is set to "." in the pasted node and it shows this error:

Not sure how to avoid this error, other than removing the error on non-common parent and just aborting.

I haven't investigated the details, but since duplicating a node doesn't trigger this error, could there be a way to avoid it without removing it completely?
Anyway, it's a an error that would need fixing eventually but it's not critical since it doesn't seem to break anything (on my 3.1-based custom branch it was causing the path to be reset to .).

Deleting a node from the tree doesn't work anymore, it triggers this error:

It works for me, the binding you quoted no longer exists. Are you testing on master?

I was initially testing on a different branch, but I have the same issue on master. After clicking ok in the delete confirmation popup, nothing happens and I get an error.
On master it's using a callable instead of method binding though, so the fix is a bit different from what I had stated in my comment:

delete_dialog->connect("confirmed", callable_mp(this, &SceneTreeDock::_delete_confirm), varray(false));

KoBeWi · 2020-06-23T10:27:48Z

Ok, fixed the dialog bug. I missed it, because I'm deleting without confirmation 🙃

pouleyKetchoupp · 2020-06-26T19:23:42Z

@akien-mga This PR seems ready to move on :)

Since this feature has high demand, it would be nice to merge to master soon (if needed, after review from code owners) so more contributors can start testing it!

groud

Needs squashed commits, but otherwise seems fine to me.
Using duplicate_from_editor makes sense, as it kind of guaranteed that the difficult part is handled by something already working in the editor.

KoBeWi · 2021-02-09T15:43:35Z

Squashed.

KoBeWi · 2021-02-09T19:54:08Z

So I noticed that there were some problems after recent changes in core, but should be all fine now.

KoBeWi · 2021-02-12T16:43:50Z

I added a second commit that makes sure that built-in resources from nodes pasted to other scenes are unique (i.e. not shared between scenes).

editor/scene_tree_dock.cpp

reduz · 2021-02-12T16:55:00Z

~~There is also probably something else you need to do here to make this as correct as possible.~~
~~1) You need to keep track of everything you remapped, so further copies and pastes keep reusing the correct remaps. For this you will need to keep a map stored in EditorData::EditorScene~~
2) As the above will survive the clipboard and remain for as long as the scene is being edited, you don't really want to keep references to a resource you deleted, because its basically leaking it (and you probably don't want to go around tracking what you deleted either), so I suggest instead storing a Map<ObjectID,ObjectID> in this class. You can use it to look up previous remaps, and if the ObjectID became invalid on either side, then create a new one.

Update: Nevermind, I am wrong. As we discussed in chat, in this case it makes more sense that users will expect the new copy/paste to create new (duplicated) content. If you want the local one you copy from the local scene.

fire · 2021-02-12T19:54:51Z

Some shadowed declarations are breaking Linux.

KoBeWi · 2021-02-12T21:39:14Z

Ok sorted out the shadowing. Turns out I could move one of the loops to the outside scope (line 474 in scene tree dock).

akien-mga · 2021-02-12T22:16:34Z

Thanks!

jimmyjonezz · 2021-02-13T06:13:08Z

Ура. Спасибо, Comrade!

hilfazer · 2021-02-25T10:10:41Z

This, together with removal of Merge From Scene, also resolves #31744.

KoBeWi added feature proposal topic:editor usability labels Jan 7, 2020

KoBeWi added this to the 4.0 milestone Jan 7, 2020

KoBeWi force-pushed the copy-pasta_v7 branch from 4cf4be7 to 48bf62a Compare January 7, 2020 19:15

KoBeWi force-pushed the copy-pasta_v7 branch from 48bf62a to c35a89a Compare January 31, 2020 13:35

KoBeWi force-pushed the copy-pasta_v7 branch from c35a89a to 15395ac Compare March 12, 2020 22:35

pouleyKetchoupp mentioned this pull request May 29, 2020

Make true copy and paste as well as Ctrl+C, Ctrl+X, Ctrl+V throughout the editor a thing godotengine/godot-proposals#955

Open

KoBeWi force-pushed the copy-pasta_v7 branch 2 times, most recently from fe38c66 to b5ab0e1 Compare June 8, 2020 19:32

KoBeWi mentioned this pull request Jun 9, 2020

Allow copying property path from inspector. #39404

Merged

pouleyKetchoupp reviewed Jun 20, 2020

View reviewed changes

KoBeWi mentioned this pull request Sep 1, 2020

Merge from scene confusion (maybe bug) #32039

Closed

KoBeWi requested a review from reduz October 19, 2020 19:07

This was referenced Dec 19, 2020

Add ability to drag nodes between scenes #23447

Closed

Merge From Scene option invokes two file handling dialogs #36474

Closed

KoBeWi added the cherrypick:3.x Considered for cherry-picking into a future 3.x release label Feb 9, 2021

groud requested changes Feb 9, 2021

View reviewed changes

KoBeWi force-pushed the copy-pasta_v7 branch from 0cc1d6e to 892c1d7 Compare February 9, 2021 15:43

groud approved these changes Feb 9, 2021

View reviewed changes

Add node copy-paste

abe548d

KoBeWi force-pushed the copy-pasta_v7 branch from 892c1d7 to abe548d Compare February 9, 2021 19:53

KoBeWi force-pushed the copy-pasta_v7 branch from eda93b2 to f78dd54 Compare February 12, 2021 16:43

reduz requested changes Feb 12, 2021

View reviewed changes

editor/scene_tree_dock.cpp Outdated Show resolved Hide resolved

KoBeWi force-pushed the copy-pasta_v7 branch from f78dd54 to 8d28faa Compare February 12, 2021 19:16

reduz approved these changes Feb 12, 2021

View reviewed changes

Duplicate resources pasted to other scenes

36494e8

KoBeWi force-pushed the copy-pasta_v7 branch from 8d28faa to 36494e8 Compare February 12, 2021 21:38

akien-mga merged commit e7ab3a4 into godotengine:master Feb 12, 2021

KoBeWi deleted the copy-pasta_v7 branch February 12, 2021 22:17

KoBeWi mentioned this pull request Feb 13, 2021

Remove Merge From Scene #45943

Merged

This was referenced Feb 13, 2021

Fix internal resource detecting when pasting nodes #45950

Merged

[3.2] Add node copy-paste #45951

Merged

KoBeWi removed the cherrypick:3.x Considered for cherry-picking into a future 3.x release label Feb 13, 2021

akien-mga mentioned this pull request Feb 25, 2021

Merging instanced node from another scene bug #31744

Closed

akien-mga mentioned this pull request Apr 21, 2021

Copy, Cut, Paste scene nodes #3720

Closed

YuriSizov mentioned this pull request Apr 27, 2021

Copy-pasting nodes between scenes (and projects) godotengine/godot-proposals#2659

Closed

lewiji mentioned this pull request Oct 12, 2021

Add copy/paste keyframe functionality to AnimationPlayer editor godotengine/godot-proposals#3419

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Yet another node copy-paste PR #34892

Yet another node copy-paste PR #34892

KoBeWi commented Jan 7, 2020

YeldhamDev commented Jan 7, 2020

KoBeWi commented Mar 12, 2020

pouleyKetchoupp left a comment

pouleyKetchoupp Jun 20, 2020

KoBeWi Jun 22, 2020

pouleyKetchoupp Jun 22, 2020

KoBeWi commented Jun 22, 2020

pouleyKetchoupp commented Jun 23, 2020

KoBeWi commented Jun 23, 2020

pouleyKetchoupp commented Jun 26, 2020

groud left a comment

KoBeWi commented Feb 9, 2021

KoBeWi commented Feb 9, 2021

KoBeWi commented Feb 12, 2021

reduz commented Feb 12, 2021 •

edited

Loading

fire commented Feb 12, 2021

KoBeWi commented Feb 12, 2021 •

edited

Loading

akien-mga commented Feb 12, 2021

jimmyjonezz commented Feb 13, 2021

hilfazer commented Feb 25, 2021

Yet another node copy-paste PR #34892

Yet another node copy-paste PR #34892

Conversation

KoBeWi commented Jan 7, 2020

YeldhamDev commented Jan 7, 2020

KoBeWi commented Mar 12, 2020

pouleyKetchoupp left a comment

Choose a reason for hiding this comment

pouleyKetchoupp Jun 20, 2020

Choose a reason for hiding this comment

KoBeWi Jun 22, 2020

Choose a reason for hiding this comment

pouleyKetchoupp Jun 22, 2020

Choose a reason for hiding this comment

KoBeWi commented Jun 22, 2020

pouleyKetchoupp commented Jun 23, 2020

KoBeWi commented Jun 23, 2020

pouleyKetchoupp commented Jun 26, 2020

groud left a comment

Choose a reason for hiding this comment

KoBeWi commented Feb 9, 2021

KoBeWi commented Feb 9, 2021

KoBeWi commented Feb 12, 2021

reduz commented Feb 12, 2021 • edited Loading

fire commented Feb 12, 2021

KoBeWi commented Feb 12, 2021 • edited Loading

akien-mga commented Feb 12, 2021

jimmyjonezz commented Feb 13, 2021

hilfazer commented Feb 25, 2021

reduz commented Feb 12, 2021 •

edited

Loading

KoBeWi commented Feb 12, 2021 •

edited

Loading