Fix behavior of window_is_hdr_output_supported for Wayland and adjust warnings.#117913
Conversation
allenwp
force-pushed
the
hdr-output-allow-request
branch
2 times, most recently
from
05159a8 to
917abac
Compare
| @@ -857,7 +857,10 @@ _FORCE_INLINE_ int _convert_utf32_offset_to_utf16(const String &p_existing_text, | |||
|
|
|||
| void DisplayServerAppleEmbedded::window_request_hdr_output(const bool p_enabled, DisplayServerEnums::WindowID p_window) { | |||
| #if defined(RD_ENABLED) | |||
| ERR_FAIL_COND_MSG(p_enabled && rendering_device && !rendering_device->has_feature(RenderingDevice::Features::SUPPORTS_HDR_OUTPUT), "HDR output is not supported by the rendering device."); | |||
| if (p_enabled && (!rendering_device || !rendering_device->has_feature(RenderingDevice::Features::SUPPORTS_HDR_OUTPUT))) { | |||
| WARN_PRINT("HDR output requested, but is not supported by the rendering device."); | |||
There was a problem hiding this comment.
I'm not sure these should be warnings. In my opinion, warnings indicate that the operation you requested didn't necessarily fail, but may have some limitations or side effects. An error indicates that the operation you requested didn't work at all.
Since you cannot turn on HDR if the rendering device does not support it, this seems like an error to me.
There was a problem hiding this comment.
I don't think it's a big problem to have all of these messages (including the DisplayServer not supporting HDR one) be ERR_FAIL_COND_MSG.
It may be true that it should be an error instead of a warning... But there appears to be a strong precedence for simply printing a warning as demonstrated in the base class of DisplayServer, where a number of properties simply print a warning when setting a property fails because the feature of that property isn't supported by the display server.
I'm trying to think of why this may be the case in DisplayServer... I presume this is because there are no negative consequences to try using a feature that isn't supported? Maybe the reason warnings are used throughout is because an error message communicates to the user: "hey, something here is very wrong and you have to fix it because something is probably very broken and not shippable", where a warning is "hey, you're trying to do something you can't do, but that's fine because the engine is handling it gracefully and we just thought we should let you know".
There was a problem hiding this comment.
@bruvzg Could you weigh in on what the right thing to do here is?
There was a problem hiding this comment.
Not sure, I do not think we handle it fully consistently. But this is not a critical failure, so warning should be fine.
allenwp
force-pushed
the
hdr-output-allow-request
branch
2 times, most recently
from
a4eed9e to
88ff096
Compare
ArchercatNEO
left a comment
ArchercatNEO
left a comment
There was a problem hiding this comment.
As far as I am concerned I am okay with the implementation. If the PR changes for other reasons (warn to error, more conditions, etc) I will stick around.
…endering device for HDR output support and adjust warnings/errors.
allenwp
force-pushed
the
hdr-output-allow-request
branch
from
88ff096 to
4309120
Compare
|
Thanks! |
6 participants
What problem(s) does this PR solve?
DisplayServerWayland::window_is_hdr_output_supportedreturnstruewhen the rendering device does not support HDR output. This behaviour is incorrect and does not match the behaviour of other display servers.If Godot is compiled with
RD_ENABLEDnot defined orrendering_device == nullptr, therendering_device->has_feature(RenderingDevice::Features::SUPPORTS_HDR_OUTPUT)checks will never be run and the code assumes that HDR output is supported, which is the opposite behaviour of how this should be written.What is the rationale for the approach used in this PR?
The problem is resolved by simply adding a check to the
DisplayServerWayland::window_is_hdr_output_supportedfunction so it matches other display servers.In addition to this change, I have adjusted some errors to be warnings and improved some of the warning messages.
Finally, because the functions that enable HDR output do not check if the rendering device supports HDR output, I have slightly changed the
window_request_hdr_outputlogic to fail to request HDR output whenrendering_deviceisnullptrorRD_ENABLEDis not defined. This is safer logic that ensures its impossible for HDR output to be successfully requested when the rendering device does not support HDR output.Was generative AI (LLM AI) used to create a portion of this PR?
No.
Are there any parts of this PR that you are uncertain of or require special attention from reviewers?
No, but please double check that my logic statements are correct.