-
Notifications
You must be signed in to change notification settings - Fork 200
#2648 Spring.SendLuaUIMsg #2712
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
base: master
Are you sure you want to change the base?
Conversation
…ring modes.
The Spring.SendLuaUIMsg API now accepts either a string ("s", "a", or "") retaining compatibility or an integer (0–255) to specify recipients , supporting direct messaging to individual players as well as groups (everyone, spectators, allies).
Updated network packet handling and protocol code to match the new recipient ID scheme.
…252-254). Updated LuaHandle, LuaUnsyncedCtrl to reflect these changes.
…e during merge, unsure why code got reduplicated during merge from main -> fork
p2004a
left a comment
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.
Only skimming through, it seems that:
- Breaks autohost interface compatibility: can't distinguish between old and new format
- Breaks replay parsing: can't distinguish between old and new format
… Allows replay parsers/autohost to differentiate between packets. Updated HandleLuaMsg to reflect these changes.
… Allows replay parsers/autohost to differentiate between packets. Updated HandleLuaMsg to reflect these changes. Swapped SendLuaUIMsg to use old logic upon receiving string, new logic upon receiving int.
|
Stepping back a bit, what is even the use case of #2648 @sprunk? What game wants to do what? If you squint, you could consider #2040 to be related and in #2355 (comment) we considered pushing that through One note from that investigation maybe is also relevant here:
so, upon receiving the lua msg callin, game has no way to know whatever that luamsg was send to everobydy, allies, only that player. Is that a problem for whatever use case this functionality is supposed to be used for? |
I don't quite remember, but I think somebody wanted to implement requests for a specific person, as in you'd have a widget that sends "hey you, and I mean you specifically, please share me a unit" or "hey you, please attack here" to a particular teammate and only the recipient sees it on his GUI. Maybe even send a "hey you specifically, please ceasefire with me" to a particular enemy in FFA.
Good point to consider #2040. My thoughts:
Shouldn't be a problem. |
p2004a
left a comment
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.
Thanks Sprung, good points and I agree, use case seems reasonable.
To the PR itself, the constructions optimizes for backwards compatibility, not clarity, maybe that's still good call at this point, and let's see how things evolve.
rts/Net/Protocol/BaseNetProtocol.cpp
Outdated
| * @param uint16_t script | ||
| * @param uint_8 destination | ||
| * @param vector<uint8_t> rawData | ||
| * Packs LuaMsgData into PackPacket object, wrapped in a PacketType (shared ptr?) |
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.
nit: let's drop ?
rts/Net/Protocol/BaseNetProtocol.cpp
Outdated
| /* | ||
| * @param uint8_t playerNum | ||
| * @param uint16_t script | ||
| * @param uint_8 destination |
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.
let's use the same name of variables and params here, but more important: why just list them here without any comment repeating what goes into function arguments?
rts/Lua/LuaUnsyncedCtrl.cpp
Outdated
| /*** @function Spring.SendLuaUIMsg | ||
| * @param message string | ||
| * @param mode string "s"/"specs" | "a"/"allies" | ||
| * @param mode string "s"/"specs" | "a"/"allies" | int x \in [0,255]; |
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.
Is this valid? I don't think I've seen this format anywhere else. Also x is not too helpful as far as the meaning.
Although I guess the type here is actually just string and "s"/... | int x in [0,255]; is the description. So perhaps it should be like this? idk.
| * @param mode string "s"/"specs" | "a"/"allies" | int x \in [0,255]; | |
| * @param mode string|integer "s"/"specs" | "a"/"allies" | playerID |
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.
Agreed. The intention was to write "an integer x belonging to the interval 0-255, but playerID is much better.
Changed locally to match suggestion.
…types w/ better error handling, explicitly added case where 2nd argument not received. Also updated descriptor wording + formatting. Readded LuaHandle::LUAMSG_TYPES enum to solve magic numbering for HandleLuaMsg Renamed HandleLuaMsg variable to better reflect usage, uses LUAMSG_TYPES enum to avoid magic numbering within the LUA_HANDLE_ORDER_UI case. Renamed CBaseNetProtocol::SendLuaMsg mode argument to recipientID to better reflect usage.
rts/Lua/LuaHandle.cpp
Outdated
| const int msgAllyTeam = teamHandler.AllyTeam(player->team); | ||
| sendMsg = teamHandler.Ally(msgAllyTeam, gu->myAllyTeam); | ||
| } | ||
| } |
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.
| } | |
| } |
rts/Lua/LuaHandle.h
Outdated
| enum LUAMSG_TYPES { | ||
| EVERYONE = (uint8_t) '\0', | ||
| ALLIES = (uint8_t) 'a', | ||
| SPECTATORS = (uint8_t) 's', | ||
| }; |
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 type casting doesn't work, it's still just int https://godbolt.org/z/8hcxsofPb
| enum LUAMSG_TYPES { | |
| EVERYONE = (uint8_t) '\0', | |
| ALLIES = (uint8_t) 'a', | |
| SPECTATORS = (uint8_t) 's', | |
| }; | |
| enum class LUAMSG_TYPES: uint8_t { | |
| EVERYONE = '\0', | |
| ALLIES = 'a', | |
| SPECTATORS = 's', | |
| }; |
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.
Good catch. I didn't know C++ enums forces a cast to integer unless you specify it above like that.
rts/Lua/LuaUnsyncedCtrl.cpp
Outdated
| * @param message string Lua Message to be sent. | ||
| * @param mode string/number, mode designator/recipient id. expects none (everyone) | "s"/"specs" | "a"/"allies" | playerID (0-255) |
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.
| * @param message string Lua Message to be sent. | |
| * @param mode string/number, mode designator/recipient id. expects none (everyone) | "s"/"specs" | "a"/"allies" | playerID (0-255) | |
| * @param message string Lua Message to be sent. | |
| * @param mode string/number, mode designator/recipient id. expects none (everyone) | "s"/"specs" | "a"/"allies" | playerID (0-255) |
Co-authored-by: Marek Rusinowski <marekrusinowski@gmail.com>
Fixes function descriptor formatting + error typo for LuaUnsyncedCtrl::SendLuaUIMsg Fixes logic formatting and whitespace in LuaHandle::HandleLuaMsg. Fixes enum LUAMSG_TYPES implicitly casting `uint8 -> int` by explicitly setting enum-value type in declaration. Co-authored-by: Marek Rusinowski <marekrusinowski@gmail.com>
rts/Lua/LuaUnsyncedCtrl.cpp
Outdated
| } else if (lua_israwnumber(L, 2)) { | ||
| recipientID = luaL_optnumber(L, 2, -1); // if no arg, outputs -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.
Can't be no arg since it was just checked in the line above.
| } else if (lua_israwnumber(L, 2)) { | |
| recipientID = luaL_optnumber(L, 2, -1); // if no arg, outputs -1 | |
| } else if (lua_israwnumber(L, 2)) { | |
| recipientID = lua_tonumber(L, 2); |
rts/Lua/LuaUnsyncedCtrl.cpp
Outdated
| } else if (lua_israwnumber(L, 2)) { | ||
| recipientID = luaL_optnumber(L, 2, -1); // if no arg, outputs -1 | ||
| if (recipientID < 0 || recipientID > MAX_PLAYERS) { | ||
| luaL_error(L, "Invalid player ID sent (expecting between 0-251): ", recipientID); |
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.
Needs to use printf formatting, and also use the magic constant rather than hardcode 251:
| luaL_error(L, "Invalid player ID sent (expecting between 0-251): ", recipientID); | |
| luaL_error(L, "Invalid player ID sent (%d, expecting between 0-%d): ", recipientID, (int) MAX_PLAYERS); |
Ditto for the other error calls.
rts/Lua/LuaHandle.cpp
Outdated
|
|
||
| switch (script) { | ||
| case LUA_HANDLE_ORDER_UI: { | ||
| case LUA_HANDLE_ORDER_UI_SINGLE: { // expecting 0-251, if invalid int, doesn't register. |
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.
0-251 doesn't explain the meaning behind the number so is a bit less helpful than it could be
| case LUA_HANDLE_ORDER_UI_SINGLE: { // expecting 0-251, if invalid int, doesn't register. | |
| case LUA_HANDLE_ORDER_UI_SINGLE: { // expects a playerID |
… printf formatting when yielding verbose errors.
Implements #2648 - previous PR was on a branch that for some reason duplicated 4000+ lines of code. This is on a new branch which shouldn't have this issue.
Spring.SendLuaUIMsg now has a single target mode for integer x 0-251, while retaining 'a', 's', '\0' compatibility. Compatible with Zero K widgets calling SendLuaUIMsg as well.