godot icon indicating copy to clipboard operation
godot copied to clipboard

Allow Quick Open dialog to preview change in scene

Open Meorge opened this issue 6 months ago • 8 comments

Implements and closes https://github.com/godotengine/godot-proposals/issues/7809.

With this PR, resource references are updated as soon as a new resource is highlighted in the Quick Open dialog. This allows the user to very quickly scroll through resources and preview how they look in the scene.

Video (with music for dramatic effect, of course):

https://github.com/user-attachments/assets/b011895b-bcc9-4795-9a15-6d48b97ee47b

Meorge avatar May 30 '25 00:05 Meorge

I've added a toggle to the window that allows the user to switch between the old and new behavior for the dialog box.

Meorge avatar Jun 01 '25 17:06 Meorge

Thanks for testing! I'll look into fixing the navigation and preview updating.

As for the default setting, I don't remember providing an initial setting for it - will get that done as well! I think it might be best to have it off by default so it matches the existing behavior. But that may also risk it going unnoticed and unused by people.


Notes for myself: When clicking an item in Quick Open's grid view, the item becomes selected. However, to allow the arrow keys to change the selection, I first have to tap the right arrow key as many times as the current selected item is items from the right. For example, if there are 3 columns, then I have to tap the right key 3 times. Then the search bar highlights, and after that, if I press the right key again, the selection starts moving again.

Meorge avatar Jun 03 '25 02:06 Meorge

You can now navigate with the keyboard after clicking on an item, and the Instant Preview setting now officially defaults to false. (Again, totally open to make it default to enabled if others would prefer it.)

I think this would be a better fit for a later PR, but it appears the way that the focused Quick Open item is selected is... nonstandard. It seems to be some sort of hack around the search bar's inputs, rather than using Godot's built-in UI focus system. When the keyboard controls didn't work for selecting items after clicking one, that was because the item itself had become selected according to Godot - but in order for items to appear selected in Quick Open, the search bar has to actually be selected so it can pass the inputs through to display one of the items as selected... again, rather strange, and probably worth looking into replacing with Godot's normal focus system.


Hm, so I thought I reproduced the issue you were experiencing with my test project, and fixed that. However, just to confirm I just downloaded your test project and I can see the material isn't updating when Instant Preview is enabled. Will have to look more at this tomorrow 😅


3 June 2025, 10:34 PM:

So, the good news is, I think I've found the underlying issue causing this bug. The bad news is, it seems to have to do with how the editor handles property/resource pickers, which is definitely gnarlier than just the Quick Load menu.

Basically, it appears that whenever a new texture is selected for the Material, the entire Material EditorResourcePicker is removed from the tree, and a new(?) one is added back in its place. Because the old one provided the _file_selected callback, when this removal and replacement is done the Callable that the Quick Load menu has becomes invalid, and thus selecting a new texture is selected nothing is passed to the Material to actually update it (i.e., _file_selected is never called).

I'm not totally sure yet how to address this, but I'll see what I can think of. If anyone has suggestions (or recognizes any inaccuracies in my current diagnosis), they would be very helpful! 😄

Meorge avatar Jun 03 '25 05:06 Meorge

After a short break, I think I've made some headway on this, but it still seems it's going to be much more involved than I'd previously hoped.

Currently the Quick Load window is passed the object and property path it is setting. When the user selects a new resource, it simply calls Object::set() with the selected resource, and the editor updates with it instantly.

https://github.com/user-attachments/assets/43dcbc40-b9e3-4337-984b-ae9d918b1f08

However, undo/redo is currently not supported, and the editor does not recognize that modifications have been made to the scene/project that trigger the "unsaved" status for the window.

Meorge avatar Jun 08 '25 20:06 Meorge

I think you could do similar to ColorPicker - change things instantly, but allow canceling them. If you press Enter, the resource gets accepted and makes undo/redo action. If you press Escape, the resource goes back to the initial value, so no undo action is needed.

KoBeWi avatar Jun 11 '25 09:06 KoBeWi

Done! I think I'd avoided doing that before because I misunderstood how the undo/redo actions worked, and thought it would be much harder to do than it actually was (assuming it worked, of course 😅 )

Meorge avatar Jun 12 '25 04:06 Meorge

Crash in theme editor:

CrashHandlerException: Program crashed
Engine version: Godot Engine v4.5.dev.custom_build (d9cd011e2fa9fa9a3011371843729f33032cee35)
Dumping the backtrace. Please include this when reporting the bug on: https://github.com/godotengine/godot/issues
[0] Object::get (C:\godot_source\core\object\object.cpp:367)
[1] Object::get (C:\godot_source\core\object\object.cpp:367)
[2] EditorQuickOpenDialog::popup_dialog_for_property (C:\godot_source\editor\gui\editor_quick_open_dialog.cpp:174)
[3] EditorResourcePicker::_edit_menu_cbk (C:\godot_source\editor\editor_resource_picker.cpp:369)
[4] call_with_variant_args_helper<EditorResourcePicker,int,0> (C:\godot_source\core\variant\binder_common.h:228)
[5] call_with_variant_args<EditorResourcePicker,int> (C:\godot_source\core\variant\binder_common.h:338)
[6] CallableCustomMethodPointer<EditorResourcePicker,void,int>::call (C:\godot_source\core\object\callable_method_pointer.h:107)
[7] Callable::callp (C:\godot_source\core\variant\callable.cpp:58)
[8] Object::emit_signalp (C:\godot_source\core\object\object.cpp:1288)
[9] Node::emit_signalp (C:\godot_source\scene\main\node.cpp:4130)
[10] Object::emit_signal<int> (C:\godot_source\core\object\object.h:940)
[11] PopupMenu::activate_item (C:\godot_source\scene\gui\popup_menu.cpp:2744)
[12] PopupMenu::_input_from_window_internal (C:\godot_source\scene\gui\popup_menu.cpp:686)
[13] PopupMenu::_input_from_window (C:\godot_source\scene\gui\popup_menu.cpp:468)
[14] Window::_window_input (C:\godot_source\scene\main\window.cpp:1843)
[15] call_with_variant_args_helper<Window,Ref<InputEvent> const &,0> (C:\godot_source\core\variant\binder_common.h:223)
[16] call_with_variant_args<Window,Ref<InputEvent> const &> (C:\godot_source\core\variant\binder_common.h:338)
[17] CallableCustomMethodPointer<Window,void,Ref<InputEvent> const &>::call (C:\godot_source\core\object\callable_method_pointer.h:107)
[18] Callable::callp (C:\godot_source\core\variant\callable.cpp:58)
[19] Callable::call<Ref<InputEvent> > (C:\godot_source\core\variant\variant.h:953)
[20] DisplayServerWindows::_dispatch_input_event (C:\godot_source\platform\windows\display_server_windows.cpp:4404)
[21] DisplayServerWindows::_dispatch_input_events (C:\godot_source\platform\windows\display_server_windows.cpp:4375)
[22] Input::_parse_input_event_impl (C:\godot_source\core\input\input.cpp:903)
[23] Input::flush_buffered_events (C:\godot_source\core\input\input.cpp:1184)
[24] DisplayServerWindows::process_events (C:\godot_source\platform\windows\display_server_windows.cpp:3794)
[25] OS_Windows::run (C:\godot_source\platform\windows\os_windows.cpp:2253)
[26] widechar_main (C:\godot_source\platform\windows\godot_windows.cpp:97)
[27] _main (C:\godot_source\platform\windows\godot_windows.cpp:122)
[28] main (C:\godot_source\platform\windows\godot_windows.cpp:136)
[29] WinMain (C:\godot_source\platform\windows\godot_windows.cpp:150)
[30] __scrt_common_main_seh (D:\a\_work\1\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl:288)
[31] <couldn't map PC to fn name>
-- END OF C++ BACKTRACE --

EditorResourcePicker can be used outside inspector, you can't always assume a property.

Also the behavior could be improved. Right now with instant preview enabled you need to double click the resource. Maybe you could change it so clicking a selected resource will accept the selection.

KoBeWi avatar Jun 12 '25 09:06 KoBeWi

Instant preview should be disabled for global quick open dialogs, like the one for opening scenes:

https://github.com/user-attachments/assets/e4551595-8823-419a-a852-e616e8c3263b

Maybe hide the button if the dialog is not open for a property?

KoBeWi avatar Jun 12 '25 09:06 KoBeWi

Crash in theme editor EditorResourcePicker can be used outside inspector, you can't always assume a property.

I'll take a closer look at the theme editor case and more generally using Quick Load outside of the inspector soon, thanks!

Instant preview should be disabled for global quick open dialogs, like the one for opening scenes Maybe hide the button if the dialog is not open for a property?

Agreed, or at least there should be some option in the popup arguments to disable Instant Preview.

Also the behavior could be improved. Right now with instant preview enabled you need to double click the resource. Maybe you could change it so clicking a selected resource will accept the selection.

Not having the selection be confirmed with a single click is by design. The idea is that, just like how you can press the arrow keys to quickly flick through a bunch of potential resources, you can also single-click resources to see how they look, and then use a double-click to confirm a particular selection. While I suppose we could make it so that a single click on the currently-highlighted resource would confirm it, I don't think that's consistent with most other UX. (For example, in an OS's file browser, single-clicking a folder to select it and then single-clicking it again doesn't perform the same action as double-clicking it to open it.)

Meorge avatar Jun 12 '25 16:06 Meorge

The Instant Preview option and functionality should now only show up if the Quick Load menu is being used to change a property on a resource. Other cases, like the one for opening scenes, won't display the toggle and should behave just like they did in the past.

The crash with the theme editor should now be fixed. Unfortunately, for now it doesn't support Instant Preview and instead just works like it used to. This would be a very nice thing to add in a future PR, but for now I think having the Instant Preview functionality for resources in the inspector is at least better than nothing.

Meorge avatar Jun 12 '25 20:06 Meorge

Another crash:

CrashHandlerException: Program crashed
Engine version: Godot Engine v4.5.beta.custom_build (8de08c7c2151d4c49809fd52321ddfefb702d04b)
Dumping the backtrace. Please include this when reporting the bug on: https://github.com/godotengine/godot/issues
[0] Variant::~Variant (C:\godot_source\core\variant\variant.h:870)
[1] Variant::~Variant (C:\godot_source\core\variant\variant.h:870)
[2] UndoRedo::Operation::~Operation
[3] UndoRedo::add_do_property (C:\godot_source\core\object\undo_redo.cpp:216)
[4] EditorUndoRedoManager::add_do_property (C:\godot_source\editor\editor_undo_redo_manager.cpp:228)
[5] MultiNodeEdit::_set_impl (C:\godot_source\editor\multi_node_edit.cpp:84)
[6] MultiNodeEdit::_set (C:\godot_source\editor\multi_node_edit.cpp:38)
[7] MultiNodeEdit::_setv (C:\godot_source\editor\multi_node_edit.h:36)
[8] Object::set (C:\godot_source\core\object\object.cpp:351)
[9] UndoRedo::_process_operation_list (C:\godot_source\core\object\undo_redo.cpp:403)
[10] UndoRedo::_redo (C:\godot_source\core\object\undo_redo.cpp:82)
[11] UndoRedo::commit_action (C:\godot_source\core\object\undo_redo.cpp:331)
[12] EditorUndoRedoManager::commit_action (C:\godot_source\editor\editor_undo_redo_manager.cpp:256)
[13] EditorQuickOpenDialog::update_property (C:\godot_source\editor\gui\editor_quick_open_dialog.cpp:282)
[14] EditorQuickOpenDialog::ok_pressed (C:\godot_source\editor\gui\editor_quick_open_dialog.cpp:184)
[15] AcceptDialog::_ok_pressed (C:\godot_source\scene\gui\dialogs.cpp:140)
[16] AcceptDialog::_text_submitted (C:\godot_source\scene\gui\dialogs.cpp:127)
[17] call_with_variant_args_helper<AcceptDialog,String const &,0> (C:\godot_source\core\variant\binder_common.h:223)
[18] call_with_variant_args<AcceptDialog,String const &> (C:\godot_source\core\variant\binder_common.h:338)
[19] CallableCustomMethodPointer<AcceptDialog,void,String const &>::call (C:\godot_source\core\object\callable_method_pointer.h:107)
[20] Callable::callp (C:\godot_source\core\variant\callable.cpp:58)
[21] Object::emit_signalp (C:\godot_source\core\object\object.cpp:1288)
[22] Node::emit_signalp (C:\godot_source\scene\main\node.cpp:4130)
[23] Object::emit_signal<String> (C:\godot_source\core\object\object.h:941)
[24] LineEdit::gui_input (C:\godot_source\scene\gui\line_edit.cpp:878)
[25] Control::_call_gui_input (C:\godot_source\scene\gui\control.cpp:1867)
[26] Viewport::_gui_input_event (C:\godot_source\scene\main\viewport.cpp:2277)
[27] Viewport::push_input (C:\godot_source\scene\main\viewport.cpp:3430)
[28] Window::_window_input (C:\godot_source\scene\main\window.cpp:1800)
[29] call_with_variant_args_helper<Window,Ref<InputEvent> const &,0> (C:\godot_source\core\variant\binder_common.h:223)
[30] call_with_variant_args<Window,Ref<InputEvent> const &> (C:\godot_source\core\variant\binder_common.h:338)
[31] CallableCustomMethodPointer<Window,void,Ref<InputEvent> const &>::call (C:\godot_source\core\object\callable_method_pointer.h:107)
[32] Callable::callp (C:\godot_source\core\variant\callable.cpp:58)
[33] Callable::call<Ref<InputEvent> > (C:\godot_source\core\variant\variant.h:953)
[34] DisplayServerWindows::_dispatch_input_event (C:\godot_source\platform\windows\display_server_windows.cpp:4404)
[35] DisplayServerWindows::_dispatch_input_events (C:\godot_source\platform\windows\display_server_windows.cpp:4375)
[36] Input::_parse_input_event_impl (C:\godot_source\core\input\input.cpp:903)
[37] Input::flush_buffered_events (C:\godot_source\core\input\input.cpp:1184)
[38] DisplayServerWindows::process_events (C:\godot_source\platform\windows\display_server_windows.cpp:3794)
[39] OS_Windows::run (C:\godot_source\platform\windows\os_windows.cpp:2253)
[40] widechar_main (C:\godot_source\platform\windows\godot_windows.cpp:97)
[41] _main (C:\godot_source\platform\windows\godot_windows.cpp:122)
[42] main (C:\godot_source\platform\windows\godot_windows.cpp:136)
[43] WinMain (C:\godot_source\platform\windows\godot_windows.cpp:150)
[44] __scrt_common_main_seh (D:\a\_work\1\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl:288)
[45] <couldn't map PC to fn name>
-- END OF C++ BACKTRACE --

I think you should make _edit_set() public and call it from the quick open, if it's really needed. It's too much code to duplicate it, and you duplicate one is bugged. The crash happens when undoing setting on MultiNodeEdit.

KoBeWi avatar Jun 19 '25 15:06 KoBeWi

Thanks for the testing and review! I'll try to get to looking it over and implementing necessary changes soon, but since we're currently in feature freeze for 4.5 and I have some other projects I've fallen behind on, it'll be a little lower on my priorities for now 😅

Meorge avatar Jun 20 '25 21:06 Meorge

Now with 4.6 in development, I've returned to try and see this PR through.

For now, I've rolled back the attempt at copying the _edit_set() code, and instead just have the "confirm selection" code path work the same way that it did before this PR, by calling a callback method from EditorResourcePicker.

Bugs I'm aware of currently:

  • When Instant Preview is enabled and the Quick Open window is used to modify a property of a single object, the "undone" state seems to keep the same property value as the "done" state.
    • My guess here is that the temporarily-switched resource is being remembered as what was there before the operation, so the "undo" and "redo" operations save the same resource? If this is the case, I think it should be fixable by saving the current resource to a variable when the Quick Open window is opened, then just before the resource change is confirmed, set it back to that initial one.
  • When Instant Preview is enabled and the Quick Open window is used to modify a property on multiple objects at once, selecting a new resource in the window without confirming it still adds it to the undo/redo stack.
    • It looks to me like the undo/redo functionality is at least baked into the setting function for MultiNodeEdit. So I may need to add some kind of parameter to MultiNodeEdit, such that I can tell it I want to set the property without adding an undo/redo item.

(These notes are mostly just for myself to keep track of things, but if anyone can read them and has useful insights, I would really appreciate them!)

Having MultiNodeEdit support Instant Preview would be very nice, but if it seems like it's going to be a major hurdle to overcome due to deep structural stuff, I think it might be wise to have Instant Preview simply not activate for MultiNodeEdit, and then perhaps focus on that in a future PR.

Meorge avatar Sep 21 '25 05:09 Meorge

I've done a bit of testing, and so far editing both single nodes and groups of nodes at a time has seemed to work correctly!

To handle MultiNodeEdit, I've made the EditorQuickOpenDialog class a friend of it, so it can access the _set_impl() method; furthermore, I've added an optional argument to _set_impl() that prevents it from pushing to the history stack. This way, we reuse more code instead of duplicating it.

Meorge avatar Sep 21 '25 17:09 Meorge

MultiNode no longer crashes, but it creates wrong undo action when you cancel the dialog.

KoBeWi avatar Sep 25 '25 20:09 KoBeWi

That should be fixed now, I think! Sorry for all the runaround with the bugs, but thank you very much for testing it! 😅

I've also made it so that the first selection in each Quick Open dialog (i.e., the one that happens automatically when the window opens) does not trigger the "preview" behavior, overwriting whatever property value was previously set. The Quick Open window itself still displays the first item in its list as being selected; in the future, it could be nice to have the currently resource for the property be highlighted instead, but IMO the behavior now should be good enough.

Meorge avatar Sep 26 '25 03:09 Meorge

Thanks!

Repiteo avatar Oct 21 '25 20:10 Repiteo