Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add the ability to expose nodes for direct access in instantiated scenes #84018

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

yahkr
Copy link
Contributor

@yahkr yahkr commented Oct 26, 2023

Updated 11/1/2024

Description

This pull request implements a feature that significantly enhances Godot's scene editing capabilities. It allows specific nodes within a scene to be exposed, making them visible and allowing their properties to be overridden when the scene is instantiated elsewhere. I believe it is an improved version of editable children.

  • By exposing only relevant nodes, the scene tree remains uncluttered, which is especially useful for creating reusable scenes (Custom containers, Generic windows).
  • This PR adds the ability to expose nodes on import, for use when wanting to expose parts of a scene (such as the hand bone of a character for equipping).

Note

  • Node exposure only propagates up one level of instantiation
  • Nodes that are exposed can be re-exposed

Important

The use of unique names has been removed from this PR (#84018 (comment)) as there were too many issues with it, once #86960 is merged this PR should function like originally planned

Example 1

Lets say we have this window scene that we want to re-use everywhere we can exposed the title label and the content nodes:

image

With this PR we can modify the properties of the exposed nodes and append child nodes to them, this lets us create super flexible scenes and use them like so:

image

and this is the same scene with editable children enabled. Far messier and poorer UX

image

Example 2

For a simple scene like the following, we expose the Sprite2D, the resulting tscn looks like this:

image
image

[node name="scene_0" type="Node2D"]

[node name="Parent" type="Node2D" parent="."]

[node name="Exposed_0" type="Sprite2D" parent="Parent"]
+ exposed_in_owner = true
texture = ExtResource("1_mdjal")

+[exposed path="Parent/Exposed_0"]

In another scene we instantiate this simple scene and override the exposed node's rotation property and add a child node to the exposed node. We also modify the color of the exposed node to orange

image
image

.tscn with this pr using exposed nodes:

[node name="scene_1" type="Node2D"]

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

[node name="Exposed_0" parent="scene_0_instance/Parent" index="0"]
self_modulate = Color(1, 0.666667, 0, 1)

[node name="Child_Of_Exposed" type="Sprite2D" parent="scene_0_instance/Parent/Exposed_0" index="0"]
position = Vector2(100, 30)
scale = Vector2(0.5, 0.5)
texture = SubResource("CompressedTexture2D_3sd7o")

.tscn with editable children and doing the same thing:
image

[node name="scene_1" type="Node2D"]

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

[node name="Exposed_0" parent="scene_0_instance/Parent" index="0"]
self_modulate = Color(1, 0.666667, 0, 1)

[node name="Child_Of_Exposed" type="Sprite2D" parent="scene_0_instance/Parent/Exposed_0" index="0"]
position = Vector2(100, 30)
scale = Vector2(0.5, 0.5)
texture = SubResource("CompressedTexture2D_3sd7o")

+ [editable path="scene_0_instance"]

Sample Project

TODO

  • Continue to test

I think that this pr addresses the following proposals:

@AThousandShips AThousandShips added this to the 4.x milestone Oct 26, 2023
@fire fire changed the title first pass - proof of concept Expose nodes inside of an instantiated scene instead of "Editable Children" - proof of concept Oct 26, 2023
@jcostello
Copy link
Contributor

This will work with imported gltf scenes?

@yahkr
Copy link
Contributor Author

yahkr commented Oct 27, 2023

This will work with imported gltf scenes?

I havn't looked into that aspect, but would be a nice feature to add to this as well

EDIT 11/2/2024:

Yes!

@AThousandShips
Copy link
Member

Also please update your branch by rebasing instead of merging, important skill to get used to with contributing, see the pr workflow for details

@yahkr yahkr force-pushed the exposed_in_owner branch 2 times, most recently from aacbd8c to c85dded Compare October 28, 2023 17:34
@yahkr yahkr closed this Nov 23, 2023
@AThousandShips
Copy link
Member

You have reset your branch and this closes the PR, if you update your branch this can be reopened

@AThousandShips AThousandShips removed this from the 4.x milestone Nov 23, 2023
@yahkr
Copy link
Contributor Author

yahkr commented Nov 24, 2023

Sorry, still trying to get a grip on correct way of updating my repo from main while keeping my changes. I've pushed my new changes up. This includes a somewhat functional version of this pr.

@yahkr yahkr reopened this Nov 24, 2023
@AThousandShips AThousandShips added this to the 4.x milestone Nov 26, 2023
@yahkr yahkr force-pushed the exposed_in_owner branch 7 times, most recently from 3233eac to 5257a35 Compare December 3, 2023 19:19
@yahkr
Copy link
Contributor Author

yahkr commented Nov 19, 2024

@yahkr
Copy link
Contributor Author

yahkr commented Dec 12, 2024

  • updated to 19e003b to resolve merge conflicts

EDIT:

  • updated to 4364ed6 to resolve merge conflicts
  • updated to 6395450 to resolve merge conflicts

@yahkr yahkr force-pushed the exposed_in_owner branch 3 times, most recently from 4b5a1d3 to 38b8eb0 Compare December 18, 2024 14:24
@timoschwarzer
Copy link
Contributor

Currently there seems to be an issue with connecting signals to exposed nodes.

Signals won't get connected even though the editor shows it in the first place. When closing and reopening the scene after connecting a signal, the connection is lost. Trying to connect the signal again crashes the editor.

Screencast.From.2024-12-23.02-05-12.mp4

@yahkr
Copy link
Contributor Author

yahkr commented Dec 23, 2024

Currently there seems to be an issue with connecting signals to exposed nodes.

Signals won't get connected even though the editor shows it in the first place. When closing and reopening the scene after connecting a signal, the connection is lost. Trying to connect the signal again crashes the editor.
Screencast.From.2024-12-23.02-05-12.mp4

thanks for testing, will take a look

@timoschwarzer
Copy link
Contributor

It seems that META_EXPOSED_IN_INSTANCE was supposed to be checked for when serializing signal connections instead of META_MARKED_FOR_EXPOSURE. This at least fixed connections not being saved. Please take a look whether this makes sense because I'm only guessing 😅

The crash part of the issue was unrelated to this PR.

diff --git a/scene/resources/packed_scene.cpp b/scene/resources/packed_scene.cpp
index f916875c0d..9b189babd2 100644
--- a/scene/resources/packed_scene.cpp
+++ b/scene/resources/packed_scene.cpp
@@ -1090,7 +1090,7 @@ Error SceneState::_parse_node(Node *p_owner, Node *p_node, int p_parent_idx, Has
 
 Error SceneState::_parse_connections(Node *p_owner, Node *p_node, HashMap<StringName, int> &name_map, HashMap<Variant, int, VariantHasher, VariantComparator> &variant_map, HashMap<Node *, int> &node_map, HashMap<Node *, int> &nodepath_map) {
        // Ignore nodes that are within a scene instance.
-       if (p_node != p_owner && p_node->get_owner() && p_node->get_owner() != p_owner && !p_owner->is_editable_instance(p_node->get_owner()) && !p_node->has_meta(META_MARKED_FOR_EXPOSURE)) {
+       if (p_node != p_owner && p_node->get_owner() && p_node->get_owner() != p_owner && !p_owner->is_editable_instance(p_node->get_owner()) && !p_node->has_meta(META_EXPOSED_IN_INSTANCE)) {
                return OK;
        }

@yahkr
Copy link
Contributor Author

yahkr commented Dec 26, 2024

It seems that META_EXPOSED_IN_INSTANCE was supposed to be checked for when serializing signal connections instead of META_MARKED_FOR_EXPOSURE. This at least fixed connections not being saved. Please take a look whether this makes sense because I'm only guessing 😅

The crash part of the issue was unrelated to this PR.
...

Thanks @timoschwarzer, that was definitely meant to be EXPOSED vs MARKED, this didn't immediately fix the saving for me, however adding in checks like I have for _parse_node did, probably due to the multiple inheritance of my test scene. Thanks for the bug find!

  • fixed _parse_connections not saving signals from exposed nodes or children of exposed nodes in instanced scenes
  • updated to 0f95e9f

@novhack
Copy link

novhack commented Jan 4, 2025

I've been using this PR for some time merged on top of the most recent master and I love it.

  1. I noticed a tiny issue that sometimes it seems that instanced scenes with exposed child nodes do not respect the order of how these children are in the scene tree.

image

image

In this case RoomOne and RoomTwo collision polygons are switched around in the instanced scene.

  1. It also seems that renaming an instanced scene with exposed nodes breaks the feature. Clicking exposed nodes doesn't show their properties nor signals anymore. It's required to close and reopen the scene with the instanced scene.

@yahkr yahkr force-pushed the exposed_in_owner branch from a56e24d to 0d2217e Compare January 5, 2025 22:59
@yahkr
Copy link
Contributor Author

yahkr commented Jan 5, 2025

@novhack thanks so much for giving this test drive and finding these issues!

  1. I've found the reason for this and pushed a "fix" see the main takeaways below...

  2. As for the rename issue, unsure how to best fix this at the moment. will be giving it a good think. See point 2 below.

The issue arises when it tries to find the path to an exposed node and can't, the nodes within the path are not renamed before this check is made. so its looking for the old name of the node...

The p_parent->get_metadata(0) does not get updated with the correct path.

I've tracked this down to a scene like this:
image

the parent nodes are not being added to the node_cache within SceneTreeEditor::_update_node_subtree. To me this is one potential issue of not actually having these nodes appear in the scene tree.

image

renaming instance reproduces the errors you're seeing. Doozy of an issue and hard to find the solution since I havn't seen this node_cache stuff too much before now.

Main takeaways:

  1. exposed node order is a bandaid fix and needs to be reviewed once point 3 is decided on.
  2. the node_cache is incorrect and will need fixed somehow
  3. Noticed some odd behavior when trying to place siblings of exposed nodes, should they be a child of the exposed node's true parent, or the instanced scene itself, not allowed, or something else entirely?

@timoschwarzer
Copy link
Contributor

The behavior for point 3 depends:

  • If a node is dragged next to the exposed node in the editor, I would expect it to be added to the instanced scene root, just like with scenes without exposed nodes as well.
  • When calling add_sibling on an exposed node, it should add it to the exposed node's parent.

Regarding the node cache, I don't know it it's related, but note that it is currently bugged sometimes (#100812).

@yahkr
Copy link
Contributor Author

yahkr commented Jan 5, 2025

The behavior for point 3 depends:

* If a node is dragged next to the exposed node in the editor, I would expect it to be added to the instanced scene root, just like with scenes without exposed nodes as well.

* When calling `add_sibling` on an exposed node, it should add it to the exposed node's parent.

Regarding the node cache, I don't know it it's related, but note that it is currently bugged sometimes (#100812).

  1. I agree with you, siblings should be siblings at the instanced scene level like you describe. As that is the most intuitive.
  2. I was worried that Improve Scene Tree editor performance #99700 would affect this PR, hopefully the fix Make sure marked nodes are reset on scene change #101145 is all this PR needs :) my head hurts after tracing through this node_cache/scene state stuff.

@yahkr yahkr force-pushed the exposed_in_owner branch 2 times, most recently from e05df31 to 5418e0f Compare January 6, 2025 09:22
@yahkr
Copy link
Contributor Author

yahkr commented Jan 6, 2025

@timoschwarzer
so this may be a bit more complex of a topic.

for this scene, Where should sib be in the hierarchy?

image

image

or what if we put sib between p1child1 and p1child2? I can't think of a consistent way of handling this beyond limiting the user to placing siblings (via editor) either before or after all exposed nodes or disallowing it alltogether.

@timoschwarzer
Copy link
Contributor

timoschwarzer commented Jan 6, 2025

@yahkr I might miss something, but from intuition I think in the example you sent sib should be placed on after parent1.

If you define these two rules for placing siblings in the editor:

  • siblings always become a child of the root node/scene
  • siblings always go after the previous exposed node (p1child2 in your example)

...then it should end up in a defined position in the tree for all cases, shouldn't it?

@timoschwarzer
Copy link
Contributor

Another solution that I think would be fine here is disallowing placing siblings between exposed nodes, so that exposed nodes will always be shown at the top in the tree below the scene root and you can only place siblings after the exposed nodes.

@btarg
Copy link

btarg commented Jan 15, 2025

One of the first major hurdles in understanding Godot development when I started learning it coming from Unity was that it was confusing to me how every instantiated scene wasn't unique - e.g. if I had a red cube and instantiated it, to change the colour I would need to create a new copy of its material, change that copy's colour, then apply that via GDScript. I have gotten used to this workaround for now, but being able to create unique instances without doing "make local" and forcing future me to go back and update all those instances would be a huge boon for quick prototyping.
This solution looks very interesting, I'll be keeping my eye on it! 👀

@timoschwarzer
Copy link
Contributor

@btarg Resources have a checkbox "Local to Scene" in the inspector, which automatically does this for you. If you check this, this resource will be unique for every instance of the scene.

@btarg
Copy link

btarg commented Jan 16, 2025

@btarg Resources have a checkbox "Local to Scene" in the inspector, which automatically does this for you. If you check this, this resource will be unique for every instance of the scene.

Thank you for this, I'll be sure to check it out!

@yahkr
Copy link
Contributor Author

yahkr commented Jan 17, 2025

current update:

image

The source of the current issues is related to the node_cache. The issue lies in the fact that when instanced, parent does not exist within the scene tree as a TreeItem. The node_cache specifically looks for one to be added
HashMap<Node *, CachedNode>::Iterator add(Node *p_node, TreeItem *p_item);

If I subvert this and add an item without the treeitem like so HashMap<Node *, CachedNode>::Iterator add(Node *p_node); then everything words with renaming etc. The issue then becomes when adding child nodes to the instance.

image
[node name="new child" type="Node3D" parent="instance/parent" index="0"]]

I will be looking into how nodes get added to which parent on move/adding and seeing if I can control that outcome easily. Then setup those rules as @timoschwarzer suggested. Personally I like the idea of exposed nodes always being at the top and any siblings of exposed nodes simply be children of the root instance node.

  • pushed and updated to v4.4.beta.custom_build [f2b8106]

⚠️ EDIT: this change introduced an issue with vanishing parents when a node gets assigned to a non-exposed node, I'll need to dig into this more, but it should resolve itself once I prevent nodes from being added to nodes such as 'parent' in the example above.

@Repiteo Repiteo modified the milestones: 4.4, 4.5 Jan 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet