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

Improve Popup content_scale_factor #104399

Merged

Conversation

scgm0
Copy link
Contributor

@scgm0 scgm0 commented Mar 20, 2025

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
Copy link
Member

@Calinou Calinou left a comment

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:
图片

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

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants