SSCSM: Add sscsm api for hud elements, mirroring csm api - #17331
SSCSM: Add sscsm api for hud elements, mirroring csm api#17331ZenonSeth wants to merge 4 commits into
Conversation
| * `core.hud_change(id, stat, data)` | ||
| * `core.hud_get(id)` | ||
| * `core.hud_get_all()` | ||
| * Shares an id space with client-provided CSM (CPCSM) HUD elements; |
There was a problem hiding this comment.
it should share the ID space with server-provided HUD elements instead. i should be able to modify a SSM-controlled HUD element from a SSCSM, rather than having to jump through the hoops of letting SSCSM manage the element entirely.
CPCSM doesn't really make sense because as a server, i don't know which CSMs the client is running. i do however know which SSMs i'm running and which SSCSMs i'm sending.
and because SSCSMs are server-controlled and not user-controlled, there is no concern about this being immediately used for cheating.
There was a problem hiding this comment.
it should share the ID space with server-provided HUD elements instead.
Not exactly share but rather map into the SSCSM id namespace, probably. You need to be able to allocate an id on server/client without conflicting with the other one accidentally.
Imo in many cases it would be good to (at least conceptually) implement SSM calls by sending a packet to the client and then handle that packet on the client in SSCSM, using the SSCSM API. For modders this is then a white-box (that can be overwritten).
So, here think about what you would do if you were an SSCSM that wants to implement an API for SSMs to do hud stuff, and how that is supposed to interact with other SSCSMs' hud elements.
There was a problem hiding this comment.
Yes CPCSM elements should be completely separate.
You can just map into the SSM elements with SSCSM like it was the case with CPCSM before #17268.
Game::m_hud_server_to_client still exists, so you basically only need an extra core.hud_id_from_server(id) for SSCSM to get the real id from the server element id.
SSCSM mods have to know themselves which ids belong to them anyway.
The only problem may be if a SSCSM removes an SSM element, in this case you have to clear the id in Game::m_hud_server_to_client too, since SSCSM could reuse the removed id.
If a SSCSM changes or removes an SSM element the server still holds the old element, but I don't think this is a problem, since mods can deal with this themselves.
There was a problem hiding this comment.
Right, so other issues are easy to address, this one is a bit more complex I think. Had some digging through the code (And asking claude questions about the state of things + discussing ideas). Two main things I see here:
If I understand correctly SSCSM should operate directly on player->hud which is where SSM calls to client end up anyway. That's doable.
I would also could or should add something like a Lua API core.hud_id_from_server(server_id) - which would just be exposing the translation logic of HUD serverID->clientID to SSCSM mods from the m_hud_server_to_client map.
This can be done but would need to move the m_hud_server_to_client map from Game class to somewhere in Client class, since I don't think there's a way for the SSCSM exec function which only gets the Client class, to reach that map in the Game class - Client has no reference to Game. This seems like it would also have to be the authoritative map, not just a copy, if we want SSCSM elem to free up element ids.
The other part though - being able to modify SSM-created HUD element from a SSCSM (or vice-versa) seems like it would require something a bit beyond the scope of this PR: The ability for SSCSM to exchange data with its corresponding SSM.
The above changes to use m_hud_server_to_client and put directly into player->hud would enable this to happen, but those changes alone are not enough, and I imagine a generic communication channel SSM<->SSCSM is needed anyway, and would be for SSM to control SSCSM HUD or vice versa
So my suggestion is that I can do the 1st part in this PR, but then the SSM<->SSCSM communication would complete the entire picture as I understand it.
There was a problem hiding this comment.
No need to complicate things. LocalPlayer is a better place to store m_hud_server_to_client anyway.
We may even want to move the map to Lua eventually, and handle the HUD events (that are sent from the server) in SSCSM.
You currently can't even send data from SSM to SSCSM, I think, so core.hud_id_from_server(server_id) is currently useless, because you can't know about a server_id, but it will be needed in the future.
(You can make some guesses about the server_id, but theoretically it's not supported by the API.)
I guess you don't have to implement core.hud_id_from_server(server_id), just yet.
And yes, SSCSM should operate directly on player->hud, this is what we want.
There was a problem hiding this comment.
Yeah ofc, like i said, doable and makes sense. Just was making sure the comment about "modify a SSM-controlled HUD element from a SSCSM" is addressed - that this PR alone isn't enough, even if I did add the hud_id_from_server func - but i won't do it here then.
| print(string.format("sscsm_test0: hud_add returned id=%s", tostring(id))) | ||
|
|
||
| local got = core.hud_get(id) | ||
| local ok, mismatch_key, expected, actual = fields_match(form, got or {}) |
There was a problem hiding this comment.
I'd simplify this a lot. Just make it assert(deep_equal(form, got)). All the fluff isn't really necessary unless it breaks. And if it does I'll just throw a quick print(dump(form), dump(got)) at it myself.
Also in the future we'll likely use busted-style unit testing so this just becomes assert.same or similar, which has all these niceties.
Same for the other prints. Should just be assertions IMO. Makes the tests much more concise, and is less annoying if everything works as expected.
| bool read_hud_change(lua_State *L, HudElementStat &stat, HudElement *elem, void **value, | ||
| int stat_idx = 3, int data_idx = 4); | ||
|
|
||
| // Applies one stat computed on the SSCSM thread onto the element's main-thread copy. |
There was a problem hiding this comment.
| // Applies one stat computed on the SSCSM thread onto the element's main-thread copy. | |
| // Used to apply one stat computed on the SSCSM thread onto the element's main-thread copy. |
the function itself just copies a property between HUD elements
| // hud_add(form) | ||
| int ModApiSSCSM::l_hud_add(lua_State *L) | ||
| { | ||
| auto request = SSCSMRequestHudAdd{}; |
There was a problem hiding this comment.
style-wise, i think i would prefer
| auto request = SSCSMRequestHudAdd{}; | |
| SSCSMRequestHudAdd request; |
same for the other structs.
to make sure that primitive members without default constructors are still initialized, i'd give them = 0; and = false; initializers.
| LocalPlayer *player = client->getEnv().getLocalPlayer(); | ||
| u32 id = player->csm_hud.add(std::make_unique<HudElement>(std::move(elem))); | ||
|
|
||
| Answer answer{}; |
There was a problem hiding this comment.
same here. this and id = 0 in the struct def is what i'd prefer.
| Answer answer{}; | |
| Answer answer; |
| { | ||
| struct Answer final : public ISSCSMAnswer | ||
| { | ||
| std::vector<std::optional<HudElement>> elems; |
There was a problem hiding this comment.
personal opinion: std::vector<std::pair<u32, HudElement>> elems; or similar is the more robust design, for two reasons. it'll have much nicer density (std::optional<HudElement> is basically as large as HudElement) and it deals much nicer with sparse and potentially large IDs, which we may get in the future.
(i think HUD should likely be switched to a hash map in the future and ID reuse should be avoided.)
…sed on PR comments
| #include "hud_element.h" | ||
| #include "log_internal.h" | ||
| #include "script/common/c_content.h" | ||
| #include <optional> |
There was a problem hiding this comment.
| #include <optional> |
|
|
||
| u32 id; | ||
| HudElementStat stat; | ||
| HudElement value; |
There was a problem hiding this comment.
this stores an entire HUD element when all you really need is the single field that's getting updated. wouldn't it be cleaner to have a std::variant of all the HUD elem field member types?
then you could split into two steps: read this variant from a lua key-value pair (via read_hud_change), then have a HudElement function to apply the variant to a HUD element.
imo this would be cleaner than the copy_hud_stat trick which effectively abuses HUD elements as a variant store. as a bonus, we could get rid of the void pointer nonsense.
There was a problem hiding this comment.
I can switch to using std::variant but the void pointer wasn't something I added, and to fix that there's a few other files that would need changes, stuff I haven't touched here, so not sure this PR is the place to do that - but def can introduce a variant union of all the hud elem types to be used here and for later.
There was a problem hiding this comment.
Did the std::variant change, put it in its own commit so you can see.
62cf2fd to
8f279d0
Compare
8f279d0 to
62cf2fd
Compare
Goal of the PR
How does the PR work?
Changed
read_hud_elementandread_hud_changeto now take explicit Lua stack indices with defaults matching the old hardcoded 2/3/4 instead of hardcoding them. Existing CSM/server callers are not affected.Adds new
copy_hud_stat(stat, from, to): applies one stat field from one HudElement onto another, used to move a stat computed on the SSCSM thread onto the real elementAdds new SSCSM Lua bindings:
core.hud_add(form),core.hud_remove(id),core.hud_change(id, stat, data),core.hud_get(id),core.hud_get_all().hud_changeworks in two round trips: fetch the current element, apply the stat change to a local copy, then send just that one stat back viacopy_hud_stat.Adds new request/answer structs: SSCSMRequestHudAdd, SSCSMRequestHudRemove, SSCSMRequestHudGet, SSCSMRequestHudGetAll, SSCSMRequestHudChange. Each exec()s on the main thread and use the LocalPlayer::csm_hud store
Does this relate to a goal in the roadmap?
SSCSM
If you have used an LLM/AI to help with code or assets, you must disclose this.
Yes, Claude Code was used for many boilerplate work around lua binding as well as doing code review and fixing bugs in my code.
To do
This PR is a Ready for Review.
How to test
Run singleplayer with SSCSM enabled for singleplayer.
Two things:
First observe debug output, should print:
Second: In-game should show a simple text added in the top center of the screen: