godotengine / godot-proposals

Godot Improvement Proposals (GIPs)
MIT License
1.15k stars 97 forks source link

Shorten too-long exported resource dropdown menus and switch to CreateDialog #43

Open willnationsdev opened 5 years ago

willnationsdev commented 5 years ago

Describe the project you are working on:

Describe how this feature / enhancement will help your project:

When one does export(Resource) var res = null, they are presented with an empty exported resource that can be supplied with any type of resource. The "valid_inheritors" of this resource includes all possible resource types in the engine as well as all possible Custom Type or Script Class resources.

When you click the dropdown menu, the EditorPropertyResource class calls _update_menu_items and populates the dropdown with one New <type> menu item for every valid inheritor of the exported type. For Resources (and probably a few others), this floods the UI with a list that is obscenely long to navigate.

What's worse is that users who operate on a touchpad-equipped laptop, and who don't have a mouse with a scroll wheel, CANNOT scroll this list at all (or if there is a way, it is not intuitive as I cannot figure out how to do it). Using a keyboard to move the selection down the list will make it move down, but the list will not scroll up to continue displaying the moving-down selector to you, so you are blind.

Either way, the UX in this scenario is abysmal. I propose that we assign a maximum number of allowed menu items to be generated, and when the size has been reached, we instead add an "Open Create Dialog" button and stop generating menu items. If the user selects the Create Dialog, the EditorPropertyResource would configure it for the currently exported type and then open the dialog for the user to select the type.

Show a mock up screenshots/video or a flow diagram explaining how your proposal will work:

This is what exporting a Resource looks like before clicking the dropdown menu: prechanges_before_click

This is what it looks like afterwards: prechanges_after_click

This is what exporting an AudioEffect looks like: prechanges_after_click_audio

Assuming the number of entries had been reached at something like 23 (I was gonna say 20), the AudioEffect dropdown POST the suggested changes might look like this: postchanges_after_click_audio

Clicking the button would prompt the opening of the CreateDialog (but configured for AudioEffect in this example, not for Resource which is what the image below shows): postchanges_after_click

Describe implementation detail for your proposal (in code), if possible:

  1. Add a const int maximum number of menu items to the EditorPropertyResource class in editor/editor_properties.h
  2. When the EditorPropertyResource is instantiated, it should check to see how many valid inheritors there are of its exported property type. If it exceeds the maximum limit, it should instantiate a CreateDialog and configure it for the specified resource type. It should then also connect a callback to this CreateDialog so that it can assign itself the value of any selected class.
  3. In editor/editor_properties.cpp, EditorPropertyResource::_update_menu_items, keep a running count of which menu item index you are adding. When it becomes equal to the maximum allowed count AND there are still more to render, then add a menu item with no functionality that is just an ellipses (to indicate that there are more that exist) and then add a "Open Create Dialog..." menu item.
  4. When the menu item for the CreateDialog is selected, it should fetch the internally managed CreateDialog and open it center-screen.

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

It is used very often for data-driven games on small screens / laptops. It is built into the editor source code and is not very pluggable via the EditorPlugin's available API, so no, it cannot be worked around.

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

The only code capable of making the necessary changes is direct alterations to the EditorPropertyResource class in the editor source code.

Xrayez commented 5 years ago

Either way, the UX in this scenario is abysmal. I propose that we assign a maximum number of allowed menu items to be generated, and when the size has been reached, we instead add an "Open Create Dialog" button and stop generating menu items.

I'd rename "Open Create Dialog" to "Browse" and move to the top, unless populating numerous resource items is severe performance-wise, because then you'd have to account for window size for relatively small number of maximum elements, and you'd still have to scroll down if the popup doesn't fit the window.

Razzeeyy commented 5 years ago

Also searchbox at the top of that dropdown would be useful, that way you don't have to click on open dialog button if you know the name you're looking for, so it will just show that filtered part of the list.

willnationsdev commented 5 years ago

@Razzeeyy Having a searchbox at the top would accomplish the same function that adding a full create dialog would though, and the searchbox would involve needing to make changes to the engine core since PopupMenu doesn't expose an API for adding custom Controls to its internal VBoxContainer. I think adding both is going overboard.

Edit: Although, maybe I could just add a method that exposes the VBoxContainer itself and the external context can add in a search filter all by itself(?). That would be a very minor change to the engine's scene/gui code, so I don't see why we couldn't do that at least. But even if we did make that change, I still think having both a search bar at the top of the EditorPropertyResource's PopupMenu and having a CreateDialog is too much search/filter work going on for such a small use case. We should aim to keep things simple/consolidated, so I think re-using the existing CreateDialog (and the future optimizations it would get from #22) is the better play here.

@Xrayez What if instead, we just add an ellipses to the bottom of the list and clicking on it would open the CreateDialog? Would you guys rather have that, or a disabled ellipses (just there to indicate that there are more, unlisted entries) AND a "Browse..." at the top that only appears when there are too many, just like the ellipses at the bottom?

Razzeeyy commented 5 years ago

As for me I actually meant we can do both. If it doesn't hurt anyone's sense of style and not a burder to implement. ^^'

Though I'm not sure if that search bar would come in handy in different areas. Also would be great if someone else voiced there opinions too. Would be great to hear more from the community to know which method the majority would prefer...

Xrayez commented 5 years ago

If I "hope" that I'll find a resource manually by scrolling but eventually scroll down to bottom anyway, then I wouldn't need to scroll up/re-popup the menu again just to "Browse...", so adding ellipses would make sense here as well I guess.

groud commented 5 years ago

I think adding a popup would be really bad UX-wise, complexifying the process of creating a new resource. Either a search bar, or basically making the popup menu scrollable are better ideas.

willnationsdev commented 5 years ago

@groud We'd need core engine changes for that, wouldn't we (to add an optional search bar filter for a subset of PopupMenu items, the updating of which does not affect the search bar menu item)? I guess I'm good with taking that approach, but then we would also be duplicating a lot of ClassDB/script-class management logic and inheritance caching between EditorPropertyResource and CreateDialog whenever #22, point 4 is addressed.

In which case, I think we'd need to refactor out inheritance hierarchy caches into their own third-party class which EditorPropertyResource and CreateDialog can reference on their own; something like an EditorInheritanceCache class in EditorData or something. Otherwise we'll start seeing the same slowdowns currently present in the CreateDialog inside of the EditorPropertyResource's PopupMenu.

Do you see that being approved?

groud commented 5 years ago

If something is needed twice, it can indeed be made common (if it's complex enough obviously). If the code is clean I don't see any problem with that.

astrale-sharp commented 4 years ago

this is true! I support this!

Jummit commented 4 years ago

Would be partially fixed by godotengine/godot#36687.

this is true! I support this!

BTW, I think bumping issues without information isn't that welcome.

Jummit commented 4 years ago

Note that PopupMenu already has a allow_search property, which enables navigation with the letter keys. This is just strangely not enabled on most of the dropdowns in the editor.

Calinou commented 4 years ago

@Jummit Is there a reason why allow_search isn't enabled by default?

Jummit commented 4 years ago

@Jummit Is there a reason why allow_search isn't enabled by default?

I don't think so.

Reneator commented 4 years ago

Alternative idea:

Implement the search bar into the dropdown menu itself, as the 'allow_search' might be too hidden for beginners:

image

Now by typing something in the search bar it would automatically limit to the options that contain the typed in string (i chose the search bar over the implicit typing in the keyboard, as it might be something that people new to godot might miss) image

image

The "Edit etc." options are hidden below, in a collapse menu, so they are always available (not just at the very bottom of the context) but hideable. This menu might remember if the user had it open the last time the user was in a resource context (or had it collapsed) image These buttons could also just open to the bottom (not pushing into the options)

Mockup-Project: Godot proposal resource context.zip

The feature proposal: #18 might help to limit this by a lot for most of my cases, but i still think its much better for everyone if the resource context got improved in its useability.

The mockup is focused on rough layout or estimate functionality and is not a specific dictate. Its mainly meant to communicate my idea and not represent the final expectations. As this is mainly about the functionality i didnt bother with the icons

Calinou commented 4 years ago

@Jummit I noticed it's allowed by default in OptionButton but not PopupMenu. Here's a pull request where it's enabled by default in PopupMenu: https://github.com/godotengine/godot/pull/38811

CookieBadger commented 1 year ago

Would be nice to have this in 4.0 or 4.x, since the usability of this context menu is - to put it mildly - minimal, compared to the rest of the editor.

YuriSizov commented 1 year ago

@ExquisiterEmil This is less of an issue in 4.0+ because you can export any custom resource you have, so you practically never need to export just Resource where this issue would be most noticeable.

CookieBadger commented 1 year ago

Is that possible in beta 10 already? I am exporting a custom Resource (in C#) and still get the abysmal drop down...

YuriSizov commented 1 year ago

It's possible, but maybe not in C#. See https://github.com/godotengine/godot/pull/63126