search

godotengine / godot-proposals

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

Allow user to expose nodes within an instantiated scene #8266

Open yahkr opened 1 year ago

yahkr commented 1 year ago

Describe the project you are working on

A re-usable ui framework that would allow a user to quickly and efficiently create new ui that follows the same formats and styles of the project. The only way to use the current system is to enable "Editable Children" on an instantiated scene.

Describe the problem or limitation you are having in your project

"Editable Children" clutters the scene tree dock and while functional, when modifying the underlying scene (moving/renaming), the child placed on the instantiated version becomes 'corrupt' losing its parent. This prevents the user from creating one scene and reusing it many times while maintaining the abilty to modify the underlying scene. In my case specifically UI related, but can be applied to all areas.

Example of using "Editable Children" pr1

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

By allowing the user to expose nodes inside of an instantiated scene you can add children to specific sections of the subscene. This allows the user to re-use scenes as a type of custom container node.

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

I propose nodes get an additional bit of data added to them "exposed_in_owner" and related functions

--- node.h ---
bool exposed_in_owner = false;
void set_exposed_in_owner(bool p_enabled);
bool is_exposed_in_owner() const;

My current thought is having exposed nodes be dependent on also having a unique name within the scene so it's more straightforward when saving a scene to figure out where they should go. Current implementation forces 'unique_name_in_owner' on, but wont force it off if you disable the exposure setting.

Would love to get some feedback/assistance with this. @dmchurch if you see this, would love to work together on a solution :)

Things left to do ( for my implementation ):

(For reference lets assume Scene A instantiates a copy of Scene B, in which Scene B has an exposed node E):

Proof of concept parts done:

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

Visual example of what I have so far:

pr2

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

Would be used throughout a project, but if forced to workaround it could possible be done, not in a few lines of code, but an example of a workaround could be similar to @raulsntos 's suggestion here: @8184. However this approach has a lot of manual work for the user.

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

I don't currently see a way around this without being able to modify the scene_tree_dock/pack_scene/node code. Giving the user the ability to reuse code like this would increase readability of the Scene hierarchy and remove unnecessary duplication across the project.

yahkr commented 1 year ago

see also:

dmchurch commented 1 year ago

You're more than welcome to use any of the code from my fork, though I'm not keeping it in topic branches anymore. I'll try to remember to push if I do anything interesting. In particular, you may want to adopt the packed_scene unit test I wrote and adjust it for your needs, since the existing Godot codebase doesn't have any unit tests for the "instantiate a scene with a packed subscene" code path and its behavior is pretty critical for, y'know, running games.

Feel free to tag me and ask if you have any problems you can't figure out a solution to, though I'm not planning to actively follow the discussion here.

ETA: There's also a feature I'm intending to add which is the equivalent of the right-click "create scene from this branch" but more powerful, letting you create a scene from just the intermediary nodes of a branch, collapsing a scene fragment into a subscene with default_child_parent without adjusting the current scene's hierarchy in any way. It could be altered to let you create a subscene with exposed children - I'd always expected that an "exposed in owner" feature was the natural follow-on to "default child parent", though I think I called the feature "forced-visible children" in my comments.

yahkr commented 1 year ago

Appreciate it @dmchurch . I'll definitely borrow the unit test . That extra feature of collapsing a branch does sound handy. Will have to explore some more

My current goal is to better understand the scene tree dock structure and how nodes get saved etc.

dmchurch commented 1 year ago

My current goal is to better understand the scene tree dock structure and how nodes get saved etc.

Take a look at the list of files and methods I've changed - you won't necessarily need to make the same changes I did, but they should provide you a good jumping-off point for exploring the code paths.

rob1gat commented 1 year ago

Hi, you might be interested by #3248 discussion.

yahkr commented 11 months ago

Pushed an update that is somewhat functional, results showing promise. Screenshot 2023-11-24 153636 is saved to:

[gd_scene load_steps=2 format=3 uid="uid://bikicvcke5g1o"]

[ext_resource type="PackedScene" uid="uid://bgbfogy57nk63" path="res://control.tscn" id="1_l1gfn"]

[node name="Root" type="Control"]
layout_mode = 3
anchors_preset = 15
anchor_right = 1.0
anchor_bottom = 1.0
grow_horizontal = 2
grow_vertical = 2

[node name="HBoxContainer" type="HBoxContainer" parent="."]
layout_mode = 0
offset_right = 262.0
offset_bottom = 291.0

[node name="InstantiatedScene" parent="HBoxContainer" instance=ExtResource("1_l1gfn")]
layout_mode = 2

[node name="Button" type="Button" parent="HBoxContainer/InstantiatedScene/%Exposed" index="0"]
layout_mode = 2
text = "test1"

Exposed node inside control.tscn looks like this.

[node name="Exposed" type="VBoxContainer" parent="Panel/PanelContainer/VBoxContainer"]
unique_name_in_owner = true
exposed_in_owner = true
layout_mode = 2
theme_override_constants/separation = 10
jcostello commented 11 months ago

It will be awesome to have gltf scene that you can expose meshes in the import window. Sometime I have models that I can drag and drop materials and I have to click on editable children for all of them before doing so

yahkr commented 11 months ago

It will be awesome to have gltf scene that you can expose meshes in the import window. Sometime I have models that I can drag and drop materials and I have to click on editable children for all of them before doing so

I did look into this feature this past day or so, and while i can get exposure working on import, since the exposed node isn't saved in the instantiated scene the material override isn't either. So will have to re-think that aspect.

bits-by-brandon commented 11 months ago

I'm making some semi-advanced UI for my game, where I want to animate in arbitrary children at certain locations. It would make my workflow and project structure way cleaner to be able to build custom Control components that allow predetermined slots. If this was implemented, I think this would become my preferred way to compose control components.

dugramen commented 8 months ago

This would be super useful for reducing the amount of boilerplate code you need for editing "composed" nodes. So I think built in nodes should also use this feature to expose their hidden nodes. For example:

There's likely a bunch more. But in all of these cases, it's a pain to edit properties, connect signals, and add theme overrides because the associated nodes are hidden. Some of these nodes have extra (and redundant) properties to expose some of the hidden attributes, but they're incomplete, and it would just be better to directly access them with this.

yahkr commented 4 months ago

Hi all, just to continue the conversation and provide a small update. I've been tinkering with overrides/exposed nodes and have added some features I would love to get feedback on. Work below inspired by #3248 discussion, @jcostello's comment, and @dugramen comment

Overrides: Provides a way to make changes to nodes that are exposed: This is what my exposed_spinner.tscn looks like: The root spinner has a script that simply spins the "$Exposed" node. Exposed is a MeshInstance3D that is marked as exposed, and the "Not Exposed" node is just a decal projector that is a child of the exposed node.

exposed_spinnger

When you instantiate this scene you can apply override materials or other modifications and they will be saved into the tscn file like so: main.tscn main

main.tscn main tscn

This allows you to have scenes be really flexible. Heres a quick demo of what you can do with this system.

overrides 1.webm

I also worked on .blend imports and can now use "-exp" to define if a node should be exposed or not. The overriding feature also works with this. This is in response to @jcostello's comment about gltf importing (currently only works with .blend but i will make it work with gltf as well), below is a demo of how this system handles .blend file changes and applies the overrides.

overrides 2.webm

Keep in mind all of this is without the pitfalls of 'editable children' being enabled. So when things move around in the instantiated scene, so long as the names are kept the same any children/overrides will remain in-tact. I'll definitely be exploring how to propagate renames to any other scene that uses the tscn, but we shall see. Current implementation is kinda hacky I feel so I havn't committed it to my repo yet. I would love to get some feedback on this overriding feature. Thanks!

yahkr commented 4 months ago

@dugramen

This would be super useful for reducing the amount of boilerplate code you need for editing "composed" nodes. So I think built in nodes should also use this feature to expose their hidden nodes. For example:

  • MenuButton & LineEdit have a hidden PopupMenu
  • ScrollContainer & TextEdit have a hidden HScrollBar and VScrollBar
  • SpinBox has a hidden LineEdit
  • ColorPickerButton has a hidden ColorPicker and Popup

There's likely a bunch more. But in all of these cases, it's a pain to edit properties, connect signals, and add theme overrides because the associated nodes are hidden. Some of these nodes have extra (and redundant) properties to expose some of the hidden attributes, but they're incomplete, and it would just be better to directly access them with this.

I was curious about this, so i looked into it. To make this work with the exposed nodes, spin_box.cpp must be modified like so:

405 add_child(line_edit, true, INTERNAL_MODE_FRONT);
406 line_edit->set_exposed_in_owner(true);

Whats really important here is the second param, when false it generates a random id tacked onto the end of the name, this makes it unusable for the exposed node system. so changed it to true, where its forced to be named LineEdit. (This would cause issues if you wanted your own LineEdit child of a SpinBox for some reason...

This does produce the desired results, however it also produces quite a lot of junk in the tscn file when saving, regardless of if you made overrides the engine does after instantiation, so theyre included in the tscn.

image

Added a right_icon and overwrote the font_size for theme:

[override path="./SpinBox/LineEdit"]
unique_name_in_owner = true
exposed_in_owner = true
layout_mode = 1
anchors_preset = 15
anchor_right = 1.0
anchor_bottom = 1.0
offset_right = -16.0
mouse_filter = 1
theme_override_font_sizes/font_size = 60
text = "0"
right_icon = ExtResource("4_ce87b")
caret_column = 1

Unedited:

[override path="./SpinBox2/LineEdit"]
unique_name_in_owner = true
exposed_in_owner = true
layout_mode = 0
anchor_right = 1.0
anchor_bottom = 1.0
offset_right = -16.0
mouse_filter = 1
text = "0"
caret_column = 1

image

All in all i think it's cool that these could be exposed, but im not sure they would benefit from allowing the user to override any of their properties with how they are currently setup in engine.

dugramen commented 4 months ago

Whats really important here is the second param, when false it generates a random id tacked onto the end of the name, this makes it unusable for the exposed node system. so changed it to true, where its forced to be named LineEdit. (This would cause issues if you wanted your own LineEdit child of a SpinBox for some reason...

Maybe it could be named more explicitly or in a special way, to avoid conflicts. Like "LineEdit@Internal" or just "_LineEdit_"

All in all i think it's cool that these could be exposed, but im not sure they would benefit from allowing the user to override any of their properties with how they are currently setup in engine.

Do you mean that users wouldn't be able to override, or just that there may be too many drawbacks?

(Also just wanna say this looks awesome already!)

yahkr commented 4 months ago

Maybe it could be named more explicitly or in a special way, to avoid conflicts. Like "LineEdit@Internal" or just "LineEdit"

For the naming I agree it could certainly be made to be more predictable when they auto generate internal node names, that way when saving overrides we can trust the name of the internal node matches that of what gets generated at runtime. I'm sure there's a lengthy discussion somewhere that decided the generated uid was best but who knows.

Do you mean that users wouldn't be able to override, or just that there may be too many drawbacks?

Users could still override any property they wanted. Like I do in that example I override the properties:

theme_override_font_sizes/font_size = 60
right_icon = ExtResource("4_ce87b")

But with how these types of nodes are currently implemented in the engine (spinbox etc) they create their own internal children and setup the properties it needs by default, so for spinbox its the following:

layout_mode = 0
anchor_right = 1.0
anchor_bottom = 1.0
offset_right = -16.0
mouse_filter = 1
text = "0"
caret_column = 1

Since how my override system works is looking for modified properties it picks these up as overrides when they really aren't (from a functional standpoint). This isn't necessarily a bad thing, it just clutters the tscn file when you save. Appreciate the kind words :)

yahkr commented 4 months ago

relevant and would solve the issues with built-in node references and renaming of nodes within subscenes: https://github.com/godotengine/godot/pull/86960

jack27121 commented 3 days ago

im working on comprehensive GUI stuff rn, and have a lot of stuff where this would be useful. Would love this to get added

Dfred commented 2 days ago

I also had to create tool scripts that duplicate child nodes to replace specific ReferenceRect in packed scenes. This adds a significant amount of editor/runtime branching that this feature would replace.