godotengine / godot-proposals

Godot Improvement Proposals (GIPs)
MIT License
1.17k stars 98 forks source link

Add ability to flag a "Node Slot" for instanced scenes #5475

Open markdibarry opened 2 years ago

markdibarry commented 2 years ago

Describe the project you are working on

A game framework with a lot of tool scripts and custom container scenes.

Describe the problem or limitation you are having in your project

If I have a hierarchy of nodes that I use often, I'll make a tool or scene of it. However, when I make a new tool/scene for use in the editor, I run into the same issue over and over. If I decide I want, as one example, a GridContainer that has some padding and border, I may make a scene that is set up like this: image

However, the only way to use this scene is to enable Editable Children and to add the children to the specific child of the scene: image

This is cumbersome and cluttered, and requires all the inner-workings of a scene to be displayed when it's made to be used as a single tool.

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

I think it'd be very helpful if you could specify a node in a scene as a "Slot", so when using the scene, nodes added as children of the scene would be actually added as the children of the specified node, but unless Editable Children is enabled, it would appear to be a child of the scene. This way, you can use custom scenes like regular built in nodes without having to enable Editable Children to get the expected behavior.

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

I imagine it's pretty obvious how it would work, but here's an example just to be clear: image

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

Not that I know of, and haven't found anyone who has found a workaround. But happy to be wrong!

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

Editable Children is a core Editor feature.

Mickeon commented 2 years ago

Not that I know of, and haven't found anyone who has found a workaround. But happy to be wrong!

There is a... valid workaround. However, because this is the first time someone has ever suggested this, as far as I know, this situation has not been accounted for. As such, it does not display properly in the Scene Tree Dock.

I believe that improving the visualization on the Scene Tree Dock to account for this niche but very interesting and useful case should be a priority over adding a whole new feature for it.

Here is the sample Script (3.x):

tool
extends PanelContainer

func _ready():
    connect("child_entered_tree", self, "_on_child_entered_tree")

func _on_child_entered_tree(child: Node):
    if (child.owner != self):
        remove_child(child)
        $MarginContainer/GridContainer.add_child(child)

And look: image Making the children editable shows that it works completely fine. image

markdibarry commented 2 years ago

That's pretty much the idea! I guess what would be minimally needed to reproduce this feature is a way to make children of the scene that isn't part of the scene visible in the editor without Editable Children enabled. I suppose the scene itself would need to have a reference to the node where they should be inserted and displayed.

Mickeon commented 2 years ago

I suppose the scene itself would need to have a reference to the node where they should be inserted and displayed.

Do note that, the Editable Children toggle doesn't have much effect on the way PackedScene itself is saved. It's metadata that just basically tells the Scene Tree Editor "Hey, don't let the user see or modify the Nodes that this sub-Scene owns".

In fact, this is the .tscn file of the example above (in 3.x):

[gd_scene load_steps=3 format=2]

[ext_resource path="res://NodeSlottingContainer.tscn" type="PackedScene" id=1] ;<-----------
[ext_resource path="res://icon.png" type="Texture" id=2]

[node name="NodeSlotWorkaround" type="Control"]
anchor_right = 1.0
anchor_bottom = 1.0

[node name="PanelContainer" parent="." instance=ExtResource( 1 )]

[node name="MarginContainer" parent="PanelContainer" index="0"]
margin_right = 164.0
margin_bottom = 159.0

[node name="GridContainer" parent="PanelContainer/MarginContainer" index="0"]
margin_right = 66.0
margin_bottom = 154.0

[node name="TextureRect" type="TextureRect" parent="PanelContainer/MarginContainer/GridContainer" index="0"] ;<-----
margin_right = 64.0
margin_bottom = 64.0
texture = ExtResource( 2 )

[editable path="PanelContainer"] ;<--------------
MikeFP commented 2 years ago

Other UI frameworks solve that problem by making a "Template"/"Content"/"Slot" component available exactly for that purpose. So Godot could include a "Slot Node" that listens for notifications of the parent Node for when a child is added, very much like what @Mickeon proposed.

The only thing left would be to fix the Scene Tree Dock not showing it.

markdibarry commented 2 years ago

Do note that, the Editable Children toggle doesn't have much effect on the way PackedScene itself is saved

Oh of course. I just meant if a scene were to branch in any way, the editor would need to know which node it should be added to. Unless I'm mistaken. Unless it should just show all/any nodes added. Maybe that wouldn't be an issue?

Lielay9 commented 2 years ago

Having something akin to the RemoteTransform2D and RemoteTransform3D for control-derived nodes would go a long way. It would only need to push its size, position, and layer to the remote node and probably receive its minimum size back but that's about it. It wouldn't be quite as straightforward to use as what is proposed here but it would make it possible to affect nodes outside the scene (the remote nodepaths would still be needed in the script of the scene root). Also, a single node would be less work to implement and maintain... probably. "RemoteControl"-node could alleviate most node-slot troubles until a proper system is complete.

MikeFP commented 2 years ago

Although a RemoteTransform for Controls would be useful, it would not cover the use case for when you want to access a grandchild node with relative path, and you want the Scene user to put a child in its instance so it occupies the spot of the grandchild on the tree.

Lielay9 commented 2 years ago

Although a RemoteTransform for Controls would be useful, it would not cover the use case for when you want to access a grandchild node with relative path, and you want the Scene user to put a child in its instance so it occupies the spot of the grandchild on the tree.

You could always replace the RemoteTransform at runtime. Wouldn't obviously work if the node is already required in `_ready() by other nodes but for simple implementations, it works. A criminally unfinished example of implementation:

extends Container
class_name RemoteContainer

@export var replace := false

func _ready() -> void:
    await get_tree().process_frame
    if is_instance_valid(owner):
        for child in owner.get_children():
            if child.owner != owner and child is Control:
                if replace:
                    owner.remove_child(child)
                    replace_by(child)
                    queue_free()
                else:
                    var child_rid : RID = child.get_canvas_item()
                    var parent_rid : RID = get_canvas_item()
                    RenderingServer.canvas_item_set_parent(child_rid, parent_rid)
                    RenderingServer.canvas_item_set_custom_rect(child_rid, true, get_rect())
                break

Could be extended to be a tool-script and do everything dynamically and not just once in ready. But hey it works! kinda...

Once again, I don't suggest this is going to be the best solution - but it can do what is proposed and is simple to implement.

dmchurch commented 1 year ago

It's looking like I'll be addressing this over in #8184, so anyone with opinions about how this gets implemented should weigh in over there :smile:

timothyqiu commented 1 year ago

I'd like to generalize this ability.

Mock

Since the slot panel is simply a view into a subtree.

YuriSizov commented 1 year ago

I think that if slotted nodes are added, then NodePath should also have dedicated slot semantics. If users interact with slots instead of actual paths, then they should be able to rely on them.

Say you're using a node with slots and put a child in there. Then you use a NodePath/node reference in an export field of another node to that child. If that reference, that node path depends on the internal structure of the scene, that's a problem (same as with editable children). Except now it's completely hidden from the user that paths may change internally, since the user relies on these exposed slots, not actual paths.

So paths need to support slots and be able to resolve them without hardcoding fully expanded paths in such references and having to update them (which is slow and unnecessary here).

Mickeon commented 1 year ago

NotePaths having more "node-pointer" logic beyond basically being encapsulated Strings sounds pretty neat on paper. Currently, all of the "node-pointing" logic happens exclusively in Node's own get_node(). I wonder how feasible this is in practice.

YuriSizov commented 1 year ago

@Mickeon I think in this case slots themselves would act a bit like unique scene nodes, so they can be resolved the same way (they can be basically made scene unique nodes implicitly, and that would resolve most of my comment).