HUD: support float values for text size property

[Zughy]

Fixes #16603

size.mp4

⬅️ master
➡️ PR

(code below)

It's my first time really messing with the networking part, as I wanted to learn a bit how the engine works. So... heh.

I didn't want to add another property (e.g. font_size), as I'd like to avoid any additional work to game devs. So I've converted size into a v2f. I've tested both with new server - old client (as in video) and vice versa.

To do

This PR is Ready for Review.

How to test

function animate(player, HUD_ID, old_val, decrement)
	local val = old_val + (decrement and -0.2 or 0.2)
	player:hud_change(HUD_ID, "size", {x=val})
	
	core.after(0.1, function()
		if not core.get_player_by_name(player:get_player_name()) then return end
		
		if val > 5 then
			decrement = true
		elseif val < 2 then
			decrement = false
		end
		animate(player, HUD_ID, val, decrement)
	end)
end

core.register_on_joinplayer(function(player)
	local HUD = {
	    type      = "text",
	    position  = { x = 1, y = 0.5 },
	    offset    = { x = -25 },
	    alignment = { x = -1 },
	    text      = "AAAAA",
	    size      = { x = 1.8 },
	    number    = "0xff0000",
	}
	local HUD_ID = player:hud_add(HUD)
	
	animate(player, HUD_ID, 1.8)
end)

[Wuzzy2]

Works for me. I also tested minimap. 👍

Please note the size attribute is used for a few other HUD types, like minimap, compass and statbar, where the values are used for pixel sizes or percentages. Pixel sizes are best stored as integers, so switching to float feels somewhat dirty. On the other hand, the float imprecision problems won't kick in until very large numbers.

So I can can get behind the case for adding a font_size instead, and reading from size for the text size only as a fallback (if font_size is undefined), so that existing mods still work. This solution might be the "cleanest".

But either way, I don't care that much as long it works; the problems with a float size are mostly of theoretical nature.

Also, I noticed this: if I use size={x=320.999, y=320.999} for minimap, this now looks different than size={x=320, y=320}. Not sure if this is intended or not.

Finally, should the documentation mention that floats are accepted now? (In general, I think the documentation would improve if data types (esp. float vs int) and size bounds (min/max value) are mentioned everywhere, but this is off-topic …)

[Zughy]

if I use size={x=320.999, y=320.999} for minimap, this now looks different than size={x=320, y=320}

I wanted size (as a float) to be used only for text. And since no field type is specified..

I can can get behind the case for adding a font_size instead, and reading from size for the text size only as a fallback

I didn't want to add a new field. However, if core devs think this is the way to go, just tell me and I'll go with this approach instead

[Zughy]

Actually, this can be considered "UI improvements". Relabelled, if core devs don't agree, feel free to close it as not adhering to the roadmap.

[appgurueu]

I was initially a bit skeptical of this (I would have preferred a separate font_size). I've now approached this again with a fresh mind and I figure it's okay, for the following reasons:

• We do currently use size for font size, so the natural way to extend that is by just allowing floats.
• Adding font_size would introduce redundancy and/or require a deprecation of size for font size, creating work for modders (as Zughy noted).
  • This will be slightly dirty either way. It's not clear to me that font_size would be meaningfully cleaner.
• It also introduces further HUD element non-uniformity (custom logic for a specific element type).
• "Upgrading" s32 to f32 is fine; we can pretend that f32 is a supertype of s32, current code keeps working. Nobody needs more than 8388608 pixels.
• As far as the Lua API user is concerned, not much changes either. I believe our current v2s32 reading logic silently casts already. Now the cast just moves to a different place deeper inside the code (network level and/or HUD drawing).
• From a Lua perspective, where everything is f64, f32 is a more natural type even.

If we want to be extra careful, the other HUD elements that currently use size and expect it to be integer can get explicit casts to v2s32 in their drawing logic.

The review mostly consists of suggestions to use the appropriate vector2d<T>::from cast helpers.

[Zughy]

Rebased and review addressed

[appgurueu]

Took the liberty to commit your test code as a unit test under your name. Works as expected in singleplayer 👍

I believe the protocol version check needs to be updated to 52 however, as 51 is now used by the released 5.15.0. The protocol version then also needs to be bumped in networkprotocol.cpp, with an appropriate note in the comment.

One small detail I missed initially, could you explicitly state in the docs the client version requirement for this feature (5.16.0+), and note that older clients only support integer sizes so modders can adjust accordingly?

[SmallJoker]

The protocol version then also needs to be bumped in

@appgurueu I would suggest to prepare the 5.16.0 notice and not bump yet - especially in regard to another feature that would benefit from a bump: #17020. The PR can be tested manually by changing the supported proto version locally.

[Zughy]

for reviewers: this avoids a rebase conflict with #17020 , as it sounds like both this PR and the other are being merged within 5.16

[SmallJoker]

LGTM. Thank you for the adjustments.

[appgurueu]

sure, as long as the state in master is consistent when this is merged it's good

[SmallJoker]

This branch cannot be rebased due to conflicts

What conflicts? Anyway. I assume this needs the label ...

[Zughy]

@SmallJoker I don't see it, it tells me there are no conflicts