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

Expose SceneTreeDialog to scripting #7636

Closed
nlupugla opened this issue Sep 9, 2023 · 12 comments · Fixed by godotengine/godot#81655
Closed

Expose SceneTreeDialog to scripting #7636

nlupugla opened this issue Sep 9, 2023 · 12 comments · Fixed by godotengine/godot#81655

Comments

@nlupugla
Copy link

nlupugla commented Sep 9, 2023

Describe the project you are working on

An addon built with GDExtension that helps users create saving and loading systems for their games.

Describe the problem or limitation you are having in your project

I want to use the engine's SceneTreeDialog to provide a UI where users can select nodes in a scene they want to save/load.

I can imagine this dialog would be useful in other plugins/addons as well (although admittedly I can't think of a concrete example other than my own project mentioned above).

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

Exposing SceneTreeDialog to scripting will allow users to select which nodes they want to save/load using a familiar UI; the same one used in AnimationPlayer and MultiplayerSynchronizer.

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

Simply expose the existing SceneTreeDialog https://github.com/godotengine/godot/blob/fc99492d3066098e938449b10e02f8e01d07e2d1/editor/gui/scene_tree_editor.cpp#L1515C4-L1515C4 to scripting. See proposed implementation in draft PR godotengine/godot#81489.

Edit: I received feedback that exposing SceneTreeDialog directly is not a good idea because it couples the implementation with the API. Instead, it was recommended that I incorporate some of SceneTreeDialog's functionality into EditorInterface. Here are my proposed additions to EditorInterface.

signal nodes_selected(nodes : Array[Node])

func popup_dialog_scene_tree(valid_types : Array[StringName]) -> void

A few notes on the API

  1. Emitting an Array[Node] instead of just a Node let's you infer that the dialog was canceled (by checking whether the array is empty) and also allows multiple nodes to be selected in principle.
  2. The signal could emit an Array[NodePath] instead of an Array[Node] if it makes more sense.
  3. I chose popup_dialog_scene_tree as opposed to popup_scenetree_dialog (which is a function in the C++ source code) so that any future editor dialog functions added to EditorInterface in this way (eg: popup_dialog_property_selector) will appear near each other when sorted alphabetically.
  4. The proposed API doesn't give the user much control over the dialog (eg: it's size and position). If there is a need for that extra level of control, we can always add optional parameters to popup_dialog_scene_tree.

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

It's more than a few lines of script to recreate the SceneTreeDialog.

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

N/A

@raulsntos
Copy link
Member

@AThousandShips
Copy link
Member

AThousandShips commented Sep 10, 2023

I can imagine this dialog would be useful in other plugins/addons as well (although admittedly I can't think of a concrete example other than my own project mentioned above).

This is kind of a crux here, exposing internal components is a major tying up of the functionality and interface of things, which has to have a very good reason

I really think editor functionality should be exposed through the editor interface, and not by directly exposing the classes themselves

@poohcom1
Copy link

For other use-cases for this and #7635, my add-on animation-player-refactor needed to recreate both the SceneTreeDialog and PropertySelector to have a good UI for selecting nodes and properties for refactoring:

SceneTreeDialog PropertySelector
image image

There are also addons like the popular aseprite-wizard or my own animated-sprite-2-player which needs a way to select an AnimationPlayer from the scene. This is currently done using a dropdown of NodePath, but having a dedicated node selector would make the UI much better.

image

I think for editor tools that deals with editing the scene, having these options would make development much more convenient.

@nlupugla
Copy link
Author

I really think editor functionality should be exposed through the editor interface, and not by directly exposing the classes themselves

What do you mean by the editor I interface? I doesn't super matter to me how dialogues like SceneTreeDialog and PropertySelector get exposed. If there is a different way to provide their functionality to developers, I'd be happy to look into it :)

@AThousandShips
Copy link
Member

Literally EditorInterface :)

@nlupugla
Copy link
Author

Thanks for the support @poohcom1.

Out of curiosity, how did you manage to implement PropertySelector without a built-in function for listing the properties of non-object Variants?

@nlupugla
Copy link
Author

Literally EditorInterface :)

Ah yes, that makes sense :)

I'm still a little unclear on your proposal: do you want to add to EditorInterface a method like get_property_selector ? Wouldn't PropertySelector still need to be exposed?

@AThousandShips
Copy link
Member

AThousandShips commented Sep 10, 2023

No that ties in with the required behaviour of SceneTreeDialog, I don't see how actually exposing it is required here

@poohcom1
Copy link

Out of curiosity, how did you manage to implement PropertySelector without a built-in function for listing the properties of non-object Variants?

@nlupugla I had a more specific use-case so it's probably not what you're looking for. My property selector was only for AnimationPlayer tracks, so I just had to check the tracks for the properties I wanted to display and verify via has_method or get_indexed. You can see the code here.

I'm not 100% sure if I understand your requirements for listing only non-object Variants, but I don't see why you couldn't use get_property_list which seems to be what is use internally by PropertySelector (unless I'm missing something).

I really think editor functionality should be exposed through the editor interface, and not by directly exposing the classes themselves

I agree that a feature like this is probably best implemented via the editor interface, although I'm not sure what it would look like since the whole process is asynchronous as we need to somehow retrieve the selected node later.

We could do something like get_file_system_dock(), where we have get_scene_tree_dialog() that just spawns the dialog and return an instance which can only be accessed via this method, or maybe something more abstract like open_scene_tree_dialog() that returns the selected node asynchronously either through a callback or some kind of signal.

@AThousandShips
Copy link
Member

AThousandShips commented Sep 10, 2023

Let's keep each topic to each proposal shall we?

The dialog uses signals already so it is asynchronous already in its use

Edit: An alternative method would be to create a method like so:
pick_node(Callable p_callback, Parameters...)
Where the callback is called when you've picked something, and you can configure things directly from there

@nlupugla
Copy link
Author

Let's keep each topic to each proposal shall we?

Fair point :)

Thanks for the extra info poohcom1.

@nlupugla
Copy link
Author

@AThousandShips @raulsntos @YuriSizov I updated my OP with a proposed API addition for EditorInterface. Let me know if you have any thoughts :)

Also is there any preference around whether I implement this in the currently link PR versus creating a new PR that reflects the different implementation.?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants