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 function to get all hud elements #14042

Merged
merged 8 commits into from
Jan 14, 2024
Merged

Conversation

cx384
Copy link
Contributor

@cx384 cx384 commented Nov 26, 2023

  • Currently it is not possible to get all HUD element IDs. If you would try to enumerate them, it is impossible to know when to stop. You can't just stop after an ID is empty, since they are not consecutive if an element gets removed.
  • This PR adds a player:hud_get_all() function which returns a table indexed by all present HUD ID's and containing the corresponding HUD element definition structures.
  • Like the get_lists() function for inventories, it allows bulk access and makes it easy to iterate through all HUD elements.
  • This PR adds the new function to the Lua API as well as to the client Lua API.

Does it resolve any reported issue?

No.

Does this relate to a goal in the roadmap?

No.

To do

Ready for Review.

How to test

  • Start devtest
  • Execute the /hudprint chat command
  • To test some more, change the HUD with for example:
local def = {
   hud_elem_type = "text",
   name = "test",
   position = {x = 0.5, y = 0.5},
   text = "test"
}
player:hud_add(def)
local rid = player:hud_add(def)
player:hud_add(def)
player:hud_remove(rid)

and use /hudprint again or print(dump(player:hud_get_all()))

To check whether the ids are correct

for id, elem in pairs(player:hud_get_all()) do
   if dump(elem) ~= dump(player:hud_get(id)) then
   	print("Wrong ids")
   end
end

@wsor4035 wsor4035 added @ Script API Feature ✨ PRs that add or enhance a feature labels Nov 26, 2023
@rubenwardy rubenwardy added the Concept approved Approved by a core dev: PRs welcomed! label Nov 27, 2023
@appgurueu
Copy link
Contributor

Currently it is not possible to get all HUD element IDs. If you would try to enumerate them, it is impossible to know when to stop. You can't just stop after an ID is empty, since they are not consecutive if an element gets removed.

I think it could still be possible: Add HUD elements until you get two consecutive IDs. That gives you an upper bound up until which you could enumerate HUD elements ;)

On a more serious note, what's the use case for this? It seems to me so far that forcing modders to keep track of HUD IDs themselves is the better solution.

@cx384
Copy link
Contributor Author

cx384 commented Nov 30, 2023

I think it could still be possible: Add HUD elements until you get two consecutive IDs. That gives you an upper bound up until which you could enumerate HUD elements ;)

There could always be a gap of more than two IDs, if they get removed, so I don't see how this could be a solution, but maybe I didn't understand you correctly. Also currently removed IDs get reused, so you don't get much information from adding new elements. (Maybe HUD IDs shouldn't be reused, but unique anyways.)

In practice you could just assume that there are at most 1000 or so elements, since it is very unlikely that there are more, but this would just be inefficient.

On a more serious note, what's the use case for this? It seems to me so far that forcing modders to keep track of HUD IDs themselves is the better solution.

Mods must still keep track of their own HUD IDs for unambiguous identification (the ID is the only unique identifier), but most of the time they don't expose them for others to see. So from another mod you can't know for sure which elements exist.

Some use cases:

  • Change the z_index of all existing elements.
  • Overwrite all elements of a certain kind.
  • Find a free HUD position where there isn't already another element .

@appgurueu
Copy link
Contributor

On a more serious note, what's the use case for this? It seems to me so far that forcing modders to keep track of HUD IDs themselves is the better solution.

Mods should still keep track of their own HUD IDs for identification, but most of the time they don't expose them for others to see. So from another mod you can't know for sure which elements exist.

Yes, and I think this is a feature: You are forced to use the APIs of other mods (or your own APIs / tables) to manage HUD elements. This helps prevent mod incompatibilities since it makes it harder to "pull the rug" from under other mods.

Frankly, I'm worried that such an API would be used mostly irresponsible. The only "responsible use" I can imagine is one regarding HUD elements you control yourself, which is already what modders do given the current API(s), and which is probably cleaner (and more efficient) to keep this way. I'm not really opposed to adding such a feature, but I believe it should probably at least get a warning in the docs, considering e.g. your use cases, all of which have high potential to "pull the rug" from under other mods:

  • "Change the z_index of all existing elements" - suddenly the mods' assumptions about z-index conventions are broken. (I also believe you should almost never change z-indices of existing elements, esp. of other mods - rather, you should pick your own z-indices of the new elements you're adding appropriately.)
  • "Overwrite all elements of a certain kind" - this is extremely likely to introduce incompatibilities.
  • "Find a free HUD position where there isn't already another element" - this seems like a valid use case (though something like a "layouting system" would probably be better for this).

Effectively, whenever you're messing with "foreign" HUD elements, you're likely to break stuff. Using this API to get a hold of HUD IDs to modify foreign elements should thus be discouraged. Furthermore, using this API to let Minetest manage your own elements should be discouraged as well: Managing them yourself is cleaner and more efficient.

TL;DR: You're right, this feature is useful, as esp. your last use case shows. I'm worried that modders will abuse this though, so I'd recommend that the docs discourage (1) messing with foreign HUD elements; (2) foregoing keeping track of their HUD elements themselves.

@cx384
Copy link
Contributor Author

cx384 commented Nov 30, 2023

I get what you mean, but other mods not knowing whether HUD elements already exist can also create incompatibilities (e.g. multiple minimaps or overlapping elements). And you have the same problem almost everywhere else too. (e.g. item metadata description, player physics override, global callbacks, ...)

"Change the z_index of all existing elements" - suddenly the mods' assumptions about z-index conventions are broken. (I also believe you should almost never change z-indices of existing elements, esp. of other mods - rather, you should pick your own z-indices of the new elements you're adding appropriately.)

Yeah, a better use case would be to find the biggest or smallest z_index

"Overwrite all elements of a certain kind" - this is extremely likely to introduce incompatibilities.

I meant it to be used like formspec_prepend if you for example want to change the style somehow.

TL;DR: You're right, this feature is useful, as esp. your last use case shows. I'm worried that modders will abuse this though, so I'd recommend that the docs discourage (1) messing with foreign HUD elements; (2) foregoing keeping track of their HUD elements themselves.

I added this to the API documentation.

games/devtest/mods/testhud/init.lua Outdated Show resolved Hide resolved
games/devtest/mods/testhud/init.lua Outdated Show resolved Hide resolved
src/script/lua_api/l_object.h Outdated Show resolved Hide resolved
src/script/lua_api/l_localplayer.cpp Outdated Show resolved Hide resolved
src/script/lua_api/l_object.cpp Outdated Show resolved Hide resolved
doc/client_lua_api.md Outdated Show resolved Hide resolved
doc/client_lua_api.md Outdated Show resolved Hide resolved
src/player.cpp Show resolved Hide resolved
doc/lua_api.md Outdated Show resolved Hide resolved
@SmallJoker SmallJoker self-requested a review January 6, 2024 16:50
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. Code is fine too. (Not sure we want to nothing vs. nil, but that's a nitpick; also not duplicating HUD code would be ideal, but is out of scope for this PR to resolve.)

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.

Works on my PC.

@appgurueu appgurueu merged commit 92c55c2 into minetest:master Jan 14, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Concept approved Approved by a core dev: PRs welcomed! Feature ✨ PRs that add or enhance a feature @ Script API >= Two approvals ✅ ✅
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants