Change embedded window options to use three stacked dots and add HDR info#118079
Conversation
d0e645c to
f3ddfb3
Compare
| Error SceneDebugger::_msg_hdr_output_request_state(const Array &p_args) { | ||
| DisplayServer *ds = DisplayServer::get_singleton(); | ||
| bool renderer_supports_hdr_output = false; | ||
| #if defined(RD_ENABLED) |
There was a problem hiding this comment.
I see that code that uses RD::get_singleton() is often wrapped with RD_ENABLED, but I don't actually know if this is appropriate in this case. Let me know if I shouldn't have this here.
There was a problem hiding this comment.
It should be needed since RenderingDevice is not defined if that option is off.
There was a problem hiding this comment.
Should the rendering_device.h include also be wrapped in RD_ENABLED if that’s the case?
You should use |
f3ddfb3 to
9d2eb63
Compare
|
Thanks, Calinou! Very helpful explanation. And it also pointed me to I've updated the use of |
There was a problem hiding this comment.
Tested locally on Linux/X11 (no HDR support), it works as expected. Code, docs and UX look good to me.
Note that while the project is loading, showing the dropdown will show a section without any options:
The option appears once the project is done loading:
It's not a big issue, but I thought I'd mention it nonetheless. Maybe we should show a temporary "Loading..." item while the project is still loading, as a section without any items looks odd.
AdriaandeJongh
left a comment
There was a problem hiding this comment.
Tested and works as expected. Agree with @Calinou's remark though:
Maybe we should show a temporary "Loading..." item while the project is still loading, as a section without any items looks odd.
|
I'm moving this back to "To Ratify" because I believe the suggestion that was provided by the reviewers to have "Loading..." written when the game is still loading is a good idea. It should also be fairly trivial to implement, so I should have it finished very shortly. |
9d2eb63 to
87a4ab4
Compare
|
I've pushed a change to add the "Loading..." text suggestion. I also added some cleanup code to make sure that subsequent runs don't display stale information while the debugger is loading. Because this PR depends on #118076, it shouldn't be merged until that prerequisite has been merged. |
DarkKilauea
left a comment
There was a problem hiding this comment.
LGTM, would like to see the luminance settings, but I understand the complications around that right now. Getting the HDR output status of the game is great for now.
|
Could perhaps be shortened to 'rendering driver'. |
|
I’ll change it to just be “Renderer”, since the rendering device driver is a configuration of the renderer. I’ll also see if I can add a tooltip to these error labels that gives some instruction to the user on how to resolve this. It would include mentioning changing the renderer or rendering device driver for this error. |
1f5ee2e to
edc0d8c
Compare
…window information. Co-authored-by: Josh Jones <kilauea.jones@gmail.com>
edc0d8c to
2a059cd
Compare
There was a problem hiding this comment.
Works great now 🙂 Code looks good to me.
One future improvement would be to change the unsupported tooltip on Linux/*BSD specifically to mention that the user should switch to the Wayland display driver, so that it's more explicit than just switching to "a display driver that supports HDR". This can be done separately still.
That said, maybe we can push for godotengine/godot-proposals#11583 for 4.8, making this less important to have in the future (since native Wayland would be the default).
|
Thanks! |
| suspend_button->set_disabled(empty); | ||
| camera_override_button->set_disabled(empty); | ||
| speed_state_button->set_disabled(empty); | ||
| game_size_label->set_visible(!empty); |
There was a problem hiding this comment.
I don't think it mitigates the issue shown in this stream and, in fact, makes it harder to discover in some cases. Assuming I'm not missing something obvious: Running the game in non-embed mode seems necessary when you want to record only the game. For example, I'm not a streamer but I make a short recording every day for my teammates with OBS, so I have to deactivate it. I think for this use case, the three dots in the far right corner looks like settings for this part of the editor UI, instead of other settings for the game view itself, since they are separated from other game view settings. I think the issue can be mitigated by aligning both window size and the three dots on the left with other game view settings. I agree this issue will be solved if this proposal (godotengine/godot-proposals#14445) is implemented but for the time being it seems a usability regression to me. For context I've tested in both 4.7 beta1 and 4.7 beta2. I hope this message is clear enough and it helps. Thank you for your work. It is still very much appreciated. |









This PR depends on #118076. This parent PR's two commits have been included in this PR.
What problem(s) does this PR solve?
What is the rationale for the approach used in this PR?
SDR Behaviour
By moving the game window pop-up menu to a three dots icon, the issue demonstrated in this stream should be resolved, or at least mitigated until a better and more substantial change has been implemented. Placing this three dots icon next to the game window size label should be an intuitive location to find game window options, such as the embedded window sizing.
Additionally, the game window size label is now always visible when the game is running, even if it is not embedded. This change was needed for displaying HDR info when the window is not embeded...
HDR Behaviour
A tooltip explains what the floating point value beside the "HDR" text represents: it's the maximum linear value. Additional HDR info is presented in this tooltip.
Importantly, no further HDR configuration options are provided beyond toggling
Window.hdr_output_requested. In the future, a slider should be provided to allow the user to limit the output maximum linear value to preview the appearance of different screen capabilities. This should be added in a followup PR when a maximum linear value limit parameter has been added to Godot. More details in this comment.Much of the remaining design rationale can be found in godotengine/godot-proposals#13903.
Was generative AI (LLM AI) used to create a portion of this PR?
When starting this PR, I took inspiration from https://github.com/DarkKilauea/godot/tree/editor/hdr-game-mode, which was created using AI tools. Although I copied some of the functions and approach when starting, I have re-wrote and re-worked it to be my own.
Are there any parts of this PR that you are uncertain of or require special attention from reviewers?
I really have no idea what I'm doing with theThis has been resolved!TTRCmacro. Am I using it correctly throughout this PR?I've tested this to find it works well on Windows, macOS, and Fedora KDE plasma.