Skip to content

Extend IdRef serialization from game events to protocol method args#10287

Closed
MostCromulent wants to merge 1 commit intoCard-Forge:masterfrom
MostCromulent:fix-protocol-serialization
Closed

Extend IdRef serialization from game events to protocol method args#10287
MostCromulent wants to merge 1 commit intoCard-Forge:masterfrom
MostCromulent:fix-protocol-serialization

Conversation

@MostCromulent
Copy link
Copy Markdown
Contributor

Issue

Every protocol method that passes a CardView or PlayerView argument triggers Java serialization of the entire object graph - a CardView pulls in its PlayerView owner, which pulls in all zones, which pull in all cards. This is redundant: the client already has these objects registered in its Tracker via setGameView.

GameEventProxy already replaces CardView/PlayerView references with lightweight ID markers when serializing game events -avoiding Java serialization expanding each reference into the entire object graph. This PR extends the same IdRef pattern to protocol args at the Netty encoder/decoder level to avoid serializing the same data repeatedly.

Solution

  • Replace full TrackableObject serialization in protocol method arguments with lightweight ID references (IdRef markers) at the Netty encoder/decoder level
  • Server encoder swaps CardView/PlayerView refs with IdRef(typeTag, id); client decoder resolves them from the Tracker. setGameView and openView are excluded since they carry the full state
  • Extract shared ID-mapping primitives from GameEventProxy into TrackableRef so both the encoder/decoder path (protocol args) and GameEventProxy (game events) reuse the same mechanism

Why this is safe

The client's Tracker is kept in sync by setGameView messages that deliver the full game state. A CardView/PlayerView can only appear in a protocol method's args if it already exists in the game state. If the object was recently created, that state change generated GameEvents queued in the forwarder. NetGuiGame.send() calls flushPendingEvents() before every protocol message - this flushes accumulated events via handleGameEvents, which sends a setGameView (current full state) before the events. So any newly-created object should reache the client's Tracker before the protocol message that references it.

setGameView and openView themselves are excluded from replacement - they carry the full object state that populates the Tracker. The decoder only enables IdRef resolution after openView, when the client first has a valid Tracker.

If an IdRef fails to resolve, the field becomes null and a warning is logged. This is stricter than the old behavior, where the full (possibly stale) object was serialized inline. In practice, resolution failures should be rare: the flush-before-send pattern ensures the Tracker reflects the server's state at the time each message is composed.

If there is a resolution failure this should, at worst, result in a momentary visual glitch and a warning in the logs. No desync, no disconnect, no game state corruption.


🤖 Generated with Claude Code

Protocol methods were serializing the entire CardView/PlayerView object
graph for each argument. Add IdRef replacement at the Netty encoder/decoder
level: the server encoder swaps CardView/PlayerView with lightweight
IdRef(typeTag, id) markers, and the client decoder resolves them from the
Tracker. setGameView and openView are excluded (they carry the full state).

Extract shared ID-mapping primitives (IdRef, type tags, typeTagFor,
trackableTypeFor) from GameEventProxy into TrackableRef so both the
encoder/decoder path and GameEventProxy can reuse them.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@MostCromulent
Copy link
Copy Markdown
Contributor Author

MostCromulent commented Apr 4, 2026

@tool4ever here's initial implementation. Have tested locally without issue.

A couple of open questions:

  1. Merge sequencing

Since we're planning on merging delta in the immediate future, my suggestion is to hold this until after that merge. This avoids us debugging this against current master which will only exist transiently, only to have to debug again if refactors in delta introduce unforeseen interactions.

For time being protocol handling on master is no worse than before (other than addition of the 'setHighlights' method, which follows existing architecture).

  1. Do protocol methods need 'updateGameView'?

The client should have all state information needed before the protocol is received. Currently not all methods include 'updateGameView', but even considering those that do: it's not immediately clear to me why 'showPromptMessage', 'showCardPromptMessage', 'setSelectables' etc would need an accompanying gameview update?

Three potential approaches:

a) remove updateGameView from protocols - though at risk of introducing a class of unforeseen bugs on top of anything emerging from the IDRef changes. I did briefly test a version of this with them removed and didn't spot any obvious issues, but that testing was far from comprehensive
b) decide it doesn't matter: once delta is merged the marginal network cost of a redundant 'updateGameview' is trivial
c) if we do want to optimize: iterate on this with testing as a follow-up after this is merged, checking for edge cases in each of the methods.

if (event != null) {
result.add(event);
} else {
netLog.debug("Dropped event with unresolved references");
Copy link
Copy Markdown
Contributor

@tool4ever tool4ever Apr 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can't we get rid of this whole class too?
because now the normal encoders would perform the same work...
(only some of the error handling might not be redundant)

final ChannelPipeline pipeline = ch.pipeline();
pipeline.addLast(
new CompatibleObjectEncoder(),
new CompatibleObjectEncoder(false),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't client have even less reason to send its full objects back?
needs a comment at least, e.g. maybe it makes sense to start with only the more expensive side for the first user feedback round

@tool4ever
Copy link
Copy Markdown
Contributor

  1. you could try to merge this against the delta sync branch (might have to open a fresh PR for that though)
    then we already have the correct changeset and can test a bit based on that
  2. c)

@MostCromulent
Copy link
Copy Markdown
Contributor Author

MostCromulent commented Apr 5, 2026

you could try to merge this against the delta sync branch (might have to open a fresh PR for that though)
then we already have the correct changeset and can test a bit based on that

Here's seperate branch and PR targeting delta instead: MostCromulent#17

Have also implemented client->server path changes and GameEventProxy removal there per comments above.

Should probably close this PR for now? We can either merge the other PR into delta branch once satisfied it won't cause issues, or I can later create a new PR against master once delta is merged.

@tool4ever
Copy link
Copy Markdown
Contributor

Hmn I thought it would have already been possible to open it inside this repo but I guess that doesn't really make sense when both branches are on another fork :D
anyway the retargeted PR shows it's probably too complex to reasonably merge together with it either, we'll just refine it from there for a more delayed approach

@tool4ever tool4ever closed this Apr 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants