Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make visual shader editor create node window a popup #99568

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

badsectoracula
Copy link
Contributor

This makes the Visual Shader editor's "Create Shader Node" window to be a popup instead of a dialog, thus being easily dismissed by clicking outside the window like in most other DCC applications that use a node editor. It addresses godotengine/godot-proposals#11213. It also solves the issue where the window would appear outside the screen area.

Please note that i have only tested this on Linux and X11.

add_node_popup

The main negative with this patch is that unlike the window, the popup is not resizable and instead a 300px (scaled by EDSCALE) minimum width is used to ensure that the node names are visible. While it is possible to make the window resizable by setting the Window::FLAG_RESIZE_DISABLED to false, there is no visual indicator that the window is resizable and the window closes as soon as the resize finishes (seems a race condition between the window manager and the X11 mouse handling code for popups). Ideally there should be a control that can be placed on popups (like a small triangle at the bottom right corner, as that is common in various toolkits) to handle client-side resizing, but i can't find anything like that in Godot.

A potential alternative would be to use the popup-based approach in this PR for right clicking but use a dialog when "Add Node" is selected from the popup menu. This will most likely require a larger change though as from what i can tell a window can't switch "modes" between being a Popup and a Dialog.

@passivestar
Copy link
Contributor

passivestar commented Nov 23, 2024

Please note that i have only tested this on Linux and X11.

It works on macos. Also looks better than a window imo


btw is there a reason it's not a PopupPanel like color pickers and most small editor popups? it could have rounded corners and OS-agnostic shadow after #91333 is merged if it was. (edit: nevermind I looked at code and it is a PopupPanel)

@tetrapod00
Copy link
Contributor

I tested casually on Windows 10, and this works as expected, if not better. I was really expecting some jankiness from this change.

I do wish the new popup was resizeable. Prior to this PR, I would often resize the window to about 2x the vertical size you have set for the popup. Even if that's not possible, this PR is a net UX improvement, IMO.


I did find one very edge case behavior, but it is exposing an existing edge case, not adding a new one. When:

  • using a floating shader editor,
  • which is split between two monitors, with the center of the window on the left monitor
  • and right clicking on the portion in the right monitor,

when the Create Shader Node UI is a window, it correctly appears at the clicked point. However, when the Create Shader Node UI is a popup, it appears on the right edge of the left monitor, since the center of the floating window is on that monitor.

Note that this is an existing behavior. In these videos you can see that both 4.3 and this PR have this behavior for the right-click context menu (when a node is selected), but in this PR, the behavior is also present for the Create Shader Node UI.

In these videos, the floating window is split between the monitors. UI popups spawn at the rightmost edge of the left monitor. Note that popups spawn to the left of the mouse.

4.3:

7xo5BSaKXm.mp4

This PR:

rXRpov8ZGr.mp4

I don't think this is something that needs to be fixed, since it is an edge case and a preexisting behavior.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants