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

GUITable: Scale images with display density / row height #14709

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

grorp
Copy link
Member

@grorp grorp commented May 26, 2024

Fixes #3403, opened in 2015

5.8.1
screenshot on 5.8.1

this PR
screenshot on this PR

Offtopic complaints

Something you can also see very well on these screenshots is that the mainmenu is way too small by default on Android, at least on my device. With proper scaling, the serverlist would be much more readable. This PR with gui_scaling = 1.3:

screenshot on this PR, but with better scaling

While you can set gui_scaling manually to get reasonable scaling (except for the scrollbars, which are too big now), you shouldn't have to do that. Minetest should use a reasonable fraction of the screen by default.

To do

This PR is a Ready for Review.

If you find a case where this PR makes an existing formspec look worse, please tell me. However, this isn't very likely to happen anymore with the latest commit. Zipgrep: https://web.archive.org/web/20240530091120/https://content.minetest.net/zipgrep/62090fd0-3f2e-4806-b04c-53cd5e8a20f1/

How to test

Look at the Devtest test formspec, "Table" tab. Verify that what you see matches what the code requests.

Look at the serverlist on a device with a high display density.

@grorp grorp added @ Client / Audiovisuals WIP The PR is still being worked on by its author and not ready yet. Bugfix 🐛 PRs that fix a bug Formspec UI/UX labels May 26, 2024
src/gui/guiTable.cpp Outdated Show resolved Hide resolved
doc/lua_api.md Outdated Show resolved Hide resolved
@grorp grorp removed the WIP The PR is still being worked on by its author and not ready yet. label May 28, 2024
@grorp
Copy link
Member Author

grorp commented May 30, 2024

Unfortunately we can't scale images up to match the row height without accidentally affecting some formspecs. If a formspec uses blank.png (from textures/base/pack, 1x1 pixels) as a placeholder for "no image", that placeholder is scaled up to rowheight x rowheight, potentially increasing the column width.

Repixture awards formspec before
screenshot: repixture awards before

Repixture awards formspec after
screenshot: repixture awards after

To avoid this, I've changed this PR's approach to scale by display density instead. Row height is now only used to scale down images which are too tall.

@grorp grorp changed the title GUITable: Scale images with row height GUITable: Scale images with display density / row height May 30, 2024
@grorp
Copy link
Member Author

grorp commented Jun 21, 2024

This PR has undergone more transformation so that it is a pure bugfix now.

Example case on desktop:
Fantasy Commodity Markets
SharpNet Photo Realism 64px)

before
screenshot before

after
screenshot after

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.

Tested with math.randomseed(1337). Looks good.

comparison

src/gui/guiTable.cpp Outdated Show resolved Hide resolved
games/devtest/mods/testformspec/formspec.lua Show resolved Hide resolved
@grorp grorp requested a review from SmallJoker June 23, 2024 12:21
@grorp
Copy link
Member Author

grorp commented Jun 24, 2024

Unfortunately I had to change the approach to display density scaling used in this PR again. With the previous approach, the icons were too big proportionally to the row height on my Android device:

previous screenshot
icons too big

Because the row height is derived from the font size and the default font size is lower (14 instead of 16) on Android, the images were more affected by the higher display density than the row height was.

To fix this, I've made image scaling respect font size as well.

new screenshot
icons correctly sized

@grorp grorp requested a review from SmallJoker June 24, 2024 19:00
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.

Images in tablecolumn are not scaled
2 participants