-
Notifications
You must be signed in to change notification settings - Fork 2k
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
[CSM] Add basic HUD manipulation. #6067
Conversation
|
@red-001 very nice implementation, it will permit to have custom Hud client side to show various local things without involving server |
src/network/clientpackethandler.cpp
Outdated
|
||
u32 client_id = getEnv().getLocalPlayer()->getFreeHudID(); | ||
m_hud_server_to_client[server_id] = client_id; | ||
errorstream << "Add " << "client id:" << client_id << " server id:" << server_id << std::endl; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why errorstream?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh sorry that's test code that I forgot about.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You might have left it in, but to the infostream or debugstream
@paramat yes |
src/network/clientpackethandler.cpp
Outdated
event.type = CE_HUDRM; | ||
event.hudrm.id = id; | ||
m_client_event_queue.push(event); | ||
std::unordered_map<u32, u32>::iterator i = m_hud_server_to_client.find(server_id); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const_iterator
src/network/clientpackethandler.cpp
Outdated
event.hudchange.data = intdata; | ||
event.hudchange.v2s32data = new v2s32(v2s32data); | ||
m_client_event_queue.push(event); | ||
std::unordered_map<u32, u32>::iterator i = m_hud_server_to_client.find(server_id); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
src/hud.cpp
Outdated
{HUD_FLAG_MINIMAP_VISIBLE, "minimap"}, | ||
{0, NULL}, | ||
}; | ||
#ifndef SERVER |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i don't like this ifndef. Please add the moved code to a common header between client and server. We really should remove this dependency between client/server parts which are near renderer
src/script/lua_api/l_localplayer.cpp
Outdated
{ | ||
LocalPlayer *player = getobject(L, 1); | ||
u32 id = luaL_checkinteger(L, 2); | ||
HudElement* element = player->removeHud(id); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HudElement *element
src/script/lua_api/l_localplayer.cpp
Outdated
LocalPlayer *player = getobject(L, 1); | ||
u32 id = luaL_checkinteger(L, 2); | ||
HudElement* element = player->removeHud(id); | ||
if (element == NULL) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (!element)
src/script/lua_api/l_localplayer.cpp
Outdated
if (!e) | ||
return 0; | ||
|
||
void *unused; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
void *unused = nullptr; ?
46d8bd8
to
a3c8793
Compare
Please add a description with a summary of what this does. |
i think i will adopt it. |
Yeah once i looked through the code i saw it is basic HUD element support The lack of description means people misunderstand, which caused my first comment above. The PR is not so simple that no description is needed, the basic content of a PR needs to be clear without looking at the code. RBA used to leave out the description on non-trivial PRs and it was really irritating. Even worse when one is asked for and it does not appear (deserves a -1 in those cases). It's very easy for a PR author to add a clear summary and it really helps reviewers and managing of contributions. It should be a rule for contributions. |
src/hud.h
Outdated
const core::rect<s32> *clip, | ||
Client *client, | ||
ItemRotationKind rotation_kind); | ||
extern const struct EnumString es_HudElementType[]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
redundant struct keywoard, we are in C++
can you pass clang-format on headers and add client/hud.cpp to the clang-format-whitelist file please ? |
@@ -97,8 +97,10 @@ src/guiTable.h | |||
src/guiVolumeChange.cpp | |||
src/guiVolumeChange.h | |||
src/httpfetch.cpp | |||
src/hud.cpp | |||
src/client/hud.cpp |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please respect alphabetical order on this list. Why adding src/client/hud.h ? Linter is correct on it
Thanks. |
@SmallJoker can you review it, it should be merged ASAP it's one of the key feature of CSM, especially when mod channels will be merged |
doc/client_lua_api.md
Outdated
### HUD Definition (`hud_add`, `hud_get`) | ||
```lua | ||
{ | ||
hud_elem_type = "image", -- see HUD element types |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Defaults to HUD_ELEM_TEXT
-> "text"
EDIT: Wrong defaults in position, scale, number (needs more docs too), item, size and world_pos (which is missing here).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I copied the example from the server lua api doc. I think it was meant to just be an example not a list of default values.
doc/client_lua_api.md
Outdated
@@ -976,6 +976,16 @@ Methods: | |||
* returns last look vertical angle | |||
* `get_key_pressed()`: | |||
* returns last key typed by the player | |||
* `hud_add(definition)` | |||
* add a HUD element described by HUD def, returns ID number on success | |||
* See [`HUD definition`](#hud-definition-hud_add-hud_get) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- returns the HUD ID or
nil
, if it failed to create the HUD
doc/client_lua_api.md
Outdated
* add a HUD element described by HUD def, returns ID number on success | ||
* See [`HUD definition`](#hud-definition-hud_add-hud_get) | ||
* `hud_get(id)` | ||
* returns the [`definition`](#hud-definition-hud_add-hud_get) of the HUD with that ID number |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or nil
, if non-existent.
doc/client_lua_api.md
Outdated
* remove the HUD element of the specified id, returns `true` on success | ||
* `hud_change(id, stat, value)` | ||
* change a value of a previously added HUD element | ||
* element `stat` values: `position`, `name`, `scale`, `text`, `number`, `item`, `dir` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or nil
, if the HUD ID is non-existent.
if (!element) | ||
lua_pushboolean(L, false); | ||
else | ||
lua_pushboolean(L, true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The HUD must be deleted. i.e. delete element;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comments.
@nerzhul @SmallJoker is rebased. |
@@ -448,6 +448,7 @@ set(common_SRCS | |||
version.cpp | |||
voxel.cpp | |||
voxelalgorithms.cpp | |||
hud.cpp |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please keep it sorted alphabetically.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
* add a HUD element described by HUD def, returns ID number on success and `nil` on failure. | ||
* See [`HUD definition`](#hud-definition-hud_add-hud_get) | ||
* `hud_get(id)` | ||
* returns the [`definition`](#hud-definition-hud_add-hud_get) of the HUD with that ID number or `nil`, if non-existent. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This link may not work in a few markdown viewers due the parentheses and code expressions in the title
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
ClientEvent *event = new ClientEvent(); | ||
event->type = CE_HUDRM; | ||
event->hudrm.id = client_id; | ||
m_client_event_queue.push(event); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also remove this ID from m_hud_server_to_client
when Game::handleClientEvent_HudRemove
is called later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
src/script/lua_api/l_localplayer.cpp
Outdated
{ | ||
LocalPlayer *player = getobject(L, 1); | ||
|
||
u32 id = lua_tonumber(L, -1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also an issue for the SSM l_hud_get
: This function returns 0
if it's not a valid number. Returning nil
seems reasonable in that case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't be an issue, in most cases returning nothing is almost the same as returning nil
.
Might be easier to just change the documentation for SSM to say "return nothing if invalid"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My point is that 0
is also a valid HUD ID - this would get the wrong ID for non-numeric parameters.
EDIT: -> luaL_checkinteger
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh sorry I didn't see what you meant there, I thought you meant l_hud_get
by this function and the return 0
in it which resulted in it returning nothing, when the documentation said it returns nil
in case of invalid input.
formatted the whitelist with |
note the last commit is only needed because previous changes to CSM seem to have broken on_connect callbacks |
Workaround for on_connect not working right now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay for me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 as soon the last comment is handled.
* [CSM] Add basic HUD manipulation. Workaround for on_connect not working right now.
* [CSM] Add basic HUD manipulation. Workaround for on_connect not working right now.
* [CSM] Add basic HUD manipulation. Workaround for on_connect not working right now.
Adds CSM bindings for the core HUD manipulation commands and some sample/test lua code. See docs for a list of added functions.