Revamp autoload creation

[KoBeWi]

Closes #17110
Closes godotengine/godot-proposals#10691

• Text fields are replaced with buttons:
  • Select Existing Script/Scene - select a scene or script that will be added as autoload. This is the previous browse option, except the autoload is added immediately
  • Create Script Autoload - creates a new script and adds as autoload. This already existed btw, the script dialog was opening when you tried to Add autoload without a path
  • Create Scene Autoload - same as above, but creates a scene. The UX is a bit awkward, because you need to go through 2 dialogs, but maybe it's fine

Autoload name is assigned automatically from file name (or root name in case of scene) and there is basic validation to create valid identifier, as otherwise you'll be met with an error dialog. Hopefully it won't be a big problem. If you don't like auto-generated name, you can rename the autoload by double-clicking it in Tree (although the default name is fine in most cases, hence the text inputs were mostly useless).

godot.windows.editor.dev.x86_64_3O8K4gmvOh.mp4

I'm getting some errors about exclusive children™, though they already existed with the script dialog. Not sure how to fix.

[passivestar]

A nitpick: so much text on the buttons is a bit overwhelming, no? "existing" and "autoload" are obvious from context

[YeldhamDev]

Why are the buttons centered?

[KoBeWi]

Why are the buttons centered?

There is an awkward empty space when they are aligned to one side.

[YeldhamDev]

There is an awkward empty space when they are aligned to one side.

Still odd to have them like this, since everywhere else in the editor buttons tend to be aligned to one side when on the top.

[KoBeWi]

Less text and left align:

[tokengamedev]

Just want to check there was this scenario while selecting a script you can change the name of the node, which does not have to be the same name as the script (Ignore case)
I hope you can still edit the name of the Node.

Now couple of issues I found in 4.3:

1. Double clicking on the path of the Autoload or the directory icon in the list -> closes the window.
2. You should not be allowed to create two autoload variables with the same script file. Is there a use case to load the same script twice, then I may be wrong.

Also agree with @passivestar comments, I think Create Script and Create Scene will be fine as you are already under the autoload context

[KoBeWi]

Double clicking on the path of the Autoload or the directory icon in the list -> closes the window.

That's intended. It opens the script in script editor.

You should not be allowed to create two autoload variables with the same script file. Is there a use case to load the same script twice, then I may be wrong.

There is no obvious use-case, but it's not wrong either. No reason to prevent that I think.

Also agree with passivestar comments, I think Create Script and Create Scene will be fine as you are already under the autoload context

[KoBeWi]

Rebased and fixed some errors. Autoload dialog now uses its own SceneCreateDialog and no longer open the scene when created.

[tokengamedev]

Thanks @KoBeWi for the update.

[Zshandi]

I would love to see this in the next release of Godot! Let me know if there's anything I could do to help make that a reality 🙂

I especially like the change to have it use its own dialog for both the scene and script, rather than re-using the filesystem dock one as it was previously doing! It still behaves weirdly in that you can click out of the pop-up and it goes behind the project settings, though I think that's a much larger problem than just this (which I'm guessing already has it's own Issue somewhere, unless it's already fixed on latest master?).

[KoBeWi]

Rebased.
I also deleted the Open button on the right, because you can double-click the path to do the same.

[Zshandi]

Rebased. I also deleted the Open button on the right, because you can double-click the path to do the same.

Nice, hope it gets in soon then!

I'm personally partial to having more than one way to do things in UIs (more likely users will discover it), however in this case I feel it was also counter-intuitive that a folder icon opened the file itself (rather than just revealing it in the file system dock). Great work overall!

[Calinou]

This fails to compile when rebased on top of master on my end:

In file included from ./editor/settings/action_map_editor.cpp:33,
                 from editor/settings/.scu/scu_editor_settings.gen.cpp:1:
./editor/settings/editor_autoload_settings.cpp: In constructor 'EditorAutoloadSettings::EditorAutoloadSettings()':
./core/object/callable_mp.h:216:66: error: no matching function for call to 'create_custom_callable_function_pointer(EditorFileDialog*&, const char [37], void (FileDialog::*)())'
  216 | #define callable_mp(I, M) create_custom_callable_function_pointer(I, #M, M)
      |                           ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~
./editor/settings/editor_autoload_settings.cpp:889:58: note: in expansion of macro 'callable_mp'
  889 |         browse_button->connect(SceneStringName(pressed), callable_mp(autoload_file_dialog, &EditorFileDialog::popup_file_dialog));
      |                                                          ^~~~~~~~~~~
./core/object/callable_mp.h:216:66: note: there are 4 candidates
  216 | #define callable_mp(I, M) create_custom_callable_function_pointer(I, #M, M)
      |                           ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~
./editor/settings/editor_autoload_settings.cpp:889:58: note: in expansion of macro 'callable_mp'
  889 |         browse_button->connect(SceneStringName(pressed), callable_mp(autoload_file_dialog, &EditorFileDialog::popup_file_dialog));
      |                                                          ^~~~~~~~~~~
./core/object/callable_mp.h:119:10: note: candidate 1: 'template<class T, class ... P> Callable create_custom_callable_function_pointer(T*, const char*, void (T::*)(P ...))'
  119 | Callable create_custom_callable_function_pointer(T *p_instance,
      |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
./core/object/callable_mp.h:119:10: note: template argument deduction/substitution failed:
./core/object/callable_mp.h:216:66: note:   deduced conflicting types for parameter 'T' ('EditorFileDialog' and 'FileDialog')
  216 | #define callable_mp(I, M) create_custom_callable_function_pointer(I, #M, M)
      |                           ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~
./editor/settings/editor_autoload_settings.cpp:889:58: note: in expansion of macro 'callable_mp'
  889 |         browse_button->connect(SceneStringName(pressed), callable_mp(autoload_file_dialog, &EditorFileDialog::popup_file_dialog));
      |                                                          ^~~~~~~~~~~
./core/object/callable_mp.h:133:10: note: candidate 2: 'template<class T, class R, class ... P> Callable create_custom_callable_function_pointer(T*, const char*, R (T::*)(P ...))'
  133 | Callable create_custom_callable_function_pointer(T *p_instance,
      |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
./core/object/callable_mp.h:133:10: note: template argument deduction/substitution failed:
./core/object/callable_mp.h:216:66: note:   deduced conflicting types for parameter 'T' ('EditorFileDialog' and 'FileDialog')
  216 | #define callable_mp(I, M) create_custom_callable_function_pointer(I, #M, M)
      |                           ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~
./editor/settings/editor_autoload_settings.cpp:889:58: note: in expansion of macro 'callable_mp'
  889 |         browse_button->connect(SceneStringName(pressed), callable_mp(autoload_file_dialog, &EditorFileDialog::popup_file_dialog));
      |                                                          ^~~~~~~~~~~
./core/object/callable_mp.h:188:10: note: candidate 3: 'template<class T, class ... P> Callable create_custom_callable_function_pointer(T*, const char*, void (T::*)(P ...) const)'
  188 | Callable create_custom_callable_function_pointer(T *p_instance,
      |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
./core/object/callable_mp.h:188:10: note: template argument deduction/substitution failed:
./core/object/callable_mp.h:216:66: note:   types 'void (T::)(P ...) const' and 'void (FileDialog::)()' have incompatible cv-qualifiers
  216 | #define callable_mp(I, M) create_custom_callable_function_pointer(I, #M, M)
      |                           ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~
./editor/settings/editor_autoload_settings.cpp:889:58: note: in expansion of macro 'callable_mp'
  889 |         browse_button->connect(SceneStringName(pressed), callable_mp(autoload_file_dialog, &EditorFileDialog::popup_file_dialog));
      |                                                          ^~~~~~~~~~~
./core/object/callable_mp.h:202:10: note: candidate 4: 'template<class T, class R, class ... P> Callable create_custom_callable_function_pointer(T*, const char*, R (T::*)(P ...) const)'
  202 | Callable create_custom_callable_function_pointer(T *p_instance,
      |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
./core/object/callable_mp.h:202:10: note: template argument deduction/substitution failed:
./core/object/callable_mp.h:216:66: note:   types 'R (T::)(P ...) const' and 'void (FileDialog::)()' have incompatible cv-qualifiers
  216 | #define callable_mp(I, M) create_custom_callable_function_pointer(I, #M, M)
      |                           ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~
./editor/settings/editor_autoload_settings.cpp:889:58: note: in expansion of macro 'callable_mp'
  889 |         browse_button->connect(SceneStringName(pressed), callable_mp(autoload_file_dialog, &EditorFileDialog::popup_file_dialog));
      |                                                          ^~~~~~~~~~~
scons: *** [bin/obj/editor/settings/.scu/scu_editor_settings.gen.linuxbsd.editor.x86_64.o] Error 1
scons: building terminated because of errors.

[KoBeWi]

Try now.

[passivestar]

Tested, works as expected and the UX is a clear improvement over the previous one

[AdriaandeJongh]

Also tested this and found this little icon on hover (it doesn't do anything):

And while we're at it: would be nice to give the headers a tooltip. Especially the Global Variable could use something along the lines of "If enabled, this autoload can be accessed by scripts through $Name where 'Name' is replaced with the name of the autoload".

[passivestar]

Also tested this and found this little icon on hover (it doesn't do anything)

It's not an icon but the main tree cell that the buttons are attached to. I looked briefly into getting rid of it while working on the UI for 4.6 but afair there's no easy way to get rid of it. Ideally we should allow adding any control nodes to tree cells (or allow custom drawing/input logic in cells) because this system with icon buttons on the right is very limiting. Either way this issue is unrelated to this PR

[KoBeWi]

Added tooltips.

[AdriaandeJongh]

If the bug I mentioned above isn't in the scope of this PR, then the rest looks good to me.

[dagarsar]

It's not an icon but the main tree cell that the buttons are attached to. I looked briefly into getting rid of it while working on the UI for 4.6 but afair there's no easy way to get rid of it. Ideally we should allow adding any control nodes to tree cells (or allow custom drawing/input logic in cells) because this system with icon buttons on the right is very limiting. Either way this issue is unrelated to this PR

You can already set a tree cell to CELL_MODE_CUSTOM and draw whatever you want, and get the clicks through the custom_item_clicked signal. I think it would not be aware of hover and pressed states though, and maybe that's enough to not change it and leave it as it is.

[KoBeWi]

CELL_MODE_CUSTOM still draws regular TreeItem elements (text, hover), the custom drawing only replaces the background.

[dagarsar]

CELL_MODE_CUSTOM still draws regular TreeItem elements (text, hover), the custom drawing only replaces the background.

Oh right, then I'd say drawing the background using the tree's button stylebox and make this cell with 3 buttons 3 cells with that behaviour would fall under this PR's scope

[KoBeWi]

This is a pre-existing problem and does not affect creating autoloads, so IMO it is out of scope. Feel free to open an issue, so it's tracked properly.

[akien-mga]

Thanks!