Skip to content

Improve Popup content_scale_factor#104399

Merged
akien-mga merged 1 commit intogodotengine:masterfrom
scgm0:Improve-Popup-content_scale_factor
Mar 28, 2025
Merged

Improve Popup content_scale_factor#104399
akien-mga merged 1 commit intogodotengine:masterfrom
scgm0:Improve-Popup-content_scale_factor

Conversation

@scgm0
Copy link
Contributor

@scgm0 scgm0 commented Mar 20, 2025 •
edited
Loading

Change the content_scale_factor used by some Popups from being retrieved from the parent Window to being retrieved from the parent Viewport.

Fix #69749
Fix #69171
Fix #98672

master:
图片
图片

this pr:
图片
图片

test:
图片

popup_test_2025-03-20_23-17-23.zip

@scgm0 scgm0 requested review from a team as code owners March 20, 2025 09:35
@AThousandShips AThousandShips added this to the 4.x milestone Mar 20, 2025
@scgm0 scgm0 force-pushed the Improve-Popup-content_scale_factor branch 3 times, most recently from 8d6ee8d to 2e3e97c Compare March 21, 2025 14:31
@KoBeWi KoBeWi added bug and removed enhancement labels Mar 21, 2025
@KoBeWi KoBeWi modified the milestones: 4.x, 4.5 Mar 21, 2025
@scgm0 scgm0 force-pushed the Improve-Popup-content_scale_factor branch from 2e3e97c to 1ee6f6e Compare March 24, 2025 17:12
Calinou
Calinou approved these changes Mar 24, 2025
View reviewed changes
Copy link
Member

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

Tested locally, it works as expected both with subwindow embedding enabled and disabled.

However, when subwindow embedding is disabled, the stretched popup will be blurry when using the viewport stretch mode (instead of being pixelated as I'd expect). I recall seeing another issue (and PR) specifically tackling this.

Subwindow embedding enabled

disabled canvas_items viewport
image image image

Subwindow embedding disabled

disabled canvas_items viewport
image image image

Also, this PR doesn't fix #98672:

image

@scgm0
Copy link
Contributor Author

scgm0 commented Mar 24, 2025

图片
Changing the texture filter mode of the popup window would make it display pixelated as expected, but I think that's beyond the scope of this pr.
It's already very late here (currently 3 AM in China), so I'll check tomorrow to see if I can fix #98672.

@scgm0 scgm0 force-pushed the Improve-Popup-content_scale_factor branch from 1ee6f6e to 1a8828a Compare March 24, 2025 20:28
@scgm0
Copy link
Contributor Author

scgm0 commented Mar 24, 2025

@Calinou
I've been having some insomnia, so I took the opportunity to fix #98672:
图片
There's one modification I'm unsure about, which is that the tooltip now follows the scaling of the tooltip_owner. If this isn't suitable, I can revert this change:
Before:
图片
After:
图片

bruvzg
bruvzg approved these changes Mar 25, 2025
View reviewed changes
Copy link
Member

@bruvzg bruvzg left a comment

Choose a reason for hiding this comment

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

Seems to be working as expected with both embedded and native windows.

@akien-mga akien-mga changed the title Improve Popup content_scale_factor Improve Popup content_scale_factor Mar 25, 2025
@Calinou
Copy link
Member

Calinou commented Mar 25, 2025

There's one modification I'm unsure about, which is that the tooltip now follows the scaling of the tooltip_owner. If this isn't suitable, I can revert this change:

This makes sense to do, but I can see use cases where this is an issue. In general, we discourage using Control scaling outside of animation purposes (as it breaks within containers), but if you do use it for animation, it might look weird if it affects the tooltip at the same time as the button.

@scgm0
Copy link
Contributor Author

scgm0 commented Mar 25, 2025

This makes sense to do, but I can see use cases where this is an issue. In general, we discourage using Control scaling outside of animation purposes (as it breaks within containers), but if you do use it for animation, it might look weird if it affects the tooltip at the same time as the button.

So, should I cancel it?

@scgm0 scgm0 force-pushed the Improve-Popup-content_scale_factor branch from 1a8828a to 5ed281e Compare March 26, 2025 09:11
@Calinou
Copy link
Member

Calinou commented Mar 26, 2025 •
edited
Loading

So, should I cancel it?

Yes, I suggest removing that change from the PR then submitting it in a separate PR instead.

@scgm0 scgm0 force-pushed the Improve-Popup-content_scale_factor branch from 5ed281e to 27438a1 Compare March 26, 2025 15:48
@scgm0
Copy link
Contributor Author

scgm0 commented Mar 26, 2025

So, should I cancel it?

Yes, I suggest removing that change from the PR then submitting it in a separate PR instead.

Done.

@akien-mga akien-mga merged commit 0251fc4 into godotengine:master Mar 28, 2025
20 checks passed
@akien-mga
Copy link
Member

akien-mga commented Mar 28, 2025

Thanks!

@matheusmdx matheusmdx mentioned this pull request May 9, 2025
Closed
@kleonc kleonc mentioned this pull request Oct 18, 2025
Closed
@precup precup mentioned this pull request Nov 9, 2025
Closed
@bruvzg bruvzg mentioned this pull request Jan 10, 2026
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Calinou Calinou Calinou approved these changes

@bruvzg bruvzg bruvzg approved these changes

Assignees

No one assigned

Labels

bug topic:gui

Projects

None yet

Milestone

4.5

6 participants

@scgm0 @Calinou @akien-mga @bruvzg @KoBeWi @AThousandShips