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

[DisplayServer] Add method to check if window transparency is supported and enabled. #91505

Merged
merged 1 commit into from
May 23, 2024

Conversation

bruvzg
Copy link
Member

@bruvzg bruvzg commented May 3, 2024

Adds method to check is transparency is usable, see #91333

@bruvzg bruvzg added this to the 4.x milestone May 3, 2024
@bruvzg bruvzg force-pushed the detect_comp branch 2 times, most recently from 091206c to 4959119 Compare May 3, 2024 09:36
@AThousandShips AThousandShips changed the title [DisplayServer] Add method to check is window transparency is supported and enabled. [DisplayServer] Add method to check if window transparency is supported and enabled. May 3, 2024
@bruvzg bruvzg marked this pull request as ready for review May 14, 2024 17:21
@bruvzg bruvzg requested review from a team as code owners May 14, 2024 17:22
doc/classes/DisplayServer.xml Outdated Show resolved Hide resolved
platform/linuxbsd/x11/display_server_x11.cpp Outdated Show resolved Hide resolved
platform/macos/display_server_macos.mm Outdated Show resolved Hide resolved
platform/windows/display_server_windows.cpp Outdated Show resolved Hide resolved
doc/classes/DisplayServer.xml Outdated Show resolved Hide resolved
@LunarTides
Copy link

To me, "is_window_transparency_enabled" reads like "is_window_transparency_currently_enabled" which might cause confusion. Maybe rename it to something like "is_window_transparency_supported"?

@Riteo
Copy link
Contributor

Riteo commented May 15, 2024

is_window_transparency_supported

The issue with this proposal is, IMO, that it clashes a bit too much with DisplayServer::has_feature. I guess that, in a way, it is "currently" enabled.

Perhaps is_window_transparency_available would be a good middle-ground? It still feels a bit conflicting with DisplayServer::has_feature(FEATURE_WINDOW_TRANSPARENCY) but not as much.

@LunarTides
Copy link

LunarTides commented May 15, 2024

I guess that, in a way, it is "currently" enabled.

I am not exactly sure how the display server handles transparency, but based on its name and description in the documentation: "Returns true if the window background can be made transparent", it seems like more of a "is this feature supported/available" rather than "is this feature enabled". Or at least it would seem that way had I looked at its documentation, although I may be misunderstanding what this function is for, or maybe the phrasing is just throwing me off.

Edit: On second thought, I think this is just a phrasing issue. English is not my native language, so it was hard to understand. But I still think it should be renamed for clarity.

@akien-mga akien-mga merged commit 5d6c789 into godotengine:master May 23, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

bruvzg added a commit to bruvzg/godot that referenced this pull request May 25, 2024
akien-mga added a commit that referenced this pull request May 25, 2024
Fix build with `vulkan=no` and `d3d12=no` after #91505.
Crashim03 pushed a commit to Crashim03/godot that referenced this pull request May 27, 2024
@RandomShaper
Copy link
Member

RandomShaper commented May 28, 2024

I know I'm late, but I'd like to support the idea of this being checked via has_feature(FEATURE_WINDOW_TRANSPARENCY).

I'm aware the new function added here checks if both supported and enabled. However, that's something that projects could check anyway via has_feature() and the display/window/per_pixel_transparency/allowed project setting.

If this new function is really important as a shortcut (which I'm not very sure about), it could be implemented in such terms.

@Riteo
Copy link
Contributor

Riteo commented May 28, 2024

@RandomShaper I like that idea too! The only issue is that, IMO, the use of that method is a bit ambiguous.

I think that DisplayServer::has_feature is supposed to fetch only "compile-time enabled" features, a bit like with other engine features, so it would not be a good fit for that, but that's my interpretation.

@RandomShaper
Copy link
Member

That's indeed how it's being used at the moment for the most part. However, the method is not described as not able to perform checks at runtime. DisplayServerWeb is an example of something we already have that isn't following such a constraint, by making runtime checks for features such as FEATURE_IME.

We could maybe specify the method as being runtime-constant (instead of compile-time constant).

@Riteo
Copy link
Contributor

Riteo commented May 29, 2024

DisplayServerWeb is an example of something we already have that isn't following such a constraint, by making runtime checks for features such as FEATURE_IME.

Oh wow, I didn't know that. TBH, I'd be in favour. There are already a bunch other "dynamic" features that come to mind, especially for the Wayland platform, which is build extensively on global objects that theoretically could disappear and reappear at any moment.

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.

None yet

6 participants