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

Move hard coded minimap to builtin #14071

Merged
merged 16 commits into from Feb 7, 2024
Merged

Move hard coded minimap to builtin #14071

merged 16 commits into from Feb 7, 2024

Conversation

cx384
Copy link
Contributor

@cx384 cx384 commented Dec 5, 2023

Goal of the PR

How does the PR work?

  • Currently, there exists a hard coded minimap that is not handled by a Lua HUD element. However, the HUD element type "minimap" already exists.
  • This PR moves this minimap into the builtin Lua code, such that it can be accessed and modified with the lua API.
  • It behaves similar to the previous minimap, but there are some differences:
    • Automatic resizing with respect to the client window size does not happen automatically anymore. (Maybe it would be good to have such a feature for all hud elements.)
    • The minimap HUD flag does now only change the builtin minimap. (Before this the flag temporary disabled all Lua element minimaps, but you could turn them back on by pressing the minimap key.)
  • When Treat built-in hotbar, nametags, minimap, crosshair like Lua HUD elements #9270 gets completely resolved the builtin statusbars.lua HUD code should be generalized, therefore this PR doesn't do it.
  • The compatibility to older versions is not completely contained, since multiple builtin minimaps may be shown, but it is not a breaking change, and I don't think it is a big problem.
    It is now backwards compatible in both directions (server/client).

Unittest info

I completely removed testFlagSetters since showMinimap was the only function in the GameUI class which could change a Hud flag. (The flag is now only managed in builtin like healthbar or breathbar)

To do

This PR is Ready for Review

  • Fix the automatic resizing, if it is important. This may need a new feature for all HUD elements.
    Automatic resizing is bad since it would be the only builtin element that does this.
    Now it has a fixed size like all other HUD elements. The size is similar to the previous one, only people which use a window height far away from 1080 pixels may notice a difference.
  • The minimap HUD flag should probably only enable/disable the builtin minimap like it is the case with the builtin statusbars. (Currently, the flag only partly influence other minimaps, since they can be reactivated by the client.)
  • Make minetest.hud_replace_builtin(name, hud_definition) work with the minimap.

How to test

Use this and press the mimimap key:

-- Print HUD elements
minetest.after(2, function()
	local player = minetest.get_player_by_name("singleplayer")
	for id = 0, 10 do
		local hud = player:hud_get(id)
		if hud and hud.type == "minimap" then
			print(dump(hud))
		end
	end
end)

-- A test HUD element that is not builtin
minetest.register_on_joinplayer(function(player)
	player:hud_add({
		hud_elem_type = "minimap",
		position = {x=0, y=0},
		alignment = {x=1, y=1},
		offset = {x=10, y=10},
		size = {x = 100 , y = 100}
	})
end)

-- Change minimap HUD flag every 3 seconds
local timed = 0
minetest.register_globalstep(function(dtime)
	local name = "singleplayer"
	timed = timed + dtime
	if timed > 3 then
		timed = 0
		local player = minetest.get_player_by_name(name)
		if player:hud_get_flags().minimap then
			minetest.chat_send_player(name, "off")
			player:hud_set_flags({minimap = false})
		else
			minetest.chat_send_player(name, "on")
			player:hud_set_flags({minimap = true})
		end
	end
end)

-- Override the builtin minimap (changed positions)
minetest.hud_replace_builtin("minimap", {
	hud_elem_type = "minimap",
	position = {x=0.5, y=0.5},
	alignment = {x=-1, y=1},
	offset = {x=-10, y=10},
	size = {x = 256 , y = 256}
})

(If you are unsure how the HUD flag should behave execute it also on a previous version.)

Maybe also test if the enable_minimap setting still works.

/set enable_minimap true
/set enable_minimap false

@cx384 cx384 changed the title move minimap to builtin Move hard coded minimap to builtin Dec 5, 2023
@cx384 cx384 marked this pull request as draft December 5, 2023 14:32
@grorp grorp added @ Server / Client / Env. @ Script API Maintenance Tasks to keep the codebase and related parts in order, including architectural improvements labels Dec 5, 2023
@cx384 cx384 marked this pull request as ready for review December 18, 2023 13:39
@cx384
Copy link
Contributor Author

cx384 commented Dec 18, 2023

It's rebased and ready for review, so it would be nice if someone could review it.

@sfan5 sfan5 added the Rebase needed The PR needs to be rebased by its author. label Jan 12, 2024
@cx384
Copy link
Contributor Author

cx384 commented Jan 15, 2024

Rebased again.

@Zughy Zughy added Roadmap The change matches an item on the current roadmap. and removed Rebase needed The PR needs to be rebased by its author. labels Jan 15, 2024
@cx384
Copy link
Contributor Author

cx384 commented Jan 15, 2024

This PR probably also solves #8431 if it wasn't already solved.

@appgurueu appgurueu self-requested a review January 16, 2024 22:57
@appgurueu appgurueu self-assigned this Jan 21, 2024
@appgurueu appgurueu added the Bugfix 🐛 PRs that fix a bug label Feb 1, 2024
@Zughy Zughy added the Feature ✨ PRs that add or enhance a feature label Feb 1, 2024
builtin/game/minimap.lua Outdated Show resolved Hide resolved
builtin/game/minimap.lua Outdated Show resolved Hide resolved
builtin/game/minimap.lua Outdated Show resolved Hide resolved
Copy link
Contributor

@appgurueu appgurueu 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. I have a few questions, but the code looks fine as-is. Thanks for the PR.

Fix the automatic resizing, if it is important. This may need a new feature for all HUD elements. Automatic resizing is bad since it would be the only builtin element that does this.

I am fine with your rationale for not fixing this. (Additionally, if needed, modders should be able to implement automatic resizing by querying screen size.)

You could also add a feature for screen-size relative size (similar to how images support percentages via negative values).

@appgurueu appgurueu added One approval ✅ ◻️ and removed Bugfix 🐛 PRs that fix a bug labels Feb 1, 2024
@cx384
Copy link
Contributor Author

cx384 commented Feb 1, 2024

Thanks for the review and testing.
I will soon make a PR to refactor the builtin HUD code, to generalize builtin/game/statbars.lua, such that it can not only handle statusbars.
Your questions for builtin/game/minimap.lua will probably be resolved when I'm done with this, and moved the minimap and statusbars code into one file.
(I also need this for my other HUD PRs.)

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.

Backwards compatibility is missing in both directions. Both cases must be fixed.

5.8.0 on the client, this PR on the server: minimap shown twice

screenshot of the minimap being shown twice

The compatibility to older versions is not completely contained, since multiple builtin minimaps may be shown, but it is not a breaking change, and I don't think it is a big problem.

Having two mipmaps on top of each other, each with a different size, is ugly enough to be a concern. It's also easy enough to fix by checking the protocol version.

this PR on the client, 5.8.0 on the server: no minimap at all

always shows "Minimap currently disabled by game or mod"

@grorp grorp added the Action / change needed Code still needs changes (PR) / more information requested (Issues) label Feb 1, 2024
@cx384
Copy link
Contributor Author

cx384 commented Feb 1, 2024

It is now backwards compatible.
I tested it in both directions, so I hope it behaves as intended.

(At first, I thought it wouldn't be a big problem for the builtin minimap to behave incorrectly when using different versions, since it is a rather dispensable feature.)

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.

Regardless of any potential future refactoring, you can reduce code duplication a lot by moving the minimap code to statbars.lua. (And in that case, it would make sense to rename statbars.lua to hud.lua or similar.)

grorp@59b9323

src/client/game.cpp Outdated Show resolved Hide resolved
@cx384 cx384 mentioned this pull request Feb 4, 2024
@cx384
Copy link
Contributor Author

cx384 commented Feb 4, 2024

Ok, moved it into statbars.lua for now, and made a PR to refactor the builtin HUD code. #14346

Edit:
I think the Action / change needed label can be removed, since I resolved every complaint.
It would be nice if this PR would get another review such that it can be merged, but maybe it would be better to first handle the refactoring PR.

@grorp grorp self-assigned this Feb 6, 2024
@grorp grorp removed the Action / change needed Code still needs changes (PR) / more information requested (Issues) label Feb 6, 2024
builtin/game/statbars.lua Show resolved Hide resolved
src/client/hud.cpp Show resolved Hide resolved
@grorp grorp added the Action / change needed Code still needs changes (PR) / more information requested (Issues) label Feb 6, 2024
src/network/networkprotocol.h Outdated Show resolved Hide resolved
@grorp grorp added >= Two approvals ✅ ✅ and removed One approval ✅ ◻️ Action / change needed Code still needs changes (PR) / more information requested (Issues) labels Feb 7, 2024
@grorp grorp merged commit adaa4cc into minetest:master Feb 7, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature ✨ PRs that add or enhance a feature Maintenance Tasks to keep the codebase and related parts in order, including architectural improvements Roadmap The change matches an item on the current roadmap. @ Script API @ Server / Client / Env. >= Two approvals ✅ ✅
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Temp-disabling player's minimap shouldn't toggle it "completely off" client-side
5 participants