Serialize CardViews missing from host tracker#10644
Open
MostCromulent wants to merge 1 commit intoCard-Forge:masterfrom
Open
Serialize CardViews missing from host tracker#10644MostCromulent wants to merge 1 commit intoCard-Forge:masterfrom
MostCromulent wants to merge 1 commit intoCard-Forge:masterfrom
Conversation
Skip IdRef replacement for CardViews absent from the host tracker so ephemerals (e.g. Jhoira choice copies that never enter a tracked zone) serialize natively. The receiver registers them in its tracker on arrival so subsequent IdRef references resolve correctly. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
tool4ever
reviewed
May 10, 2026
| if (obj instanceof TrackableObject trackableObject) { | ||
| if (trackableObject.getTracker() == null) { | ||
| trackableObject.setTracker(this.tracker); | ||
| registerInTracker(trackableObject); |
Contributor
There was a problem hiding this comment.
I really hope we can avoid this part - it would essentially mean losing the optimization of #10343 on the client side for CardView...
can't we use this reasoning instead:
- as long as hosts sends full objects we know they're ephemeral
- if for some reason they end in up in the real game they would start to get tracked and reach client via normal delta path first anyway
with that client can instead perform an identical tracker check if he needs to respond to host with CardView (or IdRef is enough) 💡
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #10641
Bug
In MoJhoSto network play, a remote player activating Jhoira of the Ghitu Avatar's ability (choose 1 of 3 random instant/sorcery copies to cast) never sees the choice dialog — the ability fizzles.
PlayEffectcreates the choice copies intoZoneType.Nonewithout callingZone.add, so theirCardViews never reach the host's tracker; whengetChoicesships them asIdRefs, the client resolves them to null.Fix
Skip
IdRefreplacement forCardViews that are absent from the host tracker. They serialize natively (full Java object) and the receiver registers them on arrival.Two changes:
TrackableSerializer.replace(): in non-event mode, only emitIdRefwhentracker.getObj(...)finds the entry. Otherwise return the object as-is so Java serialization writes the fullCardViewinline.PlayerViewstill usesIdRefunconditionally (always tracker-resident). Wrapped events keep the existingEventCardRef+preserveSnapshotlogic (see "Why only non-event mode" below).GameClientHandler.updateTrackers: while walking incoming protocol message args, alsotracker.putObj(...)any TrackableObject not already in the id lookup. This registers inline-serializedCardViews so subsequentIdRefreferences targeting the same id (including the client's reply path) resolve correctly.Why only non-event mode
Wrapped events use a separate
EventCardRefmechanism that already carries name + image-key snapshots inline, so ephemeral references in events display correctly via thepreserveSnapshot=falsefallback. Events also need that snapshot mechanism for a different problem — staleCardViewrefs after zone changes — which would break under inline-serialization-plus-tracker-register: a stale snapshot from an event would overwrite the newer authoritative entry in the tracker, and subsequentIdRefresolutions for that id would return the stale view.In practice, ephemeral
CardViews are unlikely to be referenced in events anyway: the window where Jhoira's choice copies are tracker-miss isCard.fromPaperCard(...)→ choice dialog → cast resolves, and during that window the cards aren't on the stack and aren't in any tracked zone.Alternative considered
A different approach was to give
updateGameViewan overload accepting "extra"TrackableObjects, so callers could inject ephemeral views into the nextDeltaPacket'snewObjects. The client tracker would then be populated via the normal delta-sync new-object path, and a bareIdRefin the subsequent protocol method would resolve cleanly.It was rejected because the implementation surface is larger and one piece of it is a real refactor of delta-sync timing.
The flow under the overload would be: build a delta carrying the ephemerals, send
applyDelta, immediately send the protocol method that uses them. The two messages have to ship back-to-back with no game-thread work between them — the ephemerals only exist to support that one protocol call, so anything that lets them scatter into normal batching would defeat the point.But with no game-thread work between the two sends, the client receives them back-to-back too — the IO thread decodes both before the EDT can apply the first. We're not aware of this race manifesting today because regular flows leave milliseconds of game-thread work between creating a card and prompting on it, giving the EDT time to catch up. The overload removes that buffer, so it also requires splitting
applyDeltastep 1 (create new objects,tracker.putObj) intobeforeCallon the IO thread to keep ordering correct.That split would create a window where new CardViews live in the tracker without their properties — every path that reads tracker entries between IO-thread
beforeCalland EDT-side property application (afterDeltaApplied hooks, event dispatch ordering, reconnection state-reset, the in-place CardView-replacement-on-zone-change semantics increateObjectOnly) would need careful re-verification, with real risk of subtle regressions for a one-issue fix.The inline-serialization approach sidesteps ordering entirely: the ephemeral
CardViewarrives as part of the protocol message itself, fully constructed before the message is dispatched. No new delta-sync code paths, and noapplyDeltasemantics shift.Testing completed
mvn -pl forge-gui -am install -DskipTestscleanNetworkPlayIntegrationTest#runQuickDeltaSyncTest(10-game batch): 10/10 success, 0 errors, 0 checksum mismatches🤖 Generated with Claude Code