Make the "internal node" feature more accessible

Uniformbuffer3 commented 2 years ago

Describe the project you are working on

I'm developing an RPG as solo developer using Godot 4 and i found the new "internal mode" feature very useful to add nodes that are supposed to not be moved/removed or to be handled by a specific node. Unfortunately i found some limitations i think they could be easily enough overcome to worth the invested effort and the usability gain.

Thanks in advance for the dedicated time reading this proposal.

Describe the problem or limitation you are having in your project

The new Godot 4 feature of internal nodes is very useful, it is possible to pass a flag on the add_child function of a Node to set the child as internal. Unfortunately that's the only way to achieve that and there are cases where having only such option is limiting, for example:

Child nodes already added to the tree cannot be switched as internal; to do so it is necessary to remove them from the parent and re-add them. This cause the triggering of the "child added" signal and most of the times forces to define special cases for "internal" nodes to distinguish them from the normal ones.
It is not possible to check if a node is "internal" or not, at least not easily: the only way i found is to check if such node is inside the parent get_children(false); if it is not is "internal". This requires to iterate over all the children, that is not very efficient. as pointed out, it is possible to check the internal status of a node by looking into its metadata. Still i think an editor visible flag would be more intuitive solution.
Nodes cannot be set as "internal" from the editor, so it is required to implement in the _ready() function the "remove and re-add" method described in the first point to achive the goal.

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

A simple solution could be to expose the internal mode of a Node as an enumerator, something that could be represented with GDScript as:

@export
var internal_mode: Node.InternalMode = InternalMode.INTERNAL_MODE_DISABLED

The child Node will be moved accordingly when such variable is modified. If a child change its internal_mode from INTERNAL_MODE_FRONT to INTERNAL_MODE_DISABLED, it will be now the first node among the normal ones. If a child change its internal_mode from INTERNAL_MODE_BACK to INTERNAL_MODE_DISABLED, it will be now the last node among the normal ones.

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

From the editor, a new member would appear in the Node section, allowing to select if a node should be considered internal or not; it would appear the same as exporting a normal enumerator. From the code it would be possible to change such state with something like:

var node: Node = node_reference
node.internal_mode = Node.InternalMode.INTERNAL_MODE_FRONT

Then the parent will react to such change and moves the child accordingly.

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

With a simple structure like:

ParentNode
- InternalNode
- OtherNode

on the "ParentNode" such code is currently required:

func _ready():
    var internal_node = $InternalNode
    self.remove_child(internal_node)
    self.add_child(internal_node,false,Node.INTERNAL_MODE_FRONT)

# If the parent usually listen for new children, it is required to distinguish normal nodes
# from the internal ones, that sometimes it is not so easy. 
func _on_parent_node_child_entered_tree(node):
    if node.name != "InternalNode":
        # Do other operations

Of course, the more nodes are required to be internal, the more code is required.

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

I think node related features should be core.

SilencedPerson commented 2 years ago

It is not possible to check if a node is "internal" or not, at least not easily: the only way i found is to check if such node is inside the parent get_children(false); if it is not is "internal". This requires to iterate over all the children, that is not very efficient.

you could set metadata set_meta("is_internal",true) in all honesty though, i have no idea what does this new feature do.

Uniformbuffer3 commented 2 years ago

Oh, i didn't know about metadata, thanks for the hint! Glad there is a way to do it, but still, i think this method is hidden enough to get overlooked or missed (like i did), so the issue still hold.

For context, the "internal node" feature modify most of the node related functions. Now the Node.add_child() requires a enum value of type Node.InternalMode to specify if the node should be added as "internal" or not. This is what is written on the documentation, that explain how it works very clearly:

enum InternalMode:

INTERNAL_MODE_DISABLED = 0 Node will not be internal.
INTERNAL_MODE_FRONT = 1 Node will be placed at the front of parent's node list, before any non-internal sibling.
INTERNAL_MODE_BACK = 2 Node will be placed at the back of parent's node list, after any non-internal sibling.

Most of the node related functions now ask for a boolean flag called "include_internal" to specify if internal nodes should be taken into account. For example, using Node.get_children(false) will NOT include internal nodes inside the returned children array.

Diddykonga commented 2 years ago

I believe a better implementation would be using methods over direct property access, as it would change the SceneTree which is an action: set_internal(Node.InternalMode mode) is_internal() -> Node.InternalMode Add Set As Internal Right-Click menu option, which expands to allow choosing Front or Back. (Only problem with this is there is no way to disable a node from being internal from the editor, as the main point of being internal is it not showing in the editor.)

Dfred commented 1 year ago

Building encapsulated GUI widgets with the editor is tricky, setting internal node from the editor could help removing a lot of code.

AThousandShips commented 1 year ago

The current implementation uses the order of child nodes, which is highly efficient, how would this proposal handle this, how would it accomplish changing the status of the node?

Dfred commented 1 year ago

The proposal could use the current re-order of the child nodes upon call to add_child() and remove_child() by calling these methods. From the editor, setting the internal mode through an approach similar to @export var internal_mode : Node.InternalMode could change the node order but that might not be much of an issue: when manually designing with the editor, too many nodes elicit saving a node-heavy branch as scene.

IMHO, if the editor could allow setting the internal mode of a Node, that would:

remove workaround code blocks calling remove_child() then add_child() with Node.INTERNAL_MODE.. flags;
remove clutter introduced by @export var internal_mode : Node.InternalMode for each such node;
avoid complicating workflow with side-effects introduced by previous points;
allow code to filter out internal nodes from get_children(true) testing a property, as opposed to subtracting this call's result to get_children(false)

Dfred commented 1 year ago

Your approach means we can only get children and their counts in linear time if we want to ignore internal nodes, losing performance, how is this an improvement?

Can you elaborate on this? From my (non-expert) point of view - if the implementation relies entirely on the current API and only modification is a new property to Node (set upon call to add_child()) - there should be close to no impact on performance. The filtering I was mentioning as an example of using the new property is for the client code to implement.

With hindsight I realize my wording in the previous message is confusing, as I meant for point 4. to retrieve internal nodes only, not filter them out. Apologies.

AThousandShips commented 1 year ago

Edit: My bad I misread it as a workaround in the editor

The only missing part really is that you can't access if a node is internal or not from script, you can do everything else with script and easily, though the ability to move from internal to non-internal would be useful it's kind of potentially messy, and I'm honestly not sure what the use case would be for changing internal status

Extracting only internal children could also be done effectively in code, but I don't see much reason for that specific use

AThousandShips commented 1 year ago

So, there's arguably good reason to expose the is_internal_front/back functions of Node, it's useful and no real reason not to

But what is the use case for changing internal mode of a node specifically? What situation would this be required that means that you can't just use the technique of swapping it

But more importantly, for the OP: Internal nodes are not supposed to be in the scene as such, they won't be saved, so there's no real good reason to add them in the editor like that, what you instead should do is just add a child node in your node and add it as internal, the editor won't even show them so why would it be useful to add a node in the editor and make it internal?

The nodes that are in the tree in the editor are only supposed to be there if they are part of the scene file, I think there's a confusion over what internal nodes are and how they work here

To clarify: Internal nodes are nodes that are in the scene but not saved or loaded with the scene, like the PopupMenu associated with a MenuButton, they are created and managed by the parent node entirely and not by the editor proper, so the current system is (almost) enough and sufficient, internal nodes shouldn't be created by the editor but by the controlling node

What situation do you have where it is relevant for a node to know if it is internal? Are you sure you really need to know that?

Dfred commented 1 year ago

@AThousandShips thanks for the elaborated response, much appreciated. The use case for me is to design custom containers that have nested containers, like ScrollContainer. As such containers might have several internal nodes, there's significant overhead removing/adding children at runtime, and honestly I'm still struggling to find a satisfying approach for tool mode: metadata is my current way to go.

markdibarry commented 1 year ago

I'm also struggling to come up with a good workflow for custom containers where the child cannot be a direct child of the top node, and needs to be the child of a child node. For this, a user is required to enable Editable Children in order to add content to the container as a whole (to the ClipContainer node). However, every time this happens, it exposes the arrows and scrollbar branches as well, which the user should never touch (or see?). It also just would be great if the user didn't need to enable Editable Children at all, but I think that's a separate (however related) issue.

Eclextic commented 7 months ago

Working on UI/Tooling in the last couple of months has shown me, how incredible this feature actually is! I really hope such a simple change gets implemented soon.

It's not, that it isn't doable without it, it's just, this is a HUGE QoL improvement to make code more readable and clean up the editor of unnecessary junk nobody wants to see!

kitbdev commented 7 months ago

I'm also struggling to come up with a good workflow for custom containers where the child cannot be a direct child of the top node, and needs to be the child of a child node.

dxdesjardins commented 1 day ago

Thought I would share a quick C# extension method that will return if a node is Internal. This works because GetChildren() does not return internal children by default.

public static bool IsInternal(this Node node) {
    Node parent = node.GetParent();
    if (parent == null || parent.GetChildren().Contains(node))
        return false;
    return true;
}

Eclextic commented 16 hours ago

Thought I would share a quick C# extension method that will return if a node is Internal. This works because GetChildren() does not return internal children by default.
public static bool IsInternal(this Node node) {
    Node parent = node.GetParent();
    if (parent == null || parent.GetChildren().Contains(node))
        return false;
    return true;
}

This is cool and all but I'm sorry to burst your bubble...

It is not possible to check if a node is "internal" or not, at least not easily: the only way i found is to check if such node is inside the parent get_children(false); if it is not is "internal". This requires to iterate over all the children, that is not very efficient.

you could set metadata set_meta("is_internal",true) in all honesty though, i have no idea what does this new feature do.

You can already do that with GDScript...

godotengine / godot-proposals