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

Refactor builtin HUD #14346

Merged
merged 26 commits into from Apr 10, 2024
Merged

Refactor builtin HUD #14346

merged 26 commits into from Apr 10, 2024

Conversation

cx384
Copy link
Contributor

@cx384 cx384 commented Feb 4, 2024

Goal of the PR

Does this relate to a goal in the roadmap?

2.2 Internal code refactoring

To do

This PR is Ready for Review.

How to test

  • Start devtest, jump into water to test the breathbar, and hurt yourself to test the healthbar.
  • Read the code notice the refactoring.
  • Test hud_replace_builtin
minetest.hud_replace_builtin("breath", {
	type = "statbar",
	position = {x = 0.5, y = 0.5},
	text = "bubble.png",
	text2 = "bubble_gone.png",
	number = core.PLAYER_MAX_BREATH_DEFAULT * 2,
	item = core.PLAYER_MAX_BREATH_DEFAULT * 2,
	direction = 0,
	size = {x = 24, y = 24},
	offset = {x = 25, y= -(48 + 24 + 16)}
})
  • Test flags
minetest.register_chatcommand("hudtoggleflag", {
	description = "Toggles a hud flag.",
	params = "<flag>",
	func = function(name, params)
		local player = minetest.get_player_by_name(name)
		if not player then
			return false, "No player."
		end

		local flags = player:hud_get_flags()
		if flags[params] == nil then
			return false, "Unknown hud flag."
		end

		flags[params] = not flags[params]
		player:hud_set_flags(flags)
		return true, "Flag \"".. params .."\" set to ".. tostring(flags[params]) .. "."
	end
})

/hudtoggleflag breathbar /hudtoggleflag healthbar

  • Also test other builtin elements in case new ones get added by other PRs.

@cx384
Copy link
Contributor Author

cx384 commented Feb 8, 2024

Rebased.
This PR now also fixes a warning you get because hud_elem_type has been renamed to type.
I caused this with #14071, because the PR was made before the renaming, and I missed to change it. Sorry about this.

@cx384
Copy link
Contributor Author

cx384 commented Mar 1, 2024

Rebased again.
(The hud_elem_type/type problem, has already been fixed in master now.)

builtin/game/hud.lua Outdated Show resolved Hide resolved
builtin/game/hud.lua Outdated Show resolved Hide resolved
@sfan5 sfan5 added Maintenance Tasks to keep the codebase and related parts in order, including architectural improvements @ Client / Audiovisuals @ Builtin and removed @ Client rendering @ Script API labels Mar 19, 2024
@cx384
Copy link
Contributor Author

cx384 commented Mar 19, 2024

Thanks for reviewing it.
With the refactoring, I didn't want to change any behavior (even if it is not documented in the API).
However, I applied your assert suggestion now, so minetest.hud_replace_builtin behaves a little differently, but I guess this is good.

I also rebased to make sure that it still works, since it is almost 2 months old.

builtin/game/hud.lua Outdated Show resolved Hide resolved
builtin/game/hud.lua Outdated Show resolved Hide resolved
builtin/game/hud.lua Show resolved Hide resolved
builtin/game/hud.lua Outdated Show resolved Hide resolved
builtin/game/hud.lua Outdated Show resolved Hide resolved
builtin/game/hud.lua Show resolved Hide resolved
builtin/game/hud.lua Outdated Show resolved Hide resolved
builtin/game/hud.lua Show resolved Hide resolved
builtin/game/hud.lua Outdated Show resolved Hide resolved
builtin/game/hud.lua Outdated Show resolved Hide resolved
@grorp grorp added the Action / change needed Code still needs changes (PR) / more information requested (Issues) label Mar 29, 2024
builtin/game/hud.lua Outdated Show resolved Hide resolved
@grorp grorp self-assigned this Mar 30, 2024
@grorp grorp self-requested a review March 30, 2024 23:08
@grorp
Copy link
Member

grorp commented Mar 31, 2024

Hey @rollerozxa, can you do a zipgrep for hud_replace_builtin please?

@rollerozxa
Copy link
Member

sure, here: https://content.minetest.net/zipgrep/cf8a1325-8075-44f6-a32c-f10e6ad93f83/

builtin/game/statbars.lua Show resolved Hide resolved
builtin/game/hud.lua Outdated Show resolved Hide resolved
builtin/game/hud.lua Outdated Show resolved Hide resolved
builtin/game/hud.lua Show resolved Hide resolved
builtin/game/hud.lua Show resolved Hide resolved
builtin/game/hud.lua Outdated Show resolved Hide resolved
builtin/game/hud.lua Outdated Show resolved Hide resolved
builtin/game/hud.lua Outdated Show resolved Hide resolved
builtin/game/hud.lua Outdated Show resolved Hide resolved
@grorp grorp added the Action / change needed Code still needs changes (PR) / more information requested (Issues) label Mar 31, 2024
@grorp
Copy link
Member

grorp commented Mar 31, 2024

(The statusbars behave the same as in the old version, the only difference is that hud_replace_builtin does not default definition.item anymore, but this is not documented inlua_api.md so it should be fine. However, I can re-add it if someone thinks it is needed.)

Looking through the zipgrep kindly provided by rollerozxa, I can see at least one package relying on this default value (https://content.minetest.net/packages/epCode/what_were_you_expecting/). You have to re-add it for backwards compatibility (and it would be good to document and/or deprecate it).

cx384 and others added 5 commits March 31, 2024 23:02
Fix grammar

Co-authored-by: grorp <gregor.parzefall@posteo.de>
Fix grammar2

Co-authored-by: grorp <gregor.parzefall@posteo.de>
Co-authored-by: grorp <gregor.parzefall@posteo.de>
@cx384
Copy link
Contributor Author

cx384 commented Mar 31, 2024

I resolved everything again, and re-added the undocumented elem_def.item behavior.

Also, I fixed the duplicate breatbbar bug.

@Zughy Zughy removed the Action / change needed Code still needs changes (PR) / more information requested (Issues) label Apr 1, 2024
@grorp grorp self-requested a review April 1, 2024 13:20
…ckwards compat

scaleToHudMax uses elem_def.item, so it must be set first to avoid a Lua error with mods
that rely on the old behavior

Remove "deprecated undocumented behavior" comment since it doesn't make much sense -
how are modders supposed to know that something is deprecated if it's not documented?
…e_guidelines

Specifically:
- "Functions and variables should be named in lowercase_underscore_style"
- "When breaking around a binary operator you should break after the operator."
@grorp
Copy link
Member

grorp commented Apr 3, 2024

For reference: cx384#2

@cx384
Copy link
Contributor Author

cx384 commented Apr 6, 2024

For reference: cx384#2

Seams good, merged. Sorry for being a little late.
(Maybe I should have refactored more and not take too many things from the old code.)

builtin/game/hud.lua Outdated Show resolved Hide resolved
Copy link
Member

@grorp grorp left a comment

Choose a reason for hiding this comment

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

This PR looks OK to me now.

@sfan5 since there have been a lot of changes, you might want to have a look at this again.

@sfan5
Copy link
Member

sfan5 commented Apr 8, 2024

@sfan5 since there have been a lot of changes, you might want to have a look at this again.

I trust your judgement.

@grorp grorp merged commit 8a5e49c into minetest:master Apr 10, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@ Builtin @ Client / Audiovisuals Maintenance Tasks to keep the codebase and related parts in order, including architectural improvements >= Two approvals ✅ ✅
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants