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

Add minimum / maximum window size project setting #80337

Closed
wants to merge 1 commit into from

Conversation

rptfrg
Copy link
Contributor

@rptfrg rptfrg commented Aug 6, 2023

This adds Window Minimum Width, Window Minimum Height, Window Maximum Width, Window Maximum Height to project settings. (under Advanced Settings)

This is my first "proper" pull request, so I apologize if there is something wrong with logic or the style of code.

Notes:

  1. Setting maximum w/h to be smaller than minimum w/h causes maximum w/h to be ignored, this happens when you set it up this way through script as well.

Closes: #31303

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 on Linux/Fedora KDE X11 (rebased on top of master 16a9356):

  • Maximum size project settings work as expected, even if the project's base window size or size override is greater than the maximum size (in this case, the size is clamped automatically, which is expected).
  • However, minimum size project settings don't appear to override the default of (64, 64) when changed.

Testing project: test_pr_80337.zip

editor/editor_run.cpp Outdated Show resolved Hide resolved
editor/editor_run.cpp Outdated Show resolved Hide resolved
@rptfrg
Copy link
Contributor Author

rptfrg commented Aug 7, 2023

@Calinou Strange, it seems to be working for me.

Screencast.from.2023-08-07.13-59-49.webm

I've noticed strange behavior - sometimes you need to restart the game several times for the newly updated project settings to take effect, maybe you've come across this while testing? This can happen with any setting.

@Calinou
Copy link
Member

Calinou commented Aug 7, 2023

I've noticed strange behavior - sometimes you need to restart the game several times for the newly updated project settings to take effect, maybe you've come across this while testing? This can happen with any setting.

Yes; I've done that 3 times to ensure the updated project settings are read by the running project.

@rptfrg
Copy link
Contributor Author

rptfrg commented Aug 7, 2023

I have updated the code, here's the complete logic (with width taken as an example):

  1. First, we set desired_width to viewport_width.
  2. If window_width_override does not equal 0, we set desired_width to window_width_override.
  3. If window_maximum_width does not equal 0, we clamp desired_width between window_minimum_width and window_maximum_width.
  4. Finally, with pick a maximum value between desired_width and window_minimum_width to make sure that minimum width always takes precedence over maximum width. (that's how it works if you set min/max width with GDScript)

I don't know what could be causing the issue Calinou was having.


I just realized I probably don't need to use CLAMP, I'll change that part of code.

@Lexpeartha
Copy link

Does this PR require more help/testing to move it forward? Would be great to have this QoL feature in 4.3

@Calinou
Copy link
Member

Calinou commented Jan 1, 2024

Does this PR require more help/testing to move it forward? Would be great to have this QoL feature in 4.3

I've tested the PR again and minimum size works now. However, it turns out it was never an issue with this PR. This was because I enforced a minimum size override in KWin's application-specific overrides, and it affected all Godot projects:

image

The only remaining concern I have is that behavior is unexpected if the minimum size is greater than the maximum size. In this case, you can freely resize the window above the minimum size, but I'd expect to be unable to resize the window at all in this case (with the window size being equal to the maximum size). It may also be worth printing a warning in this situation.

@Lexpeartha
Copy link

I would more than willing to help this get through if @reptofrog is not available.

@rptfrg
Copy link
Contributor Author

rptfrg commented Jan 3, 2024

@Lexpeartha

I would appreciate that if you could finish it up as I am a little busy at the moment. 🙂

@Lexpeartha
Copy link

Would it be fine to create a new pull request to replace this existing one?

@Calinou
Copy link
Member

Calinou commented Jan 12, 2024

Would it be fine to create a new pull request to replace this existing one?

Yes, feel free to open a pull request 🙂

Please rebase against the latest master branch to resolve merge conflicts too.

@Calinou
Copy link
Member

Calinou commented Jan 15, 2024

@Calinou Calinou closed this Jan 15, 2024
@Calinou Calinou removed this from the 4.x milestone Jan 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants