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 PopupMenu/Panel shadows properly visible again #91333

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

Conversation

YeldhamDev
Copy link
Member

@YeldhamDev YeldhamDev commented Apr 29, 2024

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 godotengine/godot-proposals#7532.
Closes godotengine/godot-proposals#8738.

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

@Calinou
Copy link
Member

Calinou commented Apr 29, 2024

I'd say this also closes godotengine/godot-proposals#8738 (by superseding it).

@YeldhamDev YeldhamDev force-pushed the working_on_this_felt_like_constantly_walking_into_rakes branch from 131d3db to 8d20d90 Compare April 30, 2024 05:44
@passivestar
Copy link
Contributor

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

91333

@YeldhamDev
Copy link
Member Author

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

@passivestar
Copy link
Contributor

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

yeah that works

91333-2

@YeldhamDev YeldhamDev force-pushed the working_on_this_felt_like_constantly_walking_into_rakes branch from 8d20d90 to eaab897 Compare May 2, 2024 15:45
@YeldhamDev
Copy link
Member Author

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.

scene/gui/popup_menu.h Outdated Show resolved Hide resolved
scene/gui/popup_menu.cpp Show resolved Hide resolved
scene/gui/popup_menu.cpp Show resolved Hide resolved
@bruvzg
Copy link
Member

bruvzg commented May 2, 2024

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.

@YeldhamDev
Copy link
Member Author

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

@YeldhamDev YeldhamDev force-pushed the working_on_this_felt_like_constantly_walking_into_rakes branch from eaab897 to 19520af Compare May 2, 2024 16:13
scene/gui/popup_menu.cpp Outdated Show resolved Hide resolved
scene/gui/popup_menu.cpp Outdated Show resolved Hide resolved
scene/gui/popup.cpp Outdated Show resolved Hide resolved
@YeldhamDev YeldhamDev force-pushed the working_on_this_felt_like_constantly_walking_into_rakes branch from 19520af to 98bdb7e Compare May 2, 2024 16:22
@passivestar
Copy link
Contributor

passivestar commented May 2, 2024

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

@YeldhamDev YeldhamDev force-pushed the working_on_this_felt_like_constantly_walking_into_rakes branch from 98bdb7e to 320dd6a Compare May 2, 2024 20:56
@YeldhamDev
Copy link
Member Author

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. 😉

@YeldhamDev
Copy link
Member Author

Updated to use the new is_window_transparency_available() function.

@YeldhamDev YeldhamDev force-pushed the working_on_this_felt_like_constantly_walking_into_rakes branch from 01f8e13 to a056b31 Compare June 19, 2024 00:33
@YeldhamDev YeldhamDev force-pushed the working_on_this_felt_like_constantly_walking_into_rakes branch from a056b31 to f75ccce Compare July 13, 2024 12:29
@YeldhamDev YeldhamDev force-pushed the working_on_this_felt_like_constantly_walking_into_rakes branch from f75ccce to c0414b5 Compare July 13, 2024 12:30
@YeldhamDev YeldhamDev modified the milestones: 4.3, 4.4 Jul 25, 2024
@passivestar
Copy link
Contributor

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 :)

@YeldhamDev
Copy link
Member Author

@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 YeldhamDev force-pushed the working_on_this_felt_like_constantly_walking_into_rakes branch from c0414b5 to c56cf20 Compare October 10, 2024 04:42
@YeldhamDev
Copy link
Member Author

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

@YeldhamDev YeldhamDev force-pushed the working_on_this_felt_like_constantly_walking_into_rakes branch from c56cf20 to 4634c47 Compare October 10, 2024 16:03
Copy link
Contributor

@passivestar passivestar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works great on my end, can confirm that clicking on shadows closes popups now! Can't wait for this to be merged

s s2

Copy link
Member

@Geometror Geometror left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, I can't get this to work for me. (tested on Windows 11 and Linux[Wayland])

grafik
Doesn't matter whether the popup is embedded or native, in the 2D editor or running project.

Test project:
mrp1_popup_shadows.zip

@YeldhamDev
Copy link
Member Author

YeldhamDev commented Nov 28, 2024

@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.

@Geometror
Copy link
Member

Geometror commented Nov 28, 2024

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

image

@YeldhamDev
Copy link
Member Author

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

@Geometror
Copy link
Member

image
Well, they are not there :)

@YeldhamDev
Copy link
Member Author

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?

@passivestar
Copy link
Contributor

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

image

@Geometror
Copy link
Member

Geometror commented Nov 28, 2024

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 (#91333 (comment))
So it looks like if DisplayServer::get_singleton()->is_window_transparency_available() doesn't determine correctly whether it is available.

@YeldhamDev
Copy link
Member Author

@Geometror Huh, all backends work for me.

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

@bruvzg
Copy link
Member

bruvzg commented Nov 29, 2024

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.

Copy link
Member

@Geometror Geometror left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, haven't looked at the code yet, but functionality-wise this it's almost ready - just one thing left:
This should work regardless of the backend when subwindows are embedded (plus, it should also work in the 2D editor, but I guess this comes for free when it works for embedded subwindows)

Related to this PR, but not required/just for reference:

  • DisplayServer::get_singleton()->is_window_transparency_available() doesn't report per-pixel transparency for Wayland, although it is supported (in my case) as you can see in my screenshot
  • Per-pixel transparency doesn't work for me on Windows 11 both with DX12 and Vulkan (single GPU system, 3060 Ti), which is kind of strange (at least when using DX12)

@YeldhamDev YeldhamDev force-pushed the working_on_this_felt_like_constantly_walking_into_rakes branch from 4f71e58 to 7c928ca Compare November 29, 2024 14:42
@YeldhamDev
Copy link
Member Author

@Geometror Changes made.

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.

Allow a way to set transparent background to popups in themes. Improve popup customization in Godot 4
6 participants