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 minetest.get_player_window_information() #12367

Merged
merged 14 commits into from
Feb 27, 2023

Conversation

rubenwardy
Copy link
Member

@rubenwardy rubenwardy commented May 21, 2022

\Adds a new packet to send dynamic client info to the server, and minetest.get_player_window_information()

Fixes #10632

* `minetest.get_player_window_information(player_name)`:

      -- Will only be present if the client sent this information (requires v5.7+)
      --
      -- Note that none of these things are constant, they are likely to change during a client
      -- connection as the player resizes the window and moves it between monitors
      --
      -- real_gui_scaling and real_hud_scaling can be used instead of DPI.
      -- OSes don't necessarily give the physical DPI, as they may allow user configuration.
      -- real_*_scaling is just OS DPI / 96 but with another level of user configuration.
      {
          -- Current size of the in-game render target.
          --
          -- This is usually the window size, but may be smaller in certain situations,
          -- such as side-by-side mode.
          size = {
              x = 1308,
              y = 577,
          },

          -- Estimated maximum formspec size before Minetest will start shrinking the
          -- formspec to fit. For a fullscreen formspec, use a size 10-20% larger than
          -- this and `padding[-0.01,-0.01]`.
          max_formspec_size = {
              x = 20,
              y = 11.25
          },

          -- GUI Scaling multiplier
          -- Equal to the setting `gui_scaling` multiplied by `dpi / 96`
          real_gui_scaling = 1,

          -- HUD Scaling multiplier
          -- Equal to the setting `hud_scaling` multiplied by `dpi / 96`
          real_hud_scaling = 1,
      }

To do

This PR is ready for review

  • documentation
  • Testing
  • Return display = nil when the client doesn't send info
  • DPI is always an integer on Windows. Is it always an integer on other platforms?
  • should this be a new API function?
  • debounce sending
  • What other display information is needed?
  • Resist fingerprinting
  • Move to player ObjectRef?
  • Add note that real_gui_scaling/real_hud_scaling can be used instead of DPI

How to test

  • See /testfullscreenfs
  • Test in sidebyside 3d mode

@rubenwardy rubenwardy added @ Script API @ Client / Audiovisuals WIP The PR is still being worked on by its author and not ready yet. Concept approved Approved by a core dev: PRs welcomed! labels May 21, 2022
@rubenwardy rubenwardy force-pushed the display_info_api branch 6 times, most recently from 492499c to 95557f8 Compare May 22, 2022 00:57
@rubenwardy rubenwardy marked this pull request as ready for review May 22, 2022 01:03
@rubenwardy rubenwardy removed the WIP The PR is still being worked on by its author and not ready yet. label May 22, 2022
@MisterE123
Copy link
Contributor

MisterE123 commented May 22, 2022

wow, what an amazingly great and awsome thing.

@sfan5 sfan5 added the WIP The PR is still being worked on by its author and not ready yet. label May 22, 2022
@sfan5
Copy link
Member

sfan5 commented May 22, 2022

    dpi = 96, -- Dots per inch
    gui_scaling = 1, -- gui_scaling setting
    hud_scaling = 1, -- hud_scaling setting

Any reason to separate these?
Having real_hud_scaling & real_gui_scaling would convey the necessary info without mods needing to do math every time.
(real_hud_scaling := hud_scaling * dpi / 96 in case that isn't obvious)

    screen_size = { -- Current window size, not including any decorations (window bar, etc)
        x = 1308,
        y = 577,
    },
  1. This can potentially be minimized into an approximate aspect ratio plus height
  2. What's the usecase here? Sizing of GUI elements?
  3. The description should say "current size of the in-game area" or similar to be future-proof.

should this be a new API function?

Definitely, "you call the API and get 20 values of which you need only two" is already an issue with get_player_information. We shouldn't make it worse.

DPI is always an integer on Windows. Is it always an integer on other platforms?

Considering the above formula the fractional part of DPI will change the scale of things by less than ~0.01, so rounding to integer is safe to do.

What other display information is needed?

Current enablement state of the touchscreen GUI.

@MisterE123
Copy link
Contributor

MisterE123 commented May 22, 2022

@sfan5, yes the usecase is sizing elements. with this information, it is possible to make fullscreen formspecs! I have made fullscreen formspecs in the mainmenu using this information (which passes it) , and it works pretty well.

also, its possible to just make sure that elements fit well on the screen.

@rubenwardy
Copy link
Member Author

rubenwardy commented May 22, 2022

    dpi = 96, -- Dots per inch
    gui_scaling = 1, -- gui_scaling setting
    hud_scaling = 1, -- hud_scaling setting

Any reason to separate these? Having real_hud_scaling & real_gui_scaling would convey the necessary info without mods needing to do math every time. (real_hud_scaling := hud_scaling * dpi / 96 in case that isn't obvious)

Looking into it, yeah it would make sense to combine these. Raw DPI is only useful if you want to know the physical size of the screen, but this dpi value can be overridden by screen_dpi anyway

  1. This can potentially be minimized into an approximate aspect ratio plus height

This seems a bit derived to me, It's not minimized, it's the same number of numbers just a derived value rather than the raw values

  1. What's the usecase here? Sizing of GUI elements?

The use case is knowing how much space is available so that you can 1) make a formspec fullsize 2) make the best use of space

If a modder wants to make a formspec full size, then they just need the ratio to calculate a large size[] with the same ratio as the window. Minetest will scale down formspec windows to fit the game window. But having the additional information allows for simplifying or adding more to the UI when there is more space

3. The description should say "current size of the in-game area" or similar to be future-proof.

Makes sense, side-by-side mode means that only half of the window's width is used

DPI is always an integer on Windows. Is it always an integer on other platforms?

Considering the above formula the fractional part of DPI will change the scale of things by less than ~0.01, so rounding to integer is safe to do.

guiFormspecMenu uses it as a double, which is a bit overkill. Given that we've decided to combine this into the _scaling settings, there's no need for a change here

What other display information is needed?

Current enablement state of the touchscreen GUI.

See #12264. This should be sent as part of the ClientDynamicInfo packet, but I don't know what the Lua API should be

@rubenwardy
Copy link
Member Author

rubenwardy commented May 22, 2022

A value that may be useful would be something like this:

-- Max formspec size before Minetest starts scaling down, assuming 
-- `fixed_size` is true in `size[]` and`padding[0,0]`.
--
-- This can be used to make fullscreen formspecs by using a padding
-- of `padding[-0.01,-0.01]` and a small inset `0.1` between the edge
-- of the formspec and the elements.
max_formspec_size = {
    x = 10.3,
    y = 5.4,
}

@rubenwardy
Copy link
Member Author

Added a mod to devtest to test fullscreen formspecs

@rubenwardy rubenwardy changed the title Add display info to minetest.get_player_information() Add window info to minetest.get_player_information() May 22, 2022
@rubenwardy
Copy link
Member Author

rubenwardy commented May 22, 2022

A value that may be useful would be something like this:

-- Max formspec size before Minetest starts scaling down, assuming 
-- `fixed_size` is true in `size[]` and`padding[0,0]`.
--
-- This can be used to make fullscreen formspecs by using a padding
-- of `padding[-0.01,-0.01]` and a small inset `0.1` between the edge
-- of the formspec and the elements.
max_formspec_size = {
    x = 10.3,
    y = 5.4,
}

This value is calculated by:

local function calculate_max_fs_size(name)
	local window = minetest.get_player_window_information(name)
	local slot_size = 0.5555 * 96 * window.real_gui_scaling
	return {
		x = window.size.x / slot_size,
		y = window.size.y / slot_size,
	}
end

This 0.5555 is taken from guiFormspecMenu.cpp: https://github.com/minetest/minetest/blob/master/src/gui/guiFormSpecMenu.cpp#L3216-L3223

This is only the case when true is provided to size[x,y;true]

@rubenwardy rubenwardy changed the title Add window info to minetest.get_player_information() Add minetest.get_player_window_information() May 22, 2022
@appgurueu
Copy link
Contributor

appgurueu commented May 29, 2022

For the API it'd be useful if this was event-driven rather than a getter (or probably both) - as a modder I prefer not to poll.

@rubenwardy
Copy link
Member Author

That can come later

@Zughy
Copy link
Member

Zughy commented Jun 12, 2022

@rubenwardy rebase needed

@Zughy Zughy added the Rebase needed The PR needs to be rebased by its author. label Jun 12, 2022
@rubenwardy rubenwardy removed the Rebase needed The PR needs to be rebased by its author. label Jun 12, 2022
@rubenwardy

This comment was marked as outdated.

@sfan5 sfan5 self-requested a review January 8, 2023 18:39
doc/lua_api.txt Show resolved Hide resolved
doc/menu_lua_api.txt Outdated Show resolved Hide resolved
src/client/client.cpp Outdated Show resolved Hide resolved
src/clientdynamicinfo.h Show resolved Hide resolved
@Zughy Zughy added the Action / change needed Code still needs changes (PR) / more information requested (Issues) label Feb 14, 2023
@rubenwardy rubenwardy removed the Action / change needed Code still needs changes (PR) / more information requested (Issues) label Feb 18, 2023
Copy link
Member

@sfan5 sfan5 left a comment

Choose a reason for hiding this comment

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

Works.
my test code:

local thing = {}

minetest.register_on_joinplayer(function(player)
	local name = player:get_player_name()
	thing[name] = {
		hud = player:hud_add({
			hud_elem_type = "text",
			position = {x = 0.22, y = 0.22},
			alignment = {x = 1, y = 0},
			scale = {x = 2, y = 2},
			number = 0xFFFFFF,
			style = 4,
		}),
		text = ""
	}
end)

minetest.register_on_leaveplayer(function(player)
	thing[player:get_player_name()] = nil
end)

minetest.register_globalstep(function(dtime)
	for _, player in ipairs(minetest.get_connected_players()) do
		local name = player:get_player_name()
		local c = thing[name]
		if c then
			local s = dump(minetest.get_player_window_information(name))
			if s ~= c.text then
				player:hud_change(c.hud, "text", s)
				c.text = s
			end
		end
	end
end)

@rubenwardy rubenwardy merged commit 39f4d26 into minetest:master Feb 27, 2023
@rubenwardy rubenwardy deleted the display_info_api branch February 27, 2023 22:58
LizzyFleckenstein03 pushed a commit to LizzyFleckenstein03/minetest that referenced this pull request Mar 7, 2023
appgurueu pushed a commit to appgurueu/minetest that referenced this pull request May 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@ Client / Audiovisuals Concept approved Approved by a core dev: PRs welcomed! One approval ✅ ◻️ Roadmap The change matches an item on the current roadmap. @ Script API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Retrieve DPI from get_player_information