Extend IdRef serialization from game events to protocol method args#10343
Extend IdRef serialization from game events to protocol method args#10343MostCromulent wants to merge 8 commits intoCard-Forge:masterfrom
Conversation
…l method args Replaces full CardView/PlayerView object graphs in protocol method args with lightweight IdRef(typeTag, id) markers resolved from the local Tracker. Consolidates TrackableObject serialization into TrackableSerializer; removes GameEventProxy (absorbed into TrackableSerializer). Also fixes pre-existing PlayerControllerHuman call to updateHasAvailableActions using removed int signature; updated to Predicate<SpellAbility> form.
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>
Extends encoder replacement (previously server→client only) to the client→server path. The client encoder now replaces CardView/PlayerView with lightweight IdRef markers in protocol method args like selectCard, and the server decoder resolves them from the tracker. Testing revealed that after a zone change (e.g. playing a land from hand), the server tracker holds a stale CardView with the old zone, causing Game.findByView to search the wrong zone and return null. Added a fallback global search in findByView when the zone-specific search misses, rather than modifying tracker update behavior which has wide blast radius. Also removes GameEventProxy — raw GameEvent objects travel inside DeltaPacket, with replacement handled by the encoder/decoder. Confirmed by batch testing (0 dropped events). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…coder Events embedded in DeltaPacket are excluded from encoder IdRef replacement (to protect state data), so their TrackableObject references arrive as raw deserialized instances with null trackers. When event handlers call TrackableTypes.lookup(), the null tracker causes NPE. Added a fallback in TrackableSerializer.resolve() that detects raw TrackableObjects with no tracker and resolves them to the canonical tracker instance, matching what GameEventProxy.unwrapAll previously did. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Events bundled in DeltaPacket were excluded from encoder-level IdRef replacement to avoid a decoder timing issue: the Netty decoder resolves IdRefs during deserialization, before applyDelta creates new objects in the client tracker. This caused events to travel as raw TrackableObjects, losing StaleCardRef detection and inflating wire size. Restore the wrap/unwrap pattern from the removed GameEventProxy, now in TrackableSerializer: events are serialized to byte arrays with IdRef replacement at DeltaPacket construction time (server), and deserialized with resolution after state application (client), when the tracker is fully populated. This gives events proper IdRef/StaleCardRef handling without the decoder timing problem. - Remove applyDelta exclusion from shouldReplaceTrackables - Remove raw-TrackableObject fallback from TrackableSerializer.resolve - Add wrapEvents/unwrapEvents to TrackableSerializer - Wrap events in both delta+events and events-only paths Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…verload Address PR #17 review feedback from tool4ever: - Delete single-arg TrackableSerializer.replace(Object) — verified byte-equivalent to replace(obj, null) (the tracker null-check is the only difference, and skipping it falls through to the same IdRef return). Update CObjectOutputStream and TrackableSerializer's own ReplacingOutputStream to use the unified two-arg form. - Delete CompatibleObjectEncoder.ReplacingObjectOutputStream and CompatibleObjectDecoder.ResolvingObjectInputStream — exact duplicates of TrackableSerializer.ReplacingOutputStream/ResolvingInputStream modulo class qualifier. Make those inner classes package-private and reuse them directly from the encoder/decoder.
tool4ever
force-pushed
the
fix-protocol-serialization-delta
branch
from
d751aba to
d4771ab
Compare
|
Planning to merge in around two weeks if nothing else comes up |
| // Zone-specific search may miss if the view has stale zone info | ||
| // (e.g. IdRef resolved from a tracker that wasn't updated after a | ||
| // zone change). Fall back to global search. | ||
| if (visit.getFound() == null) { |
There was a problem hiding this comment.
need to investigate if we'd also hit this locally:
i.e. if there are situations (viewing face down stuff etc.) where we don't want to always find newer instance!
There was a problem hiding this comment.
AI analysis:
I don't think the fallback can actually return a "different" Card instance, but it's worth walking through why.
Lookup is purely by ID.
CardIdVisitor(Game.java:677) matches oncard.getId()only — no zone/visibility/face-down logic. The fallback is semantically equivalent tofindById(view.getId()), which already exists and is used elsewhere in this file.Card IDs are unique among Cards reachable by
forEachCardInGame. That walker visits Graveyard, Hand, Library, Battlefield, MeldedCards, Exile, Command, InboundTokens, Stack. The only construct in the codebase that creates a second Card with the same ID isCardCopyService.getLKICopy(CardCopyService.java:230), and LKI snapshots live in transient trigger/replacement maps — never in zones — so the walker never encounters them. Face-down is a property toggle on a single Card instance, not a distinct Card, so face-down/face-up doesn't produce two ID-matching candidates either.So when both paths succeed, they return the same Card. The only behavioral difference is
null(old) vs. the real Card (new) when the view's zone is stale — never wrong Card vs. right Card.Local play: The miss path shouldn't normally fire locally because the local UI's CardView is the live tracker's CardView and stays in sync via
TrackableProperty.changed. The bug this fallback addresses is network-specific (server tracker holds a CardView with a stale zone becauseupdateObjLookupskips already-registered IDs). If local ever does hit it, the outcome is the same — find the Card by ID — just via the longer path.One potential downside is that silent recovery masks bugs. If local play ever starts hitting the fallback, nobody notices. This could be addressed by adding a one-line
WARNlog inside the fallback branch so we'd see it in logs if it ever fires locally.
2 participants
@tool4ever re-targeting this at master now that delta is merged.
Previous PR discussion should be preserved here: MostCromulent#17
Summary
Protocol methods serialize CardView/PlayerView references as full object graphs — every
selectCard,selectPlayer, and similar call sends the entire trackable object tree. This replaces that with lightweight IdRef(typeTag, id) markers at the Netty encoder/decoder level, and consolidates all TrackableObject serialization logic into a singleTrackableSerializerclass.wrapEvents/unwrapEventsGame.java— when a zone-specific search misses (e.g. IdRef resolved from a tracker that wasn't updated after a zone change), falls back to global ID searchsetGameViewandopenVieware excluded from replacement since they carry the full state that populates the client's Tracker.Design decisions
Fallback search vs tracker update (findByView): When the server tracker holds a stale CardView with an old zone (because
updateObjLookupskips objects whose ID is already registered),findByViewsearches the wrong zone. Rather than modifying the tracker's!hasObjguard (shared by delta sync and view updates, high blast radius), a fallback global search is added — it only triggers on a miss, matching the existing null-zone fallback path.Event wrap/unwrap timing: Events in DeltaPacket can't use encoder-level replacement because the decoder runs before
applyDeltacreates new objects in the tracker. Instead, events are serialized to byte arrays with IdRef replacement at packet construction time (server-side), and deserialized with resolution after state application (client-side), when the tracker is fully populated.Server vs client encoder modes: The server encoder uses a full Tracker (set by
RemoteClientGuiGame.setGameView) for verified replacement with StaleCardRef detection. The client encoder does simple IdRef replacement without a tracker — using one would create StaleCardRef markers that the server resolves as detached CardViews, breaking game object identity.🤖 Generated with Claude Code