Replace editor renderer dropdown with icon next to version button - #103042
Replace editor renderer dropdown with icon next to version button#103042Calinou wants to merge 1 commit into
Conversation
ANGLE can use OpenGL ES icon, it's emulating GL ES and on platforms it's used there's no native GL ES support. |
So only DX12 is a issue (current logo won't be ok for 16x16), but I guess old |
|
I think having to go under project settings to change this (in a place that might be hard to find) is a significant loss of usability, especially for new users who likely have no idea where to look in the project settings to find this |
|
If discoverability is a concern (finding it in the project settings), how about a compromise: add a "Rendering Method" sub-menu in the Project-menu under "Project Settings..."? |
How often are users changing the setting? I suspect that most people set it when creating the project (its a required setting in the project creation menu) and then never touch it again. |
|
What's the status of this PR? |
|
@syntaxerror247 It might be worth checking this alternative implementation: #109357 |
|
a710e87 to
3673f8f
Compare
3673f8f to
863bd1b
Compare
AdriaandeJongh
left a comment
AdriaandeJongh
left a comment
There was a problem hiding this comment.
Barring the icons, I think this is a great middle ground solution to removing the dropdown while keeping the renderer visible in videos and screenshots.
|
I think I like this change. The use-case for quick-access to switching renderers without going to project settings is when testing both a PC export and a mobile export when the project uses Forward+ for PC exports and Mobile for mobile exports. I really don't know how many users out there do this, so I can't comment. @BastiaanOlij do you know if this use case is common for XR? Even if it is, I don't see needing to go to project settings being a problem for these users. I think the colour for Compatibility could be improved for those with colour blindness, but it's a minor nitpick because compatibility will have its own "OpenGL" icon that makes it clear whether its compatibility or mobile.
tl;dr: this looks like a good change as-is and doesn't need any changes. Only things I see are minor or non-issues. |
|
@allenwp yes it's incredibly common for XR and a constant source of irritation when we change UX under the assumption that only one renderer is used. Many devs will use compatibility when deploying on standalone devices, and mobile or even Forward+ when deploying for PCVR. As a result they will often switch between compatibility and one of the other two because pushing out a build to a standalone device is costly, it really speeds up development when you can dev in one renderer for awhile, and then switch back to another renderer. We are slowly seeing people adopt the mobile renderer on standalone, but many will still use forward+ on PCVR and thus switch regularly between mobile and forward+ while testing Note that this is also why I am such an opponent of us hiding properties that apply to a renderer that is currently not selected. In our use case it's paramount to be able to interact with properties of the other renderers as we deploy to different ones for different target platforms. |
The rendering driver is represented by the icon, while the rendering method is represented by its color (green for Forward+, purple for Mobile, blue for Compatibility). This allows saving space at the top of the editor while providing more information than before on editor screenshots and videos, which is useful for technical support purposes. EditorVersionButton is now based on Button instead of LinkButton so that it can display an icon. However, this means its text is no longer underlined on hover.
863bd1b to
7daca3d
Compare
|
Rebased and tested again, it works as expected. See OP for updated screenshots. I considered using official logos as per #103042 (comment), but they don't scale down well to 16×16. The OpenGL/Vulkan 16×16 logos on the Khronos website are only provided in raster form, so they'd have to be recreated as SVGs to look good on hiDPI displays (or even 125% editor scale). The Metal logo would end up attracting quite a lot of attention with its bright background too (and Direct3D 12 when using a light theme), while this should be a secondary UI element. |
Overriding the rendering method and driver in the project settings using feature flags, and defining feature flags in export presets used to remote deploy, should basically solve this issue, no? Then in that workflow, the only toggle that needs to be changed is which of the export presets has the Runnable toggle set to true. |
If this approach does work for this use case, it sounds significantly more in-line with Godot's philosophies of designing the primary interface for new users who may be overwhelmed by too many options, while also providing these sort of advanced options to help the workflow of the minority of users who would benefit from it. I haven't tried it myself, but I think this approach would actually speed up development in most cases as it would avoiding needing to wait for the editor to restart? It might be a good idea to include this approach in the PR's description text for users who are accustomed to switching renderers from the main editor window when deploying to different devices. This way when people see a change that negatively affects their workflow, they will have a solution to improve their workflow readily available in the PR description. |
Yes I fully agree with that. This is a pretty niche use case, and while valid, the fact that it implies a pretty slow full editor restart is reason enough IMO for it not to use real estate in the main editor view. If there's a need, we can tuck away a "Temporary Renderer Override" option in the Project menu if the goal is to quickly check a combination which isn't the one in the project settings for the current host platform. |
|
Regarding this implementation, I'm generally in favor of removing the dropdown and showing this information in a more subtle way. But:
So personally I wonder if we really need to show this at all. I get that it's nice for issue triage to have it visible on screenshots in cases where the rendering driver is relevant, but this is something the issue template asks for and in practice many screenshots are cropped or are screenshots from the game. For screenshots which aren't cropped, we could consider putting this info in the title bar instead. |
|
I have been quite vocal about removing the icon entirely but there has been a LOT of pushback on that leading to a standstill for months (or actually, years). This PR is basically meeting everyone the middle, so for that reason I'd move it forward anyway. It's just 16 extra pixels in width in the bottom bar, right? |
You're overlooking that the dev will still want to change the active renderer in the editor and what they are running on. Sure I'm going to deploy my game with compatibility to Quest, and with mobile on PC and that is taken care of by the project settings and any needed overrules on export profiles, BUT often I want to tweak my compatibility settings while testing. HECK I might be remote debugging, running my app natively on the Quest, but my editor is still set to mobile/vulkan, so I still want to interact with the properties. Also I may change my renderer to compatibility temporarily on PC to tweak things, because its faster and easier seeing what the visual changes are, and then change it back to mobile when I'm done. Not having access to properties just because the current active configuration doesn't support it, is going to really slow a lot of devs down. And I don't think this is limited to XR devs, this applies to any dev that targets more than one platform where different renderers are required. |
|
Note that part of my concern here is due to the experience in Godot 3 days where it wasn't just hiding properties, it was also loosing properties, which caused a lot of issues for devs that target more than one renderer. |
|
I think this is a great change for flat dev where you dont switch between render methods often. I also agree that changing between render methods is pretty vital when developing for XR. Would it be possible to make removing the dropdown an editor setting? |
Moving these sorts of things to editor settings increases complexity of user experience and also increases the complexity and effort to maintain the editor. By increasing complexity it means that changes to the editor will become more difficult in the future and there is a higher risk for new bugs to be introduced in the future. But maybe in this case the code would be extremely simple and not introduce much risk. So maybe an editor setting to show the renderer switcher (default: false) might be reasonable for this scenario. I don't have strong personal opinions, but given the goal of this PR is to reduce the editor real estate of the renderer switcher: We could change the title to be "F+", "M", and "C" instead of writing out the full words. Not great because a new user would need to click on the OptionButton just to figure out what these letters stood for. Another option is to provide a renderer switching function as an official plugin on the asset store. I believe this plugin can be made to be identical to the current renderer switcher. This matches the Godot contributing guidelines to implement functionality in plugins when the feature is not required for all developers. I don't think there are major issues with the existing behaviour or any of the proposed behaviours (or this PR), so I'm pretty indifferent on all of this. But if there is a use case for the current behaviour, then I don't see it as beneficial to solve one minor problem just to introduce another minor problem. One other thing is that there are clearly issues that exist with using the editor inspector when remote debugging a game that's running with a different renderer, so these should probably be addressed in a different proposal/PR. This seems like a relatively trivial fix that would slightly reduce the amount of time needed to switch renderers in some use cases. |
Problem
Considerations
ProposalI propose:
I think this approach will address all of the use cases and solve the problem. The two proposed things could be implemented as different PRs. I don't expect an editor setting to be a large amount of code complexity or a problem to maintain in the long run. I don't have a strong preference on the second proposed item: It's just a question of what takes up the smallest amount of important screen real estate and minimizes visual noise. Edit: it’s probably clear I have no idea how to satisfy Godot developers who want to see the renderer and rendering device driver in screenshots. I believe this desire of Godot developers to see this info in screenshots is fundamentally at odds with what is best for Godot users who need a minimal and uncluttered editor. Because of this, I think I side with a design that is focused on the Godot user: remove all indications of the renderer and rendering device driver from the editor’s main GUI (and title bars) unless the user has enabled the renderer drop down in their editor settings. |
There was a problem hiding this comment.
The renderer dropdown just changes the renderer project setting and restarts which could be done in a plugin, so I don't think we need to make an editor setting for it.
And then you could customize it for your use case like not having a confirmation dialog if you really need to switch back and forth often.
But if you want the current functionality, here is the current renderer dropdown as a plugin. It is based on the editor code so it should be the same:
Re-Add Renderer Dropdown Plugin Code
@tool
extends EditorPlugin
var renderer : OptionButton
var video_restart_dialog : ConfirmationDialog
func _enter_tree() -> void:
renderer = OptionButton.new()
renderer.set_flat(true)
renderer.set_theme_type_variation("TopBarOptionButton")
renderer.set_fit_to_longest_item(false)
renderer.set_focus_mode(Control.FOCUS_ACCESSIBILITY)
renderer.set_auto_translate_mode(AUTO_TRANSLATE_MODE_DISABLED)
renderer.set_tooltip_auto_translate_mode(AUTO_TRANSLATE_MODE_ALWAYS)
renderer.set_tooltip_text("Choose a renderer.\n\nNotes:\n- On mobile platforms, the Mobile renderer is used if Forward+ is selected here.\n- On the web platform, the Compatibility renderer is always used.")
renderer.set_accessibility_name("Renderer")
add_control_to_container(EditorPlugin.CONTAINER_TOOLBAR, renderer)
var current_renderer_ps := String(ProjectSettings.get_setting_with_override("rendering/renderer/rendering_method")).to_lower()
var current_renderer_os := RenderingServer.get_current_rendering_method().to_lower()
# Add the renderers name to the UI.
if current_renderer_ps == current_renderer_os:
renderer.item_selected.connect(_renderer_selected)
# As we are doing string comparisons, keep in standard case to prevent problems with capitals
# "vulkan" in particular uses lowercase "v" in the code, and uppercase in the UI.
var props := ProjectSettings.get_property_list()
var render_prop_index := props.find_custom(func (prop): return prop.name == "rendering/renderer/rendering_method")
if render_prop_index == -1:
printerr("Cannot find rendering method property info!")
return
var renderers: PackedStringArray = props[render_prop_index].hint_string.split(",", false)
for i in range(renderers.size()):
var rendering_method := renderers[i].to_lower()
if rendering_method == "dummy":
continue
renderer.add_item(String()) # Set in NOTIFICATION_TRANSLATION_CHANGED.
renderer.set_item_metadata(-1, rendering_method)
if current_renderer_ps == rendering_method:
renderer.select(i)
else:
# It's an CLI-overridden rendering method.
renderer.add_item(String()) # Set in NOTIFICATION_TRANSLATION_CHANGED.
renderer.set_item_metadata(-1, current_renderer_os)
_update_renderer_color()
EditorInterface.get_base_control().theme_changed.connect(_update_renderer_color)
func _exit_tree() -> void:
EditorInterface.get_base_control().theme_changed.disconnect(_update_renderer_color)
if video_restart_dialog != null:
video_restart_dialog.queue_free()
video_restart_dialog = null
remove_control_from_container(EditorPlugin.CONTAINER_TOOLBAR, renderer)
renderer.queue_free()
renderer = null
func _notification(what: int) -> void:
if what == NOTIFICATION_TRANSLATION_CHANGED:
# The rendering method selector.
var current_renderer_ps := String(ProjectSettings.get_setting_with_override("rendering/renderer/rendering_method")).to_lower()
var current_renderer_os := RenderingServer.get_current_rendering_method().to_lower()
if current_renderer_ps == current_renderer_os:
for i in range(renderer.item_count):
renderer.set_item_text(i, _to_rendering_method_display_name(renderer.get_item_metadata(i)))
else:
# TRANSLATORS: The placeholder is the rendering method that has overridden the default one.
renderer.set_item_text(0, TTR("%s (Overridden)") % _to_rendering_method_display_name(current_renderer_os))
func _renderer_selected(p_index) -> void:
var rendering_method : String = renderer.get_item_metadata(p_index)
var current_renderer := ProjectSettings.get_setting_with_override("rendering/renderer/rendering_method")
if rendering_method == current_renderer:
return
# Don't change selection.
for i in range(renderer.item_count):
if renderer.get_item_metadata(i) == current_renderer:
renderer.select(i)
break
if video_restart_dialog == null:
video_restart_dialog = ConfirmationDialog.new()
video_restart_dialog.set_ok_button_text("Save & Restart")
video_restart_dialog.get_label().set_auto_translate_mode(AUTO_TRANSLATE_MODE_DISABLED)
EditorInterface.get_base_control().add_child(video_restart_dialog)
else:
video_restart_dialog.confirmed.disconnect(_set_renderer_name_save_and_restart)
var mobile_rendering_method := "mobile" if rendering_method == "forward_plus" else rendering_method
var web_rendering_method := "gl_compatibility"
video_restart_dialog.confirmed.connect(_set_renderer_name_save_and_restart.bind(rendering_method))
video_restart_dialog.set_text(
TTR("Changing the renderer requires restarting the editor.\n\nChoosing Save & Restart will change the renderer to:\n- Desktop platforms: %s\n- Mobile platforms: %s\n- Web platform: %s") % [
_to_rendering_method_display_name(rendering_method), _to_rendering_method_display_name(mobile_rendering_method), _to_rendering_method_display_name(web_rendering_method)
]
)
video_restart_dialog.popup_centered()
_update_renderer_color()
func _to_rendering_method_display_name(p_rendering_method):
if p_rendering_method == "forward_plus":
return TTR("Forward+")
if p_rendering_method == "mobile":
return TTR("Mobile")
if p_rendering_method == "gl_compatibility":
return TTR("Compatibility")
return p_rendering_method
func _update_renderer_color() -> void:
var rendering_method : String = renderer.get_selected_metadata()
var theme := EditorInterface.get_editor_theme()
var renderer_normal_color := theme.get_color(rendering_method + "_color", "Editor")
var mono_color := theme.get_color("mono_color", "Editor")
renderer.add_theme_color_override("font_color", renderer_normal_color)
renderer.add_theme_color_override("font_hover_color", renderer_normal_color.lerp(mono_color, 0.3))
renderer.add_theme_color_override("font_pressed_color", renderer_normal_color.lerp(mono_color, 0.4))
renderer.add_theme_color_override("font_hover_pressed_color", renderer_normal_color.lerp(mono_color, 0.5))
func _set_renderer_name_save_and_restart(p_rendering_method : String) -> void:
ProjectSettings.set_setting("rendering/renderer/rendering_method", p_rendering_method)
if p_rendering_method == "mobile" or p_rendering_method == "gl_compatibility":
# Also change the mobile override if changing to a compatible renderer.
# This prevents visual discrepancies between desktop and mobile platforms.
ProjectSettings.set_setting("rendering/renderer/rendering_method.mobile", p_rendering_method)
elif p_rendering_method == "forward_plus":
# Use the equivalent mobile renderer. This prevents the renderer from staying
# on its old choice if moving from `gl_compatibility` to `forward_plus`.
ProjectSettings.set_setting("rendering/renderer/rendering_method.mobile", "mobile")
ProjectSettings.save()
EditorInterface.save_all_scenes()
EditorInterface.restart_editor()
func TTR(text : String) -> String:
if not TranslationServer.has_domain("godot.editor"):
return text
return TranslationServer.get_or_add_domain("godot.editor").translate(text)
(I'm not sure if the translations will continue to work after).
The most important part is the _set_renderer_name_save_and_restart method, you could use just that and make shortcuts that call it with the different options if you wanted.
I'm not fond of the d3d, vk, mt text icons.
I tried a mockup using regular text. It's wider, and I'm not sure if its better:
The renderer and rendering device driver should be added to the title of the main editor window and also the game view window
There is usually lots of space in the title bar, so I agree this should be considered as an alternative if the icon is too problematic.
| editor_main_screen->add_theme_style_override(SceneStringName(panel), theme->get_stylebox(SNAME("Content"), EditorStringName(EditorStyles))); | ||
| bottom_panel->_theme_changed(); | ||
| distraction_free->set_button_icon(theme->get_icon(SNAME("DistractionFree"), EditorStringName(EditorIcons))); | ||
| update_distraction_free_button_theme(); | ||
|
|
||
| help_menu->set_item_icon(help_menu->get_item_index(HELP_SEARCH), get_editor_theme_native_menu_icon(SNAME("HelpSearch"), menu_type == MENU_TYPE_GLOBAL, dark_mode)); | ||
| help_menu->set_item_icon(help_menu->get_item_index(HELP_COPY_SYSTEM_INFO), get_editor_theme_native_menu_icon(SNAME("ActionCopy"), menu_type == MENU_TYPE_GLOBAL, dark_mode)); | ||
| help_menu->set_item_icon(help_menu->get_item_index(HELP_ABOUT), get_editor_theme_native_menu_icon(SNAME("Godot"), menu_type == MENU_TYPE_GLOBAL, dark_mode)); | ||
| help_menu->set_item_icon(help_menu->get_item_index(HELP_SUPPORT_GODOT_DEVELOPMENT), get_editor_theme_native_menu_icon(SNAME("Heart"), menu_type == MENU_TYPE_GLOBAL, dark_mode)); |
That's a readable compromise solution considering there seems to be a problem with getting the icons small and readable at the same time
Uhhh I think most screenshot tools show the window title bar therefore people will see the renderer/driver in screenshots (I admit this can be useful for tutorials, just like the version number was placed in a visible spot for this reason) |
|
I believe that the approach used to communicate information about the current renderer to Godot developers who are looking at screen recordings or screenshots should be handled separately from changes to the visibility of the renderer selector. For this reason, I have made a PR that simply hides the renderer selector and provides an editor setting to show it: #117754 This PR or similar ones can be handled and discussed separately to address desires of Godot developers to see more information about users' project configurations based on screen recordings or screenshots.
I believe that there is a negative user experience from adding additional text to any part of the main Godot editor window, including the title bar. This additional text adds visual noise and complexity, especially in the title bar on macOS, which has the same vertical position as workspace buttons, and run/pause buttons: 
(In this image, "my scene thing.tsc" is the current scene and "HDR Output" is the name of the project.)
|
|
Superseded by #117754. |
14 participants
The rendering driver is represented by the icon, while the rendering method is represented by its color (green for Forward+, purple for Mobile, blue for Compatibility).
This allows saving space at the top of the editor while providing more information than before on editor screenshots and videos, which is useful for technical support purposes. As a bonus, the rendering driver and method are now visible in the project manager.
EditorVersionButton is now based on Button instead of LinkButton so that it can display an icon. However, this means its text is no longer underlined on hover.
In a future PR, we can also expand the usage of these icons to be displayed next to each project on the project list.
Preview
Forward+
Mobile
Compatibility
Mobile in the editor
Forward+ in the project manager
Tooltip with rendering driver and method
TODO
OpenGL ES,ANGLEand use them.