Skip to content

Scale and resize window according to screen dpi#25

Merged
atomic-junky merged 4 commits into
mainfrom
feature/window-scale
Aug 21, 2024
Merged

Scale and resize window according to screen dpi#25
atomic-junky merged 4 commits into
mainfrom
feature/window-scale

Conversation

@atomic-junky

@atomic-junky atomic-junky commented Aug 20, 2024

Copy link
Copy Markdown
Collaborator

When a screen has a hidpi, the window becomes very small, which makes it rather complicated to use at such a small size. The solution is to calculate the scale of the window according to the dpi of the screen.

Summary by Sourcery

Scale the application window according to the screen's DPI to ensure proper sizing on high-DPI displays and enhance user experience by centering the window upon resize.

New Features:

  • Implement dynamic window scaling based on screen DPI to improve usability on high-DPI displays.

Enhancements:

  • Add functionality to center the window upon resizing to maintain a consistent user experience.

@atomic-junky atomic-junky requested a review from RailKill August 20, 2024 17:45
@sourcery-ai

sourcery-ai Bot commented Aug 20, 2024

Copy link
Copy Markdown

Reviewer's Guide by Sourcery

This pull request implements window scaling and resizing based on screen DPI to improve usability on high-DPI displays. The changes involve modifying the WelcomeWindow behavior and introducing a new global App script to handle DPI-based scaling.

File-Level Changes

Files Changes
Scripts/Global/App.gd Implemented DPI-based window scaling calculation
Scripts/Windows/WelcomeWindow.gd Added window centering functionality
Scripts/Windows/WelcomeWindow.gd Refactored window resizing logic
Scripts/Windows/WelcomeWindow.gd Connected resized signal to update window position

Tips
  • Trigger a new Sourcery review by commenting @sourcery-ai review on the pull request.
  • Continue your discussion with Sourcery by replying directly to review comments.
  • You can change your review settings at any time by accessing your dashboard:
    • Enable or disable the Sourcery-generated pull request summary or reviewer's guide;
    • Change the review language;
  • You can always contact us if you have any questions or feedback.

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Hey @atomic-junky - I've reviewed your changes - here's some feedback:

Overall Comments:

  • Consider a more flexible approach to scaling that doesn't rely on a hardcoded BASE_SCALE_DPI and doesn't cap the scale at 2.0. This would better accommodate a wider range of displays and DPI settings.
  • Saving the scale to a custom config file might override user preferences or cause issues when switching between displays. Consider alternative methods for persisting or calculating the scale that don't involve saving to a file.
Here's what I looked at during the review
  • 🟡 General issues: 2 issues found
  • 🟢 Security: all looks good
  • 🟢 Testing: all looks good
  • 🟢 Complexity: all looks good
  • 🟢 Documentation: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.

Comment thread Scripts/Global/App.gd
Comment thread Scripts/Global/App.gd Outdated

@RailKill RailKill left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

It's a great idea but it is pretty hard to use on default window size in windowed mode on big screens (2560x1440):

ui-scaling1

Even on fullscreen, I feel that the UI scaling is too much and there is some quality loss on some of the controls. Main issue is that it makes windowed mode a poor experience on large screens.

Comment thread Scripts/Global/App.gd Outdated
var guessed_screen_scale: float = min(round(screen_dpi/BASE_SCALE_DPI), 2.0)

ProjectSettings.set_setting("display/window/stretch/scale", guessed_screen_scale)
ProjectSettings.save_custom("user://override.cfg")

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This doesn't do anything when you export the project, it only works when you run Monologue from the Godot editor. When using override.cfg, it should "be used in exported projects by placing this file in the same directory as the project binary", not in the user directory, see ProjectSetttings overriding.

To configure the stretch scale at runtime from a script, use the get_tree().root.content_scale_factor property (see Window.content_scale_factor).

@atomic-junky

Copy link
Copy Markdown
Collaborator Author

Okay, I'll try to find a solution.

@atomic-junky

Copy link
Copy Markdown
Collaborator Author

Does it work better now? I took over the Godot c++ code logic.

Btw I created a discussion (in the discussion tab) I don't know if it has any interest but it does exist

@RailKill RailKill left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Scaling works fine now, nice!

I tested it on lower resolutions, the window scales down to fit the resolution, and all controls in the side node panel are shown perfectly, everything works great. Compared to older version of Monologue, this scaling did not happen, so I can see the change clearly here. On 2560x1440 (that's the max I can go on my 27" display), the size of the UI remains the same as before, maintaining the experience. I looked at the code and presumably, since it worked for smaller resolutions on my display, it should scale up fine for bigger screens, following the if-else code logic. Great job! 👍

@atomic-junky

Copy link
Copy Markdown
Collaborator Author

Thank you, I'm glad it's working well!

@atomic-junky atomic-junky merged commit f33c43b into main Aug 21, 2024
@atomic-junky atomic-junky deleted the feature/window-scale branch August 21, 2024 09:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants