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 and PropertySelector via EditorInterface #81655

Merged

Conversation

nlupugla
Copy link
Contributor

@nlupugla nlupugla commented Sep 14, 2023

Allows users to create SceneTreeDialog and PropertySelector dialogs via the EditorInterface. Lays the groundwork for adding similar dialogs in the future as needed.

The API is

EditorInterface.popup_dialog_scene_tree(callback: Callable, valid_types: Array[StringName] = [])
EditorInterface.popup_dialog_property_selector(object: Object, callback: Callable, type_filter: PackedIntArray = [])

Example usage

@tool
extends Node
class_name Main


func _ready() -> void:
	if Engine.is_editor_hint():
		EditorInterface.popup_dialog_scene_tree(_on_node_selected, [&"Button"])

func _on_node_selected(node_path : NodePath) -> void:
	if node_path == ^"":
		print("scene tree dialog canceled")
	else:
		EditorInterface.popup_dialog_property_selector(get_node(node_path), func(x) : print(x), [TYPE_BOOL])

Closes godotengine/godot-proposals#7635.
Closes godotengine/godot-proposals#7636.

Supersedes #81488.
Supersedes #81489.

Related: godotengine/godot-proposals#300.

Edit 2023-Oct-05 12:45:
I have only implemented PropertySelector::select_property_from_instance, but there are several related methods: select_property_from_base_type, select_property_from_script, and select_property_from_basic_type. Should I implement these now or wait until the need arises?

Edit 2023-Dec-09 19:06:
Edited example usage to reflect new API that uses a single NodePath instead of an array of NodePaths.

editor/editor_interface.h Outdated Show resolved Hide resolved
editor/editor_interface.h Outdated Show resolved Hide resolved
doc/classes/EditorInterface.xml Outdated Show resolved Hide resolved
editor/editor_interface.cpp Outdated Show resolved Hide resolved
@AThousandShips AThousandShips modified the milestones: 4.2, 4.3 Oct 17, 2023
editor/editor_interface.cpp Outdated Show resolved Hide resolved
@KoBeWi

This comment was marked as resolved.

editor/editor_interface.cpp Outdated Show resolved Hide resolved
editor/editor_interface.cpp Outdated Show resolved Hide resolved
editor/editor_interface.cpp Outdated Show resolved Hide resolved
editor/editor_interface.h Outdated Show resolved Hide resolved
@nlupugla
Copy link
Contributor Author

Why the callbacks use arrays of NodePaths as arguments if the dialog itself returns NodePath?

Thanks for the review @KoBeWi, I'm learning a lot from your comments!

I grateful you asked about using an Array because it's a decision I want some feedback on. I have two reasons for using an Array.

  1. It reduces the number of signals that need to be added to EditorInterfaceAPI. An empty Array can be interpreted as a "cancelled" event, which means there doesn't need to be seperate cancelled signals for each dialog, e.g. dialog_scene_tree_cancelled.
  2. It will make it easier to support multi-select in the future. I hope to add multi-select select to these dialogs soon: Add support for multiple selection in EditorPropertySelector and SceneTreeDialog godot-proposals#7676.

@KoBeWi
Copy link
Member

KoBeWi commented Dec 20, 2023

Left some code suggestions, but the functionality itself looks fine now.

@nlupugla nlupugla force-pushed the editor-interface-can-popup-dialogs branch from fb292e9 to fd8ed50 Compare December 22, 2023 00:37
editor/editor_interface.h Outdated Show resolved Hide resolved
@nlupugla nlupugla force-pushed the editor-interface-can-popup-dialogs branch from fd8ed50 to 204f14c Compare December 22, 2023 15:09
@nlupugla
Copy link
Contributor Author

Awesome, thanks for all the help KoBeWi :)

Just pushed the minor change you suggested of Vector -> PackedInt32Array.

@YuriSizov
Copy link
Contributor

Overall, this looks good to me and seems useful. Once the comments are addressed, this can be merged. Thank you for waiting :)

@nlupugla
Copy link
Contributor Author

Awesome, thanks Yuri <3

Hopefully I'll have time to commit these changes and test them soon :)

@nlupugla nlupugla force-pushed the editor-interface-can-popup-dialogs branch from 204f14c to 62ecc5a Compare January 17, 2024 18:09
@nlupugla
Copy link
Contributor Author

Made the requested changes :)

I reran it with my test project and it still works fine. I haven't done any testing of the edge cases that removing signals and removing from tree before deleting the dialog nodes would address: presumably when the user attempts to popup the same dialog twice in one frame. Should I submit a unit test for that? A test project? Or is that all overboard for this PR?

@nlupugla nlupugla force-pushed the editor-interface-can-popup-dialogs branch from 62ecc5a to bd9eb62 Compare January 17, 2024 18:18
@YuriSizov
Copy link
Contributor

You need to rebase on the current master, we had an issue with macOS builds which will fail your CI.

@nlupugla nlupugla force-pushed the editor-interface-can-popup-dialogs branch 2 times, most recently from d6d5a63 to 12acd08 Compare January 17, 2024 18:37
@YuriSizov YuriSizov self-requested a review January 19, 2024 10:11
doc/classes/EditorInterface.xml Outdated Show resolved Hide resolved
doc/classes/EditorInterface.xml Outdated Show resolved Hide resolved
@nlupugla nlupugla force-pushed the editor-interface-can-popup-dialogs branch from 12acd08 to 558c276 Compare January 19, 2024 19:17
@akien-mga akien-mga merged commit 163c00e into godotengine:master Feb 8, 2024
16 checks passed
@akien-mga
Copy link
Member

akien-mga commented Feb 8, 2024

Thanks! And congrats for your first merged Godot contribution 🎉

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

Successfully merging this pull request may close these issues.

Expose SceneTreeDialog to scripting Expose PropertySelector to scripting
5 participants