godotengine / godot-proposals

Godot Improvement Proposals (GIPs)
MIT License
1.16k stars 97 forks source link

Improve usability of Editable Children #3248

Open reyma24 opened 3 years ago

reyma24 commented 3 years ago

Describe the project you are working on

An immersive sim.

Describe the problem or limitation you are having in your project

I believe that the editable children feature has the potential to be one of the most important part of Godot's scene editor. If improved, it would finally stop all those requests for component systems / multiple inheritance / modularity systems. Godot already has the perfect solution for code modularity - adding nodes. This works perfectly and is increadibly easy to understand and implement: components

The problem is with editing them after instancing the scene. I found 3, in fact:

  1. When the scene contains many nodes, the tree becomes cluttered and shows nodes that are not needed (more in 27828): not_needed

  2. You have to right-click > select editable children every single time, on every single node (more in 22263). editable

  3. The current color of the properties (red) indicates that those properties should not be edited (see this). red

There might be more problems with editable children, these are the ones that I found and think are important things to change / implement.

Describe the feature / enhancement and how it helps to overcome the problem or limitation

Improve editing children's properties in instanced scenes.

Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams

Solutions:

  1. Select which nodes will be editable - there should be an option for setting a node's visiblity when parent node is instanced: editable_in_parent

  2. Show editable nodes by default - there should be an option in the Editor Settings for showing editable children by default (automatically, when node enters tree): new_editor_setting

  3. Remove the red coloring from childrens' properties.

If this enhancement will not be used often, can it be worked around with a few lines of script?

The alternative is to create an addon that shows the children attributes in the parent's inspector, however that also comes with many problems (editor doesn't save changes to non-editable children and you can't attach metadata to instanced scenes).

Is there a reason why this should be core and not an add-on in the asset library?

The feature is already implemented, the only thing it needs are improvements listed above.

The end result: final

Calinou commented 3 years ago
  1. Remove the red coloring from childrens' properties.

I wouldn't remove this coloring as it indicates that your changes may be lost in the future. We can change it to the warning color (yellow) instead to make it less scary.

Edit: Done in https://github.com/godotengine/godot/pull/53260.

NathanLovato commented 3 years ago

I agree with Calinou here, the issue with Editable Children isn't the presentation right now: as with inherited scenes, you have to be extremely careful not to lose your changes.

At GDQuest, we very rarely use Editable Children because of that: it's too easy to change the parent scene or refactor a class and introduce game-breaking bugs you won't detect until days or weeks later.

With that limitation fixed somehow, sure, it'd be a great feature. There was also a proposal to improve inherited scenes and limit or prevent data losses. To me, that'd be the starting point.

Wavesonics commented 3 years ago

Maybe the data loss is really what we should focus on finding a solution for. At the very least, knowing that the data loss has happened (like a bunch of changes visible in your source control) so that it doesn't silently break things.

Can someone explain the root cause of the data loss?

Calinou commented 3 years ago

Can someone explain the root cause of the data loss?

One common cause for this is when you rename a property. There's already an open proposal about adding an annotation to handle property renames, but I can't find it right now.

raulsntos commented 3 years ago

Can someone explain the root cause of the data loss?

One common cause for this is when you rename a property. There's already an open proposal about adding an annotation to handle property renames, but I can't find it right now.

I think you are referring to this one: #3152

TackerTacker commented 3 years ago

I would like a solution that avoids showing the children completely, but it would still be better than what we have right now.

In your the end result image you show the "Attacker" child as selected and the properties you want from that child, the "Script Variables". The Scene tab can become quite cluttered if there are a lot of nodes. Instanced scenes help fight this by creating bigger building blocks out of smaller pieces which get hidden to de-clutter the scene graph, so this would counteract this by partially bringing back hidden away nodes, even though you don't actually want the hidden nodes back, you want to be able to change the values found under "Script Variables" and nothing else. I think it would be much better to make only selected properties, or property groups available directly on the instanced scene itself, without cluttering up the scene dock with child nodes.

justinbarrett commented 3 years ago

I am not a contributor, but I have never had a case where I needed to edit the children.. I have always just reopened the scene, fixed the issue and moved on...This is another reason there are exported variables. That is where I do any changes needed....if someone implements this...it's, ofc, fine...but I feel like this is the wrong path and might teach bad habits. Feels very "corner case".

mark-rafter commented 3 years ago

I would prefer a variant of this where you can selectively override children (by right clicking the node and checking "Override Child").. Currently I'm doing this in code by exporting NodePath but it's a bit tedious and pollutes the code/scene tree, and is susceptible to bugs (e.g. no static type checking).

Current behaviour

// Character.cs
[Export] public NodePath CharacterAnimationTreePath { get; set; } = nameof(CharacterAnimationTree);

public CharacterAnimationTree CharacterAnimationTree { get; private set; }

public override void _Ready()
{
    CharacterAnimationTree = GetNode<CharacterAnimationTree>(CharacterAnimationTreePath);
}

override-current

Proposed behaviour

// Character.cs
public CharacterAnimationTree CharacterAnimationTree { get; private set; }

public override void _Ready()
{
    CharacterAnimationTree = GetNode<CharacterAnimationTree>(nameof(CharacterAnimationTree));
}

override-new

reyma24 commented 3 years ago

I have never had a case where I needed to edit the children.. I have always just reopened the scene, fixed the issue and moved on...This is another reason there are exported variables. That is where I do any changes needed....if someone implements this...it's, ofc, fine...but I feel like this is the wrong path and might teach bad habits. Feels very "corner case".

Here's my thinking, visualized with an example: You can't add the same Destructible script to, for example, both Static and Kinematic bodies - they extend different classes. However, this way you will get all the properties needed in the inspector.

The much better solution is to "extend" them by adding the same node:

the_same_script

Great! But now you can't see and edit those new properties after instancing this scene because the inspector shows only the properties from the classes above.

The solution: Editable Children, hence this proposal.


Edit: I created an alternative proposal for those prefering to change nested nodes with their own, custom scripts: #3264

AttackButton commented 2 years ago

I am not a contributor, but I have never had a case where I needed to edit the children.. I have always just reopened the scene, fixed the issue and moved on...This is another reason there are exported variables. That is where I do any changes needed....if someone implements this...it's, ofc, fine...but I feel like this is the wrong path and might teach bad habits. Feels very "corner case".

This is not a "corner case", look at the likes in the op. I'm using this a lot in my project, in the level design. I'm using nodes from the base scene as a container for some other scenes that will be manipulated via script. The number and type of objects in the containers changes from instance to instance, as the level design requires by the context.

This feature is very important.

Z3R0PTR commented 2 years ago

Hi.

I think, this will be a nice feature. In my projects, I have thousands of Models (FBX-Files). All of them are represented as inherited scenes, where the top node is a simple spatial (3D) that contains one ore more (hierarchical) mesh instances. So if I want to instance a model multiple times within a scene and want to change the material for some of them, i have to edit the children and set the new material for all the mesh instances. This is my biggest problem in level design.

eerbin13 commented 2 years ago

I am currently using a component based code architecture model. Improved Editable Children would be magnificent.

I stumbled across this thread looking for a way to turn Editable Children on by default.

Perhaps a method exposed to GDscript where you could use it in an _on_ready() with the tool keyword at the top of the script?

I was looking for a function inside Node called set_editable_children(state: bool) before I did my Google deep dive.

MatthijsMud commented 2 years ago

This could also be useful for creating "slots" if the various ancestor nodes aren't shown in the hierarchy.

Granted, in case of Spatial and Node2D this could be worked around by creating nodes in the root that are targeted by RemoteTransform and RemoteTransform2D respectively.

For Control nodes there currently exists no such alternative (that I'm aware of). Yet one might be interested in adding items to a "deeply" nested Container representing the content (of a custom container built out of different nodes). An accordion could be an example of such a component.

This would allow viewing the results directly in the editor rather than having to rely on runtime (or Tool scripts) that move nodes around/create scene instances.

Current editable children situation With improvements to editable children
![image](https://user-images.githubusercontent.com/11519728/180975813-ade89ed6-11bf-464f-b559-c1abf344e98f.png "Scene tree showing deep hierarchy of a character, where a \"Sword\" is the child of \"Right hand\"") ![image](https://user-images.githubusercontent.com/11519728/180977472-1f4b655a-94aa-4745-a1c8-6a91b61c5396.png "Scene tree showing only \"Left hand\" and \"Right hand\" as children of instanced scene as slots, where a \"Sword\" is the child of \"Right hand\"")
BlockyDK commented 2 years ago

I'm not entirely sure if this fits here, but I've just run into a use case that I think touches upon this discussion.

Considering the node based structure of Godot, it seems most intuitive to me to give quick and easy control over which properties from children nodes will be visible from the root node when instanced in another scene. And of course while not actually exposing the children to the scene tree in order to keep everything modular and compact. While this functionality can be achieved using @tool, there might be a better way.

Consider this prefab I put together, it is made up of several parts, a stand, a rotating brace, a tilting lamp and finally a spotlight.

EditableChildren_01

in the tool script I allow the user to rotate and tilt the head of the lamp with a simple slider, and as this is custom behavior, the tool script made sense here. And it is all in one tidy little node.

EditableChildren_03

EditableChildren_02

However, I also wanted the ability to control the properties of the spotlight. This ended up resulting in a rather long tool script just exposing all of the properties from the spotlight. At first I only exposed a few (as seen above), but as time went on I found that I continually needed access to more and more of the light's properties.

In this scenario, I found myself wishing for the ability to easily select properties from the child and have them auto expose in the root when instantiated in another scene. Or even better, a one click solution to have all of the selected child's properties exposed. Two approaches came to mind, one where the user would right click on the property in the child and the option would pop up to expose that property in the root when instantiated.

EditableChildren_04

Another would be to add a checkbox in the properties for the child (so basically a property inherited from a basic node) that would just expose everything from that child in the root when instantiated.

Then ideally, in the case of this prefab for instance, when I select the instantiated prefab, over in the inspector, I should just see a dropdown for the spotlight child and all of it's properties (provided the box is checked under the spotlights properties from within the actual prefab's scene. Obviously this property would have to be excluded when exposing all the others.

That is just my two cents, I think it would work well with Godot's node based workflow.

CookieBadger commented 1 year ago

As a level editor, this is one of my main pain points of the Godot Engine. I have so many cases where I need to edit children, when making a level.

  1. There is no way to make children editable per default when instancing a scene (either global or per scene resource)
  2. There is no hotkey to make the children of the current node editable
  3. There is no way to hide any children, that you don't want to be exposed

@justinbarrett You don't have to be a contributor to come across many use cases for this. The moment you need two instances of a StaticBody with different Collision Shape sizes, you need to make children editable.

When you constantly have to instance new scenes, make their children editable, expand an abysmal hierarchy and find the one thing you are supposed to edit, this is detrimental for the workflow. More clicks, more searching, more typing on the level editor's end, which could be avoided, by the person who designed the feature, is always bad.

As an improvement, I think the easiest improvement could be adding the hotkey. Then, the solutions from @reyma24 's proposal would be a boon for level editing. Finally, @BlockyDK 's selectively exposed properties can be nice, but that would limit it to properties in the inspector, compared to using editor viewport gizmos for e.g. transformations.

Vukbo commented 1 year ago

I totally agree with the proposal of @ExquisiterEmil. This would be a tremendous level up regarding level design in the editor...

realkotob commented 1 year ago

I made a PR that partially addresses this, let me know what you think

https://github.com/godotengine/godot/pull/70947

CookieBadger commented 1 year ago

@realkotob cool, from a level design standpoint, I can see this being very useful already! As a system designer, you often don't want the entire child node hierarchy visible to the level designer, so I am still of the opinion that something like solution 1 from the original proposal should be implemented as well.

Huraqan commented 1 year ago

Noone seems to talk about the loss of data... (EDIT: you guys did talk about it, my bad.) I lost half a level because of editable children. I saved a scene of my level to load it in a new project and most of the polygon2D nodes got reset. Well, actually the saved scene lost most of the polygon data, kept only one PoolVector2Array and put it in every Polygon2D. This happened only the moment I saved the scene and reimported it...

realkotob commented 1 year ago

@ExquisiterEmil I fully agree with you that adding a system to expose properties selectively would be useful, I can work on that as a second step after the first PR is accepted :)

realkotob commented 1 year ago

@Huraqan The data loss is an important point but it is a completely different issue.

There is a different proposal for how to fix the data loss issue that I will try to start working on soon. https://github.com/godotengine/godot-proposals/issues/3152

Huraqan commented 1 year ago

The link you shared shows a proposal about export variables and has nothing to do with the issue of losing data when saving and loading a scene using editable children. I don't understand why you shared that link.

On Mon, 16 Jan 2023, 21:09 Kotob, @.***> wrote:

@Huraqan https://github.com/Huraqan The data loss is an important point but it is a completely different issue.

There is a different proposal for how to fix the data loss issue that I will try to start working on soon. #3152 https://github.com/godotengine/godot-proposals/issues/3152

— Reply to this email directly, view it on GitHub https://github.com/godotengine/godot-proposals/issues/3248#issuecomment-1384505986, or unsubscribe https://github.com/notifications/unsubscribe-auth/AETQELD4D4I4TY7GRM4OEF3WSWTHZANCNFSM5DL6I5WQ . You are receiving this because you were mentioned.Message ID: @.***>

realkotob commented 1 year ago

@Huraqan That issue is part of the losing data problem with editable children in instances, but yes you are right it is not related to saving and loading a scene using editable children.

Is there an existing issue for that? Otherwise we should create one

Huraqan commented 1 year ago

I just searched the repository and couldn't find an issue related to my problem. I'll try to reproduce it and open up an issue.

On Tue, 17 Jan 2023, 10:06 Kotob, @.***> wrote:

@Huraqan https://github.com/Huraqan That issue is part of the losing data problem with editable children in instances, but yes you are right it is not related to saving and loading a scene using editable children.

Is there an existing issue for that? Otherwise we should create one

— Reply to this email directly, view it on GitHub https://github.com/godotengine/godot-proposals/issues/3248#issuecomment-1385058864, or unsubscribe https://github.com/notifications/unsubscribe-auth/AETQELHLYVPAXVZOZA4OU4LWSZOKXANCNFSM5DL6I5WQ . You are receiving this because you were mentioned.Message ID: @.***>

FeldrinH commented 1 year ago

If @reyma24's 'Editable In Parent' checkbox is ever implemented then I would propose that the data loss risk could be reduced by showing a warning/confirmation dialog whenever the user attempts to change a child node that is marked as editable in a way that would result in data loss. So for example if you attempt to rename an editable child the editor could show a popup that says 'Renaming this node will lose any changes made to its properties in instances of this scene' with 'Ok' and 'Cancel' buttons.

frandmb commented 1 year ago

This would be extremely useful for creating reusable components without cluttering the interface or needing the manual step of exposing the children when they have required configuration. Either exposing the children directly or displaying selected children's properties on the inspector itself for the parent node/scene view

3lis4 commented 1 year ago

I found myself wishing for the ability to easily select properties from the child and have them auto expose in the root when instantiated in another scene.

Being able, in a scene, to choose on every node which property should be shown on the root when this scene is instantiated in another scene seems very useful.

We could also add an annotation to be able to specify in the root script which child properties we want to expose. This would be the script equivalent of the editor UI "expose in parent" option.

In a similar issue we imagined a syntactic sugar to help with exposing child vars : https://github.com/godotengine/godot-proposals/issues/7803#issuecomment-1732402845

I think this proposal might be interesting as a syntactic sugar to limit the amount of "useless" setters when many exported variables of the child nodes need to be exported as well on the parent node.

It could be an annotation exposing a child node's exported variable. Something like @export_child(path_to_child_node:var_name) in parent node's script.

This @export_child was supposed to be just a syntactic sugar replacing an export var and its setter in the root script, which has two drawbacks :

But @export_child could be great as the script equivalent of the editor "expose in parent" option.

There is also this interesting discussion on reddit: https://www.reddit.com/r/godot/comments/173zn9j/how_do_you_handle_encapsulation_and_passing_data/

foolmoron commented 10 months ago

Here's another use case in support of reyma's proposal 1 to mark nodes as editable in parent. I have a CollisionPolygon2D as part of a scene and I want to edit the polygon in each instance. If I just exposed the polygon property, I would have to edit the array manually, but if I can expose the entire node, then I get the nice editor controls.

Of course, the option to expose specific properties would be very useful too. But I think exposing nodes is more important since it covers all use cases.

image

cursegate commented 10 months ago

I agree that exposing child nodes to edit is the true solution to this problem.

We can use the editor to compose scenes without writing code, which is awesome. The problem is that we can't give a scene an intuitive API without code. A designer can't even tell if a scene's children require dependencies without enabling Editable Children and scrolling through.

I don't think the solution should be script-level on children, because a child node without a script attached can still have dependencies. Also, you might want to enable this feature for one node, but disable it for another node with the same script.

I don't think "Editable in Parent" is technically accurate, though. Something like "Editable in Instance" or "Exposed Outside Scene" sounds better to me.