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

Fix max_formspec_size not taking gui_scaling into account #13493

Merged
merged 5 commits into from Jul 7, 2023

Conversation

grorp
Copy link
Member

@grorp grorp commented May 10, 2023

The max_formspec_size value returned by minetest.get_player_window_information() is currently incorrect if the player has set the gui_scaling setting to a value other than 1.

That's because prefer_imgsize in GUIFormSpecMenu::regenerateGui (link to the code) is scaled by gui_scaling, but ClientDynamicInfo::calculateMaxFSSize (link to the code) doesn't care. This PR just adds a division by gui_scaling to calculateMaxFSSize.

This bug cannot be worked around by mods since mods don't get the gui_scaling, but only the real_gui_scaling (gui_scaling * display density) of the player. Therefore, fixing this bug cannot not break any mods that already work around it.

To do

This PR is a Ready for Review.

How to test

Set gui_scaling to 0.5.
On the master branch, start Devtest and do /testfullscreenfs. The formspec won't be shown fullscreen.
Do the same on this PR. The formspec will be shown fullscreen.

@wsor4035 wsor4035 added the Bugfix 🐛 PRs that fix a bug label May 10, 2023
src/client/game.cpp Outdated Show resolved Hide resolved
@Zughy Zughy added the Action / change needed Code still needs changes (PR) / more information requested (Issues) label May 21, 2023
src/clientdynamicinfo.h Outdated Show resolved Hide resolved
@rubenwardy rubenwardy removed the Action / change needed Code still needs changes (PR) / more information requested (Issues) label May 27, 2023
Copy link
Member

@SmallJoker SmallJoker left a comment

Choose a reason for hiding this comment

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

The GUI looks as usual, thus LGTM.

@grorp grorp changed the title Make max_formspec_size take gui_scaling into account Fk max_formspec_size take gui_scaling into account Jun 19, 2023
@grorp grorp changed the title Fk max_formspec_size take gui_scaling into account Fix max_formspec_size not taking gui_scaling into account Jun 19, 2023
Copy link
Member

@Desour Desour left a comment

Choose a reason for hiding this comment

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

Tested, works.

One thing I noticed: When I'm in window mode, the fullscreenfs is still surrounded by a small boarder. (But that's prolly not relevant for this PR.)

src/clientdynamicinfo.h Outdated Show resolved Hide resolved
@rubenwardy
Copy link
Member

Hi, please wait for me to test this

@rubenwardy rubenwardy self-assigned this Jun 20, 2023
@SmallJoker SmallJoker added the Waiting (on dependency) Waiting on another PR or external circumstances (not for rebases/changes requested) label Jun 21, 2023
Copy link
Member

@rubenwardy rubenwardy left a comment

Choose a reason for hiding this comment

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

works well

@rubenwardy rubenwardy added @ Client / Audiovisuals Formspec and removed Waiting (on dependency) Waiting on another PR or external circumstances (not for rebases/changes requested) labels Jun 26, 2023
@grorp
Copy link
Member Author

grorp commented Jul 3, 2023

Bump. This has now also been approved by rubenwardy, so I think it should be ready for merging.

@SmallJoker SmallJoker merged commit 0218963 into minetest:master Jul 7, 2023
13 checks passed
Wuzzy2 pushed a commit to Wuzzy2/minetest that referenced this pull request Jul 31, 2023
@grorp grorp deleted the max_formspec_size-fix branch December 18, 2023 16:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants