godot icon indicating copy to clipboard operation
godot copied to clipboard

Make `PopupMenu/Panel` shadows properly visible again

Open YeldhamDev opened this issue 1 year ago • 11 comments

This PR does the following:

  • Make PopupMenu/Panel nodes have spacing around them to show the shadows of their theming, as well as repositioning them accordingly with their shadows' size when popping up.
  • Make PopupMenu/Panel nodes be transparent by default. Not only this is necessary for the shadows, but also for rounded corners. It slightly breaks compatibility, as a default value is changed. But I think it's non-intrusive enough to be acceptable.
  • Fix some bugs related to content scaling, and submenu positioning.

image

Closes https://github.com/godotengine/godot-proposals/issues/7532. Closes https://github.com/godotengine/godot-proposals/issues/8738.

Test Project: shadow-of-the-popups.zip

YeldhamDev avatar Apr 29 '24 20:04 YeldhamDev

I'd say this also closes https://github.com/godotengine/godot-proposals/issues/8738 (by superseding it).

Calinou avatar Apr 29 '24 21:04 Calinou

Doesn't look right with the default theme on mac 🙃

91333

passivestar avatar May 02 '24 13:05 passivestar

@passivestar Could you enable display/window/per_pixel_transparency/allowed in the project settings?

YeldhamDev avatar May 02 '24 14:05 YeldhamDev

@passivestar Could you enable display/window/per_pixel_transparency/allowed in the project settings?

yeah that works

91333-2

passivestar avatar May 02 '24 14:05 passivestar

I've added a warning that appears when a PopupMenu/Panel has a theme that needs per pixel transparency but it isn't enabled.

Note that currently it always appears when running the game because the default project theme has rounded corners. If this is merged, it will need to be modified.

YeldhamDev avatar May 02 '24 15:05 YeldhamDev

We probably can enable transparency for the editor by default, it may increase VRAM usage a bit, but should not have any significant performance impact on modern systems.

Also, I'll take a look if we can test if transparency is really supported and working (checks if compositor is active and surface/front buffer have correct format), and will add a method to the display server.

bruvzg avatar May 02 '24 16:05 bruvzg

@bruvzg Nice! But I think having the warning is a good thing nonetheless.

YeldhamDev avatar May 02 '24 16:05 YeldhamDev

Themed the menu to take advantage of this PR, makes a huge difference imo

popup2

As I understand transparency needs to be an editor setting because we probably don't want a project setting to affect editor theming. Even better if enabled by default like @bruvzg suggested

P.S. Clicking on the popup menu shadow doesn't close the popup which is a bit annoying with a large shadow

passivestar avatar May 02 '24 16:05 passivestar

P.S. Clicking on the popup menu shadow doesn't close the popup which is a bit annoying with a large shadow

Got you covered. :wink:

YeldhamDev avatar May 02 '24 20:05 YeldhamDev

Updated to use the new is_window_transparency_available() function.

YeldhamDev avatar May 23 '24 21:05 YeldhamDev

Still needs transparency to be enabled in the project settings to work but also it looks like things broke since the last time I tested this PR:

  • Popup menus now have scrollbars
  • Submenu is drawn at a weird distance
  • When shadow is enabled it's only drawn in corners inside of the bounding box of the menu
No shadow Shadow
1 2

Same with the default theme:

3

Got you covered. 😉

Can't test if clicking on shadows to close the menu is fixed because there's no shadow to click :)

passivestar avatar Oct 01 '24 21:10 passivestar

@passivestar Indeed, it seems that this PR rotted after a while. Sadly, I don't have any time to work on this at all currently.

YeldhamDev avatar Oct 09 '24 16:10 YeldhamDev

@passivestar Welp, I'm a masochist, so I worked on it anyways. Please test again.

YeldhamDev avatar Oct 10 '24 04:10 YeldhamDev

@Geometror Enable display/window/per_pixel_transparency/allowed in the Project Settings, the shadows shall appear in the running game and in the editor when you restart. A warning about it appears when you run without the setting being enabled.

YeldhamDev avatar Nov 28 '24 15:11 YeldhamDev

I actually tested with that setting enabled and disabled. Transparency wasn't the problem, it's that the shadow is cropped.

image

Geometror avatar Nov 28 '24 15:11 Geometror

How does the shadows look in your end in with my test project?

YeldhamDev avatar Nov 28 '24 16:11 YeldhamDev

image Well, they are not there :)

Geometror avatar Nov 28 '24 20:11 Geometror

Are you sure you didn't change anything? I'm on GNU/Linux (Wayland) and the shadows work fine. Maybe try rebasing it to master in your end?

YeldhamDev avatar Nov 28 '24 21:11 YeldhamDev

The test project on my end (transparency is not enabled in the project so I had to enable it):

image

passivestar avatar Nov 28 '24 21:11 passivestar

Ok, I finally got it to work. Sorry, I assumed this project setting was turned on in the test project :) However, this only works for me when using the compatibility backend (both Linux (Wayland) and Windows 11), despite per pixel transparency being supported (https://github.com/godotengine/godot/pull/91333#issuecomment-2506386147) So it looks like if DisplayServer::get_singleton()->is_window_transparency_available() doesn't determine correctly whether it is available.

Geometror avatar Nov 28 '24 23:11 Geometror

@Geometror Huh, all backends work for me.

Anyways, CC @bruvzg, since you were the one who implemented it.

YeldhamDev avatar Nov 29 '24 01:11 YeldhamDev

Should also work on macOS (with any renderer) and on Windows with D3D 12, but Vulkan depend on the driver and NVIDIA broke it some time ago.

bruvzg avatar Nov 29 '24 05:11 bruvzg

@Geometror Changes made.

YeldhamDev avatar Nov 29 '24 14:11 YeldhamDev

@Geometror @AThousandShips Changes made.

YeldhamDev avatar Dec 02 '24 17:12 YeldhamDev

This fix doesn't work properly when display/window/subwindows/embed_subwindows is true (not just shadows, but almost all theme settings).

It seems that for native windows, we need another set of code to handle the theme

shitake2333 avatar Dec 12 '24 09:12 shitake2333

@molingyu Both embedded and non-embedded works fine here. Did you enable display/window/per_pixel_transparency/allowed?

YeldhamDev avatar Dec 12 '24 23:12 YeldhamDev

@molingyu Both embedded and non-embedded works fine here. Did you enable display/window/per_pixel_transparency/allowed?嵌入式和非嵌入式在这里都工作正常。您是否启用了 display/window/per_pixel_transparency/allowed

No.

I used the Test Project provided by PR and the shading is still incorrect.

My environment: windows 10

shitake2333 avatar Dec 13 '24 08:12 shitake2333

I tried enabling display/window/per_pixel_transparency/allowed.

2f9273e91bd7dff64f5a64b17051bd11

Shadows that extend beyond the bounds are still clipped(I used the build and test projects provided by the pr action and only changed the shadow to blue to make it easier to spot.).

By the way, my editor also became abnormally transparent.

87c30c0a1842dcccf7982662afae238d

Additional information: My graphics card is AMD Radeon(TM) Graphics

Godot v4.4.dev (dc78b4c5d) - Windows 10.0.19045 - Multi-window, 2 monitors - Vulkan (Forward+) - integrated AMD Radeon(TM) Graphics (Advanced Micro Devices, Inc.; 27.20.15026.8004) - AMD Ryzen 5 5500U with Radeon Graphics (12 threads)

I checked with other people using Windows and they have no issues.

So this may be caused by the graphics card?

At this point, pr should be fine. Could the problem come from somewhere else?

shitake2333 avatar Dec 13 '24 10:12 shitake2333

cc @YeldhamDev

It looks like the issue has not been fixed in the Forward+ / Vulkan (Windows) backend when display/window/subwindows/embed_subwindows is set to false. I also get the following warning messages when launching the project under the abovementioned configuration:

W 0:00:00:0760   PopupMenu::_notification: The current theme styles PopupMenu to have shadows and/or rounded corners, but those won't display correctly if 'display/window/per_pixel_transparency/allowed' isn't enabled in the Project Settings, nor if it isn't supported.
<C++ Source>   scene\gui\popup_menu.cpp:1113 @ PopupMenu::_notification()
W 0:00:00:0762   PopupPanel::_notification: The current theme styles PopupPanel to have shadows and/or rounded corners, but those won't display correctly if 'display/window/per_pixel_transparency/allowed' isn't enabled in the Project Settings, nor if it isn't supported.
<C++ Source>   scene\gui\popup.cpp:348 @ PopupPanel::_notification()

To clarify, I do have the per_pixel_transparency enabled in the project setting, so it's somewhat not recognized by the backend.

Edit: I also ran the test on a macOS machine, and the issue in forward+ seems to be exclusive to the Windows platform.

Windows & Open GL:

Godot v4.4.dev (f7f6432af) - Windows 10.0.19044 - Multi-window, 4 monitors - OpenGL 3 (Compatibility) - NVIDIA GeForce RTX 4060 Ti (NVIDIA; 32.0.15.6603) - AMD Ryzen 9 5900X 12-Core Processor (24 threads)

Embed Not Embed
image image

Windows & Vulkan:

Godot v4.4.dev (f7f6432af) - Windows 10.0.19044 - Multi-window, 4 monitors - Vulkan (Forward+) - dedicated NVIDIA GeForce RTX 4060 Ti (NVIDIA; 32.0.15.6603) - AMD Ryzen 9 5900X 12-Core Processor (24 threads)

Embed !! Not Embed !!
image image

macOS & Open GL:

Godot v4.4.dev (f7f6432af) - macOS 15.1.1 - Multi-window, 1 monitor - OpenGL 3 (Compatibility) - Apple M4 - Apple M4 (10 threads)

Embed Not Embed
image 1bbfbf45ab074b798783365b0f6d62f3

macOS & Metal

Godot v4.4.dev (f7f6432af) - macOS 15.1.1 - Multi-window, 1 monitor - Metal (Forward+) - integrated Apple M4 (Apple9) - Apple M4 (10 threads)

Embed Not Embed
f66527c76f0e9d8f6d3c012d7dd5732f image

Delsin-Yu avatar Dec 13 '24 10:12 Delsin-Yu

@Delsin-Yu The warning comes from display/window/per_pixel_transparency/allowed not being enabled by default. We should probably make so that this is always enabled for the editor itself as well in another PR.

And about it not working on some backends, it's unrelated to the PR itself, and on @bruvzg's ballpark.

YeldhamDev avatar Dec 16 '24 15:12 YeldhamDev