Extend IdRef serialization from game events to protocol method args#10287
Extend IdRef serialization from game events to protocol method args#10287MostCromulent wants to merge 1 commit intoCard-Forge:masterfrom
Conversation
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>
|
@tool4ever here's initial implementation. Have tested locally without issue. A couple of open questions:
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).
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 |
| if (event != null) { | ||
| result.add(event); | ||
| } else { | ||
| netLog.debug("Dropped event with unresolved references"); |
There was a problem hiding this comment.
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), |
There was a problem hiding this comment.
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
|
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. |
|
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 |
Issue
Every protocol method that passes a
CardVieworPlayerViewargument triggers Java serialization of the entire object graph - aCardViewpulls in itsPlayerViewowner, which pulls in all zones, which pull in all cards. This is redundant: the client already has these objects registered in its Tracker viasetGameView.GameEventProxyalready 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
IdRef(typeTag, id); client decoder resolves them from the Tracker.setGameViewandopenVieware excluded since they carry the full stateGameEventProxyintoTrackableRefso both the encoder/decoder path (protocol args) andGameEventProxy(game events) reuse the same mechanismWhy this is safe
The client's Tracker is kept in sync by
setGameViewmessages 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()callsflushPendingEvents()before every protocol message - this flushes accumulated events viahandleGameEvents, which sends asetGameView(current full state) before the events. So any newly-created object should reache the client's Tracker before the protocol message that references it.setGameViewandopenViewthemselves are excluded from replacement - they carry the full object state that populates the Tracker. The decoder only enables IdRef resolution afteropenView, when the client first has a valid Tracker.If an IdRef fails to resolve, the field becomes
nulland 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