Skip to content

Route opponent-hand reveal and discard through FloatingZone on desktop#10660

Merged
tool4ever merged 11 commits into
Card-Forge:masterfrom
MostCromulent:opponent-discard-hand-click
May 14, 2026
Merged

Route opponent-hand reveal and discard through FloatingZone on desktop#10660
tool4ever merged 11 commits into
Card-Forge:masterfrom
MostCromulent:opponent-discard-hand-click

Conversation

@MostCromulent
Copy link
Copy Markdown
Contributor

@MostCromulent MostCromulent commented May 11, 2026

USING MIND WARP, X=2

BEFORE:
Reveal step
Screenshot 2026-05-12 074617
Select step
Screenshot 2026-05-12 074624

AFTER:
Reveal step
Screenshot 2026-05-12 074032
Select step
Screenshot 2026-05-12 074044


Summary

Per #10646 (comment): RevealYouChoose / LookYouChoose discards (Inquisition, Mind Warp, etc.) currently surface as a names-list reveal dialog followed by a separate selection grid, when the opponent's hand could be the selection UI directly.

This routes both steps through the existing FloatingZone: reveal opens the hand face-up with a minimal OK acknowledgement; the pick uses InputSelectCardsFromList so legal targets are clicked in that same FloatingZone.

Implementation

  • PlayerControllerHuman.reveal() — desktop opponent-hand reveals swap getGui().reveal() (names list) for tempShowZones + getGui().message() (visual hand + minimal OK).
  • PlayerControllerHuman.chooseCardsToDiscardFrom() — opponent-discard pick routes through InputSelectCardsFromList; tempShows the full hand for Reveal*/Look* modes without RevealNumber so revealed non-selectable cards stay visible. RevealNumber spells (Blackmail, Cabal Interrogator) keep the conservative tempShow-of-valid path so unrevealed cards stay face-down.
  • CMatchUI.hideZones() — fix a pre-existing asymmetry: tempShowZones special-cases ZoneType.Hand, but hideZones only checked FLOATING_ZONE_TYPES (which omits Hand). The FloatingZone opens for an opponent's hand but never closed programmatically.

Safety

Both PCH changes gate on UI_SELECT_FROM_CARD_DISPLAYS && !getGui().isLibgdxPort() — the same condition useSelectCardsInput already uses. Mobile and preference-off paths are unchanged. isLibgdxPort() is the per-GUI network-safe check from #10615. No engine, protocol, or TrackableProperty changes.

RevealNumber partial-reveal spells (e.g. Blackmail revealing 3 of 5) are rendered safely: FloatingZone.getCards() returns all hand cards, but each is drawn according to the existing mayView check — only mayLookTemp-flagged cards (the revealed subset) show their faces, the rest render as card backs. Non-revealed cards' identities stay hidden; only hand size is conveyed, which is already public info.

Potential code cleanup (not in this PR)

FPref.UI_SELECT_FROM_CARD_DISPLAYS was added in 2019 (commit 09fc3ae60c2) alongside the original arrangeForScry GUI introduction, which brought in-zone selection as a new UX. The preference has defaulted to true with no user-facing UI toggle ever since — effectively the active desktop UI path for 7+ years.

The preference gates whether InputSelect extends beyond the local player's own hand and battlefield to other zones (opponent-hand FloatingZone, libraries, graveyards, etc.) — i.e. click a card in its zone display versus fall back to a names-list dialog. All three desktop call sites in PlayerControllerHuman pair it with !getGui().isLibgdxPort().

A separate cleanup PR could drop the FPref entry, simplify the three dual-gate sites to single !isLibgdxPort() checks, and collapse the now-dead fallback branches in useSelectCardsInput (the zone-set ternary) and arrangeForScry (the manual-order path).


🤖 Generated with Claude Code

…on desktop

For RevealYouChoose / LookYouChoose discards (Inquisition, Thoughtseize, etc.)
on desktop, present the opponent's hand visually in the FloatingZone instead
of a names-list dialog:

- Reveal step: replace getGui().reveal() (names list) with tempShowZones plus
  a minimal OK dialog. The actual cards are visible in the FloatingZone; the
  dialog just blocks until the user acknowledges.
- Pick step: route through InputSelectCardsFromList so the user clicks legal
  targets directly in the FloatingZone. tempShow the full hand for Reveal/Look
  modes without RevealNumber so non-selectable revealed cards remain visible;
  for RevealNumber spells (Blackmail, Cabal Interrogator, etc.) the path is
  unchanged so unrevealed cards stay hidden.

Both halves gate on UI_SELECT_FROM_CARD_DISPLAYS && !isLibgdxPort() -- the
same condition useSelectCardsInput already uses. Mobile and the
preference-off path keep the existing dialog flow.

Also fix CMatchUI.hideZones to mirror tempShowZones' Hand handling -- the
show side opens the FloatingZone for an opponent's Hand, but the hide side
only checked FLOATING_ZONE_TYPES (which omits Hand), so the FloatingZone
never closed programmatically after the prompt.

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

MostCromulent commented May 11, 2026

Let me know if you want the FPref.UI_SELECT_FROM_CARD_DISPLAYS code cleanup suggested above as part of this PR.

@tool4ever
Copy link
Copy Markdown
Contributor

tool4ever commented May 12, 2026

I think in general this seems better:

  • now leverages the advantage of being able to inspect the boardstate, helping you make tough decisions
  • the used dialog had incorrect implications about the order mattering (while rare it sometimes does but that decision is done afterwards by the owner!)

the only minor downside remaining is no longer being able to use the keyboard to progress:

  • though arguably that was a rather fiddly process, I doubt many users were doing that over mouse unless they had a very good reason (technical/physical)...?
  • but I think we could do much better and turn it into a real speed-up for everyone across multiple use cases 🧠

what if we (temporarily) assign the number keys to the first 9 selectables? might have to draw small labels on them for clearity or something - maybe based on Ctrl press 🤔
want to give that a shot? 🥳

(there's actually one place in CMatchUi that uses VK_* similarly already but for abilities)

@MostCromulent
Copy link
Copy Markdown
Contributor Author

what if we (temporarily) assign the number keys to the first 9 selectables? might have to draw small labels on them for clearity or something - maybe based on Ctrl press 🤔
want to give that a shot? 🥳

yeah I'll try that out - might also look at how seamlessly we could integrate a search filter at the top as well, since we also lose that compared being able to start typing in the previous list view

Adds a name-search field at the top of FloatingZone and Ctrl+1-9
hotkeys for the first nine selectable cards during an active
selection prompt. Numbered badges render on those cards and a small
hint footer surfaces the shortcut. Also fixes CMatchUI.updateCards
so opponent-hand FloatingZones refresh on highlight changes —
previously only the local VHand was covered.

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

MostCromulent commented May 12, 2026

Screenshot 2026-05-12 213043

Should work for any card-select prompt that uses FloatingZone - opponent hand, graveyard, exile, library etc.

Does not apply for cases which don't use FloatingZone - own hand, battlefield, players, ephemeral non-zone cards (though as follow up I'm going to explore what we could do about the ephemerals; plausibly could generate a temp FloatingZone window containing the ephemeral cards to take advantage of same UX).

Comment thread forge-gui-desktop/src/main/java/forge/view/arcane/FloatingZone.java
@tool4ever
Copy link
Copy Markdown
Contributor

Let me know if you want the FPref.UI_SELECT_FROM_CARD_DISPLAYS code cleanup suggested above as part of this PR.

hmn I'm undecided if users might prefer having the preference actually GUI-exposed now that this PR increases its usage 🤔
let's wait a bit and see if we'll get some reactions after merging on Discord?

Comment thread forge-gui/src/main/java/forge/player/PlayerControllerHuman.java Outdated
@Jetz72
Copy link
Copy Markdown
Contributor

Jetz72 commented May 12, 2026

Screenshot 2026-05-12 213043

If the revealed zone is prompting the user to choose something (e.g. some number of cards to discard), maybe the prompt might replace the usual title text? Or does it already seem visible enough in the usual prompt space?

tool4EvEr and others added 3 commits May 12, 2026 17:41
When the local player is picking cards from a revealed opponent zone
(e.g. Mind Warp, Coercion), the FloatingZone now renders the current
prompt at the top of the window so the player doesn't have to glance
back to the main prompt panel while deciding.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
PlayerController.chooseCardsToDiscardFrom now takes the collection of
cards the chooser is allowed to see, rather than letting the controller
inspect the SpellAbility to decide visibility — addressing reviewer
feedback to stop dissecting SpellAbilities inside the controller.
DiscardEffect (the only caller that reveals extra cards) computes the
visible set at the call site; a 5-arg convenience overload keeps
BalanceEffect/ConniveEffect untouched.

Incidentally fixes a case where, with RevealNumber set and a DiscardValid
filter, revealed-but-invalid cards would vanish from the chooser's UI
even though the reveal had publicly disclosed them.

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

If the revealed zone is prompting the user to choose something (e.g. some number of cards to discard), maybe the prompt might replace the usual title text? Or does it already seem visible enough in the usual prompt space?

Yeah I think this is worthwhile, otherwise its not immediately obvious what you're expected to do in window unless you cross reference with the prompt.

Screenshot 2026-05-13 073019

Comment thread forge-gui/src/main/java/forge/player/PlayerControllerHuman.java
@tool4ever

This comment was marked as resolved.

@MostCromulent
Copy link
Copy Markdown
Contributor Author

Potential follow-up:

Forge currently processes a discard effect as two separate actions: reveal stage shows window with revealed cards, user clicks ok, then it repaints window for discard stage. I don't think there's any reason from a rules perspective why that two stage process is necessary? If the discard stage is able to display the revealed cards at same time (which it now does in this PR) this can potentially be consolidated into one action (at least on desktop). Would need to consider mobile UI.

@tool4ever
Copy link
Copy Markdown
Contributor

yea some API even call tempShowCards on their own
and there is also DelayedReveal which offers a more dedicated mechanism for this?

In general there should be substantial cleanup potential for redundant or almost useless code in these PlayerController methods, just need to carefully consider what each effect attempts to do...

and ideally some more Javadoc to explain less obvious concepts

MostCromulent pushed a commit to MostCromulent/forge that referenced this pull request May 14, 2026
Eight substantive corrections from the adversarial review:

1. Public-reveal regression (#1, highest severity). GameAction.reveal()
   fans out to all players' controllers, not just the chooser's.
   Dropping the engine call when consolidating UX would silently remove
   visibility for opponents, spectators, replay, and network observers.
   Replace with a chooser-aware overload that excludes one player from
   the fan-out; chooser sees cards inline via DR, everyone else still
   sees the popup. Updates Engine-side change and Behavior compatibility
   sections.

2. FloatingZone has no prompt header today (#3). Earlier draft assumed
   PR Card-Forge#10660 added prompt/button infrastructure to FloatingZone; it
   didn't. The two CMatchUI TODOs refer to the GuiChoose modal dialog
   ("search dialog"), not FloatingZone. Distinguish the two desktop
   paths in the IGuiGame section and rewrite the corresponding Risks
   bullet. Recommend a separate read-only DR FloatingZone alongside the
   selectable one to avoid extending FloatingZone in this PR.

3. Mobile multi-select is a UX shift (#4). GameEntityPicker today is
   tap-to-confirm (onItemActivate immediately sets the optionPane
   result). Multi-select needs tap-toggle + OK-button confirmation.
   Right-size the GameEntityPicker change accordingly.

4. DR-as-context vs DR-as-options (#2). DigEffect, ChangeZoneEffect,
   ChooseCardEffect pass DR cards that are disjoint from the selectable
   set. Card-click selection mode needs DR cards rendered as
   visible-but-not-selectable. The separate-FloatingZone approach
   handles this naturally; document it in Risks.

5. tempShowCards reentrancy (#5). PCH.tempShownCards is a single flat
   list; endTempShowCards clears it unconditionally. Engine effects
   outside the selection path also call tempShowCards. TempReveal.close()
   needs snapshot/restore semantics to avoid wiping outer scopes. Update
   the TempReveal sketch and add snapshotTempShown / restoreTempShown
   helpers to the spec.

6. Self-discard "bug" was a misread (#6). addMayLookTemp is an
   idempotent set add; for own-hand it's a no-op. The omission in
   chooseCardsToDiscardFrom is correct. Rewrite the motivating bullet
   to describe drift risk rather than an observable bug.

7. OrderRequest doesn't fit arrangeForScry / arrangeForSurveil /
   orderMoveToZoneList (#7). These chain selection-ordering-preference
   in idiosyncratic ways. Limit OrderRequest to the simple ordering
   case (order-the-blockers, sideboard) and have the complex methods
   stay in PCH with TempReveal + a thinner card-click-vs-dialog helper.

8. Diff size estimate (#8). Bumped to ~730-840 added / ~255-355
   removed (~+450-550 net), accounting for honest costs in CMatchUI DR
   rendering (both surfaces), mobile multi-select interaction model,
   PCH snapshot/restore helpers, and the engine-side reveal overload.

https://claude.ai/code/session_01MLo2yfVmtwVAB6HfQyv29d
MostCromulent pushed a commit to MostCromulent/forge that referenced this pull request May 14, 2026
Card-Forge#10660 (FloatingZone reveal+discard) already handles DR on the
card-click path — FloatingZone is a passive zone display and
InputSelectEntitiesFromList is the selection gate, so DR-as-options for
the discard case renders naturally with no new code.

DualCardBox introduces a card-grid widget behind chooseCardPiles with
source-pile-and-draggability-gate semantics that handle both DR shapes
(DR-as-options where options = DR.cards, DR-as-context where options
subset DR.cards) via the same mechanism.

This spec drops the CMatchUI DR rendering work — both desktop surfaces
are covered by the prerequisites. CMatchUI changes shrink to just
implementing the new predicate.

Partial migration list shrinks: arrangeForScry, arrangeForSurveil,
chooseContraptionsToCrank, manipulateCardList are all DualCardBox's
territory now. Only orderMoveToZoneList stays in PCH with TempReveal.

DR rendering for non-discard chooseSingleEntityForEffect /
chooseEntitiesForEffect callers (tutors, fetches) is now explicitly a
follow-up — DualCardBox keeps those separate and they retain today's
two-stage UX until a DR-aware widget covers them.

Diff size shrinks from ~+325-375 / ~-300-400 to ~+245-255 / ~-270-370.
Net ~-25 to -115 (this PR removes more than it adds). 0 new files,
8-9 modified.

https://claude.ai/code/session_01MLo2yfVmtwVAB6HfQyv29d
@tool4ever tool4ever merged commit acbd41d into Card-Forge:master May 14, 2026
2 checks passed
@MostCromulent MostCromulent deleted the opponent-discard-hand-click branch May 14, 2026 07:29
Icehawk78 added a commit to Icehawk78/forge that referenced this pull request May 14, 2026
…rder-prefs

* upstream/master: (50 commits)
  Add Commander bracket calculation and bracket views (Card-Forge#10672)
  Route opponent-hand reveal and discard through FloatingZone on desktop (Card-Forge#10660)
  Lobby update fixes and null-card guard in CMatchUI.updateCards (Card-Forge#10618)
  Correct Bilbo, Thief in the Night
  Stingcaster Mage (FRA leak) (Card-Forge#10484)
  Bloodline Recollector // Ancestral Craving
  Bilbo, Thief in the Night
  PH23: Mr. Monopoly, On the Go (Card-Forge#10666)
  Card cleanup: 2026-05-13
  Consolidate inner event codec onto CObject streams (Card-Forge#10664)
  Add files via upload (Card-Forge#10653)
  Realm of legends 1.045: Mapfixes (Card-Forge#10656)
  Fix CardView IdRef substitution / retire UI_NETPLAY_COMPAT (Card-Forge#10662)
  Serialize CardViews missing from host tracker (Card-Forge#10644)
  Fix Gift on non-permanents
  CounterAiCategory: positive/negative/neutral grouping for Counters  (Card-Forge#10650)
  Fix lobby start crash for auto-generated deck variants (Card-Forge#10643)
  Hotfix split cards crash (Card-Forge#10652)
  Consolidate net archive deck storage classes (Card-Forge#10640)
  CardFactory: build traits last (Card-Forge#10649)
  ...

; Conflicts:
;	forge-gui/src/main/java/forge/localinstance/properties/ForgePreferences.java
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants