godotengine / godot

Godot Engine – Multi-platform 2D and 3D game engine
https://godotengine.org
MIT License
89.22k stars 20.23k forks source link

Merge From Scene option invokes two file handling dialogs #36474

Closed Jose-Moreno closed 3 years ago

Jose-Moreno commented 4 years ago

Godot version: v3.2.Stable.Official

OS/device including version: Windows 10 Pro Version 1809 OS Build 17763.973

Issue description: Using the Merge From Scene option from any node, Root, Inherited or Instance will open two distinct file dialogs. One to open a file, and another one to import a file.

Video Demo

  1. This is a video demonstration of the issue
  2. This is a screenshot of the issue

To me it seems that it should only open "one" window to load a file instead of two. Someone on discord helped me triage and faced the same issue.

I'm not a c++ expert but could it be that the FILE_IMPORT_SUBSCENEcase is falling through to the FILE_EXTERNAL_OPEN_SCENEcase found here: https://github.com/godotengine/godot/blob/a24aafcb92992f860694bf411acab1080fc0b3db/editor/editor_node.cpp#L2319

Steps to reproduce:

  1. Create a new scene
  2. Create a root node (the node type doesn't matter I used Node2D, the other user used base Node)
  3. Right click on the node and click on the Merge From Scene context option

Minimal reproduction project: With the reproduction steps you get a 100% reproduction rate.

Edit: Updated description to take less space and added video showcasing the problem

timothyqiu commented 4 years ago

This is a feature introduced by #28292. If no scene is selected, the file dialog is automatically opened (as the only thing you can do in the dialog is to click the Browse button).

https://github.com/godotengine/godot/blob/a24aafcb92992f860694bf411acab1080fc0b3db/editor/editor_sub_scene.cpp#L74-L78

Maybe the title of the file dialog should be changed to something like "Choose a Scene".

Jose-Moreno commented 4 years ago

@timothyqiu Hi thank you for your comment, however I don't think we're talking about the same issue? Can you please read the description in detail? The problem is not about the name of the dialog, and just in case the scene is set as main scene, but the issue happens regardless of this.

If you follow the reproduction steps you should experience the problem i'm reporting, I know it's a simple thing but I don't see how automatically opening two different dialogs at the same time helps with the merge from scene functionality.

I'll update the screenshots as it seems there's a misunderstanding here, but what i'm reporting is clearly a UI / UX bug.

Jose-Moreno commented 4 years ago

@timothyqiu Ok, now I have some more time. I was reading on the phone previously so apologies for any inconvenience. Once again thank you for taking the time to review this.

After reading the PR I think I understand what you meant now as well. So the file selector dialog a.k.a open a file dialog, was implemented on that PR to occur automatically the first time the Merge From Sceneis used and a file is chosen. It will not be sprung afterwards.

This dialog will then appear ON TOP of the node import without further notice, but after selecting a scene, the next time we use the merge from scene option, only the node import will appear and you will have to click on the browse button to prompt this dialog.

To me it was confusing when the window opened at first when I was expecting only the node import window, but I understand the reasoning of it happening the first time.

With this, I unfortunately think this should still be called out as a UI / UX bug, but perhaps it's something that could be "fixed" through documentation rather than source code regression. Although for the sake of consistency I think the open file dialog should not appear on it's own as it feels it does not respect user choice and could confuse a new user.

So should I close this and ask for documentation instead? or do you think this issue can be used to prompt any modification to this behavior? Thanks in advance for your time.

KoBeWi commented 3 years ago

This doesn't look like a bug. The FileDialog is needed, the second dialog is not FileDialog, it's a separate dialog that displays scene hierarchy. It's impossible/not worth it to merge them.

Also the Merge from scene will be probably removed as obsolete after #34892 is merged.