Fix Editable Children issues with node renaming, moving, duplicating and instancing.

[hilfazer]

Hopefully fixes all the issues on KoBeWi`s tracker:
#33790

I'm aware it won't get merged to 3.2 but 4.0 is too unstable ATM to work with. Also i didn't managed to compile it yet.

I'm putting it here so people can test it properly. I did some testing myself but there's only so much one person can test (:

Bugsquad edit:
Fixes #28861
Fixes #27064
Fixes #23979
Fixes #20409
Fixes #27023

[KoBeWi]

You could remove the first argument from set_editable_instance to simplify code a lot. Instead of

EditorNode::get_singleton()->get_edited_scene()->set_editable_instance(node, false);

you could do

node->set_editable_instance(false);

Same with is_editable_instance.

Aside from that, the code looks ok. Still, I assume there was a reason to keep editable instances in scene root instead of a property, so we should ask @reduz about that first.

EDIT:
So this is what he said on IRC:

I think this also ensures that editable is per scene and not per subscene. If you instantiate a scene that instantiates a scene using editable children, there is no point in propagating that

[hilfazer]

@KoBeWi Sure, i can do that.
But what about an error check from Node::set_editable_instance(Node *p_node, bool p_editable)?

ERR_FAIL_COND(!is_a_parent_of(p_node));

Put it before that node->set_editable_instance(false); ?

[KoBeWi]

Not sure if this check is necessary. But yeah, you can put it before that.

EDIT:
btw, I didn't check that when testing, but make sure that the property is not available in the inspector nor in the GDScript.

[hilfazer]

I think i'll skip renaming those methods. is_editable_instance is called in many places and i'd need to make sure there's a check for nullptr everywhere. Too big potential for a mistake.

The less i'll change the less i'll break.

I don't see editable_instance neither in the Inspector nor in GDscript.

[hilfazer]

As for reduz's comment (assuming i understood correctly) - my implementation keeps the desired behaviour in following cases:

• instancing an inherited scene
• instancing a scene that contains an instanced scene

but doesn't do the right thing in one case

• merging from a scene that contains an instanced scene (a subscene)

Editable Children checkbox gets propagated if you merge. Which leads to a nasty bug where a node has 2 children with same names. I'll see what i can do about it.

Edit:
That nasty bug with 2 identically named children wasn't introduced by my PR. I just reproduced it on 3.2.2 RC1 creating all scenes from scratch.

Reproduction steps are in Pigdev's issue: #27023
Edit: There's a bug report for that: #31744

[hilfazer]

I've changed Subscene Editor so it clears editable children checkbox when reparenting nodes.

[KoBeWi]

Someone brought up the issue on Twitter, so I took another look at this PR.

I think I understand what problem reduz mentioned (but not why is this a problem, heh). Here's a small project that displays this:
ReproductionProject.zip

It has SceneA, SceneB instancing SceneA with editable children and SceneC instancing SceneB.

Now, when you go to SceneC and turn on editable children in the SceneB instance:

• without this PR, only SceneB will have editable children. In the tscn file you will see

[editable path="SceneB"]

• with this PR, SceneA will also have editable children, because it was saved in the instance. In the tscn file you will see

[editable path="SceneB"]
[editable path="SceneB/SceneC"]

To prevent this, when instancing a scene, we should check whether its parent (ancestor) is an instance. If it's an instance and doesn't have editable children enabled, discard the editable children for this node.

[KoBeWi]

Ok, so this is impossible. The editable instances are set when PackedScene is being instanced and it doesn't know yet whether it has parent scenes. So we'd have to clear the editable instances from parent scene (i.e. a scene instances and checks if child scenes have editable instances they shouldn't have).

But IMO this is much less severe issue than what this PR fixes. I have a pretty big project and never had a scene that would reproduce the "problem" caused by the PR. And even if I had, this doesn't sound like something serious. And the way the editable children are handled currently can lead to data loss, which is rather bad.

EDIT:
Also

I've changed Subscene Editor so it clears editable children checkbox when reparenting nodes.

What does it exactly mean? It doesn't sound like something desired.

[PLyczkowski]

And the way the editable children are handled currently can lead to data loss, which is rather bad.

Can confirm data loss occurring in day-to-day work with how it's set up currently.

[hilfazer]

EDIT:
Also

I've changed Subscene Editor so it clears editable children checkbox when reparenting nodes.

What does it exactly mean? It doesn't sound like something desired.

I made a video to explain that:
https://www.dropbox.com/s/133x1asor6cwytp/GodotEditChldrnMergeFromScene.mp4?dl=0

"Subscene Editor" is used when the user chooses "Merge from Scene" and selects a scene to merge from. A window names "Select node(s) to import" appears. By "reparenting" i meant moving nodes from that new window to the Scene tab.

I did that in order to prevent the very problem you've described for "Merge from Scene" case. But as we can see from the video it doesn't work properly with multiple instantation levels. I'll see what i can do about it.

[KoBeWi]

This looks the same as the issue I mentioned in #39533 (comment)
If you can fix this then ok, but it's not necessary to do in this PR.

[hilfazer]

I've added a recursive_disable_editable_children() function that unchecks "editable children" in all nodes added with "Merge From Scene" option. Now merging nodes with multiple levels of instantation won't propagate their "editable children" setting.

And... i've found an inconsistency with "Merge From Scene" in Godot 3.2.1 stable. The video will explain it better than i ever could:
https://www.dropbox.com/s/tqjgux6hhsfrvp7/Godot321InconsistentMergeFromScene.mp4?dl=0

With my recent change all those checkboxes are unchecked every time.

[KoBeWi]

And... i've found an inconsistency with "Merge From Scene" in Godot 3.2.1 stable.

That's #28861

[KoBeWi]

I checked if this PR fixes all issues from my tracker and seems like it doesn't fix #27023

When merging from other scene, the editable children should be preserved, because it's the same as copy-pasting a node to another scene.

[hilfazer]

Ok, so i've reverted any changes specific to "Merge From Scene".
I went through all issues from the tracker to make sure they are all fixed (i've checked that on 3.2.4 beta build).

Edit:
Not all problems with duplicating are gone. If there are multiple levels of scene instantation then edited properties that are more than 1 level of scene instantation deep won't get copied (hope that makes sense). I've already done some work on that and it looks promising but it will require current PR to be merged.

[KoBeWi]

This should be ok now.

Here's a patch for master:
Patch.zip
Just use git apply file.patch and open a new PR against master. Although I'd wait for @reduz to approve it too. I explained him on IRC how this works and he said that it should be fine (the change is fully backwards compatible and doesn't change how the information is stored in scenes).

[djrain]

So I just discovered a related problem in my project and I don't know whether this PR addresses it or not.

Basically I have a level editor scene where I use PackedScene.pack() to save out arbitrary branches of the tree (and leave that branch there, local, for further editing, as opposed to "save branch as scene"). I recursively loop over all the node's subnodes and set their owner so PackedScene will include them.

The problem is that in the freshly saved scene, any instances are now expanded (I can see and edit the children). Almost as if I manually checked "editable children" on the instance, except that this box is still unchecked, and the subnode names are not greyed out as they would normally be (though their inspector properties do display in red). And if I then try to check editable children, new children get added and I have duplicate node names.

Is this a known problem, or should I open a new issue?

[KoBeWi]

Is this a known problem, or should I open a new issue?

It's not even a bug. All nodes whose owner is the scene root will have their children visible. Instanced scenes have owner pointing to themselves instead of the scene root, hence their children are hidden by default. The "editable children" property basically exists so that instanced nodes could have their children displayed, even if their owner is different.

From what you say, all your nodes have the same owner, so it's natural that they all show.

[djrain]

Instanced scenes have owner pointing to themselves instead of the scene root, hence their children are hidden by default.

I was under the impression that a node had to have the edited_scene_root as its owner, otherwise it wouldn't be visible at all? Perhaps you can tell me how to accomplish this, as I don't see how one can make instances own themselves after packing the scene.

And aren't subnodes of an instance supposed to always have their names greyed out? If so, this is buggy because they are still white after the save. And the children with identical names aspect is a bug, although I suppose that's been addressed already...?

[hilfazer]

Instanced scenes have owner pointing to themselves instead of the scene root, hence their children are hidden by default.

I was under the impression that a node had to have the edited_scene_root as its owner, otherwise it wouldn't be visible at all? Perhaps you can tell me how to accomplish this, as I don't see how one can make instances own themselves after packing the scene.

And aren't subnodes of an instance supposed to always have their names greyed out? If so, this is buggy because they are still white after the save. And the children with identical names aspect is a bug, although I suppose that's been addressed already...?

Sibling nodes with same name is indeed a bug and it's been reported: #31744
The bug reporter has came up with a solution and asked reduz and Akien for guidance.

This PR isn't addressing that problem.

[akien-mga]

We'll need a master PR with those changes before this can be considered for merging, as we don't fix bugs in 3.2 to keep them in future releases.

[akien-mga]

Master PR: #45265.

[akien-mga]

Thanks!