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.

Sign up for GitHub

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

Jump to bottom

Fix non-embedded tooltips, popups and dialogs resizing to match Viewport content scale factors != 1.0 #86553

Merged
merged 1 commit into from
Apr 18, 2024
Merged

Fix non-embedded tooltips, popups and dialogs resizing to match Viewport content scale factors != 1.0 #86553

merged 1 commit into from
Apr 18, 2024

Conversation

Koyper
Copy link
Contributor

@Koyper Koyper commented Dec 27, 2023 •
edited by akien-mga
Loading

Required for fixes to #69749, #69171, and for fixes to the issues in #54030 when embed_subwindows is disabled.
Probably fixes #44942.
Bugsquad edit: Fixes #89482.

This will not relate directly to a fix for the blurry subwindows issue, but might help with the eventual solution for that.

This fix causes the PopupPanel children to correctly resize to fit the popup when the containing Viewport.content_scale_factor is changed. Without this fix, the children would expand outside the popup bounds if the scale factor was greater than 1.0, or shrivel if less than 1.0.

This fix is required for several separate PR's I will post, the first of which is to make tooltips scale to match the main viewport scale factor when the project embed_subwindows is disabled.

Implementation is slightly different depending on whether the popup is embedded or not. If embedded, the popup will scale simply by changing the viewport content scale factor, if the popup is not embedded, i.e., using a floating system window, the popup content scale factor should be set to match the containing viewport's content scale factor. If this is done, the rendering will be correct (non-blurry).

The fixes below show the properly scaled tooltip that will be in a separate PR.

BEFORE FIX:

Screen.Recording.2023-12-27.at.11.17.35.AM.mov

ALSO BEFORE FIX - Note the tooltip scale stuck at 1.0

Screen.Recording.2023-12-27.at.11.19.17.AM.mov

AFTER FIX:

Screen.Recording.2023-12-27.at.11.15.44.AM.mov
Screen.Recording.2023-12-27.at.11.21.02.AM.mov

@Koyper Koyper force-pushed the fix_popup_panel_content_scaling branch from 97f2c0a to de49a2d Compare December 27, 2023 16:47
@Koyper Koyper marked this pull request as ready for review December 27, 2023 17:28
@Koyper Koyper requested a review from a team as a code owner December 27, 2023 17:28
@Koyper Koyper mentioned this pull request Dec 27, 2023
Closed
@YeldhamDev YeldhamDev added this to the 4.3 milestone Dec 27, 2023
AThousandShips
AThousandShips reviewed Dec 27, 2023
View reviewed changes
scene/gui/popup.cpp Outdated Show resolved Hide resolved
Sauermann
Sauermann reviewed Dec 29, 2023
View reviewed changes
scene/gui/popup.cpp Outdated Show resolved Hide resolved
@Koyper Koyper force-pushed the fix_popup_panel_content_scaling branch from de49a2d to 8f84fc7 Compare January 1, 2024 00:41
@Koyper
Copy link
Contributor Author

Koyper commented Jan 4, 2024

I have a related question regarding PR best practices: I've finished modifications to the remaining classes that did not respond correctly to content_scale_factor values other than 1.0. Should I post a separate PR for each of these or do them as a group in a single PR? The files modified are tree.cpp, popup_menu.cpp and dialog.cpp. Thanks!

@akien-mga
Copy link
Member

akien-mga commented Jan 4, 2024 •
edited
Loading

If it's the same bug in different related classes, I would suggest fixing all of them in the same PR (so they could be added to this one, expanding its scope a bit). I'd go as far as suggesting to make the changes in a single commit (so amend this PR's commit with the similar changes to other Control classes).

@Koyper
Copy link
Contributor Author

Koyper commented Jan 4, 2024

If it's the same bug in different related classes, I would suggest fixing all of them in the same PR (so they could be added to this one, expanding its scope a bit). I'd go as far as suggesting to make the changes in a single commit (so amend this PR's commit with the similar changes to other Control classes).

So what about #86556 that would otherwise be included in this new group? Should I amend this PR with those changes and then close that PR?

@akien-mga
Copy link
Member

Yeah I think that would make sense. Fix all those related issues with one fell swoop.

@Koyper Koyper force-pushed the fix_popup_panel_content_scaling branch from 8f84fc7 to 69b99fa Compare January 4, 2024 18:27
@Koyper Koyper requested a review from a team as a code owner January 4, 2024 18:27
@Koyper Koyper changed the title PopupPanel: Fix child resizing to Viewport content scale factor Fix non-embedded tooltips, popups and dialogs resizing to match Viewport content scale factors != 1.0 Jan 4, 2024
@Koyper
Copy link
Contributor Author

Koyper commented Jan 4, 2024

I've amended this commit to add all the related changes required to implement proper scaling of non-embedded windows when Viewport.content_scale_factor is not equal to 1.0.

This includes tooltips, PopupPanel, PopupMenu, Tree, ConfirmationDialog and AcceptDialog.

@Koyper Koyper mentioned this pull request Jan 4, 2024
Open
@akien-mga akien-mga requested review from KoBeWi and Sauermann January 4, 2024 22:17
Sauermann
Sauermann reviewed Jan 6, 2024
View reviewed changes
scene/main/viewport.cpp Outdated
@@ -1527,17 +1529,22 @@ void Viewport::_gui_show_tooltip() {

tooltip_owner->add_child(gui.tooltip_popup);

Window *window = gui.tooltip_popup->get_parent_visible_window();
Copy link
Contributor

Choose a reason for hiding this comment

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

In the case that gui.tooltip_popup is embedded, it is possible, that its embedder is not the parent visible window.

Can you please try:

Suggested change
Window *window = gui.tooltip_popup->get_parent_visible_window();
Window *window = gui.tooltip_popup->get_embedder();
if (window == null) {
window = ui.tooltip_popup->get_parent_visible_window();
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Change made - thanks!

@Koyper Koyper force-pushed the fix_popup_panel_content_scaling branch 2 times, most recently from 0b38a37 to b3a5a6d Compare January 6, 2024 21:21
@scgm0
Copy link
Contributor

scgm0 commented Apr 8, 2024

How's the progress of this PR?The problem of blurry sub-windows is really annoying...

@Koyper
Copy link
Contributor Author

Koyper commented Apr 11, 2024

I'll have to take care of the conflicts with a rebase, but other than that the PR is ready to merge, pending approvals, which I've been awaiting for a while.

I've tested it for MacOS and Windows and all is working correctly for me (I've been using it continuously since the PR was posted)

I should get to the rebase within a few days...

@Koyper Koyper force-pushed the fix_popup_panel_content_scaling branch 2 times, most recently from 44bead9 to 2887ae3 Compare April 12, 2024 23:35
@Koyper
Copy link
Contributor Author

Koyper commented Apr 12, 2024 •
edited
Loading

Ok, ready for review after fixing some issues with PopupMenu created by the removal of the internal margin container by #87462.

Fixes #89482

@Koyper Koyper force-pushed the fix_popup_panel_content_scaling branch from 2887ae3 to d5bc977 Compare April 12, 2024 23:53
@Koyper Koyper mentioned this pull request Apr 13, 2024
Closed
KoBeWi
KoBeWi reviewed Apr 17, 2024
View reviewed changes
scene/gui/dialogs.cpp Outdated Show resolved Hide resolved
KoBeWi
KoBeWi reviewed Apr 17, 2024
View reviewed changes
scene/gui/popup.cpp Outdated Show resolved Hide resolved
KoBeWi
KoBeWi reviewed Apr 17, 2024
View reviewed changes
scene/gui/tree.cpp Outdated Show resolved Hide resolved
KoBeWi
KoBeWi reviewed Apr 17, 2024
View reviewed changes
scene/main/viewport.cpp Outdated Show resolved Hide resolved
KoBeWi
KoBeWi reviewed Apr 17, 2024
View reviewed changes
scene/main/viewport.cpp
@@ -1461,6 +1461,8 @@ void Viewport::_gui_show_tooltip() {
panel->set_flag(Window::FLAG_NO_FOCUS, true);
panel->set_flag(Window::FLAG_POPUP, false);
panel->set_flag(Window::FLAG_MOUSE_PASSTHROUGH, true);
panel->set_flag(Window::FLAG_TRANSPARENT, true);
// A non-embedded tooltip window will only be transparent if per_pixel_transparency is allowed in the main Viewport.
Copy link
Member

Choose a reason for hiding this comment

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

This comment should be above the flag change.
Is this related to what this PR fixes though?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's true that the PR would work fine without this, but you could not have transparent backgrounds in the non-embedded tooltips. Should I remove this and at some point do a separate PR as a bug-fix?

My sense is that because this PR makes non-embedded tooltips workable (because you couldn't use them previously for projects on HiDPI displays), no one would have come accross this bug.

Copy link
Member

Choose a reason for hiding this comment

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

This seems to have caused a regression where editor tooltips are transparent unexpectedly: #92712 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

A non-embedded tooltip window will only be transparent if per_pixel_transparency is allowed in the main Viewport.

It's allowed in the project settings not for a Viewport, and only affect requested surface format on window creation, but nothing prevents GPU driver from always using surface with alpha channel. And seems like Windows DisplayServer do not have any additional checks (unlike X11 and macOS) and will try to enable it regardless of project settings.

KoBeWi
KoBeWi reviewed Apr 17, 2024
View reviewed changes
scene/main/viewport.cpp Outdated Show resolved Hide resolved
KoBeWi
KoBeWi reviewed Apr 17, 2024
View reviewed changes
scene/main/viewport.cpp Outdated Show resolved Hide resolved
KoBeWi
KoBeWi approved these changes Apr 17, 2024
View reviewed changes
Copy link
Member

@KoBeWi KoBeWi left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I did some testing and it appears to be working correctly.
The code looks fine (aside from the style comments I left), but someone could double-check the viewport changes.

@Koyper Koyper force-pushed the fix_popup_panel_content_scaling branch from d5bc977 to 0881c81 Compare April 17, 2024 14:30
@Koyper
Copy link
Contributor Author

Koyper commented Apr 17, 2024

Thanks for the comments! I've made the suggested changes, and for now left in the transparency flag fix, but can remove if recommended.

@akien-mga akien-mga merged commit 3a88373 into godotengine:master Apr 18, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

@akien-mga akien-mga mentioned this pull request Apr 18, 2024
Closed
@Koyper Koyper deleted the fix_popup_panel_content_scaling branch April 24, 2024 16:08
@kleonc kleonc mentioned this pull request Jun 7, 2024
Closed
@StefanH-AT StefanH-AT mentioned this pull request Jun 8, 2024
Closed
@kitbdev kitbdev mentioned this pull request Aug 1, 2024
Closed
@bruvzg bruvzg mentioned this pull request Aug 1, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AThousandShips AThousandShips AThousandShips left review comments

@bruvzg bruvzg bruvzg left review comments

@Sauermann Sauermann Sauermann left review comments

@akien-mga akien-mga akien-mga left review comments

@KoBeWi KoBeWi KoBeWi approved these changes

Assignees
No one assigned
Labels
bug topic:gui
Projects
None yet
Milestone
4.3
8 participants
@Koyper @akien-mga @scgm0 @KoBeWi @Sauermann @bruvzg @AThousandShips @YeldhamDev