Let the AI count converge before it decides whether to cast

[liamiak]

Rebuilt on current master as six commits. The old series carried two approaches it later
abandoned, which is what made it hard to read — and what stopped it rebasing.

Count$Converge reads 0 while the AI is deciding. Nothing has been spent yet, so Bring to
Light searched for a nought-drop and then declined to cast at all. getPayingColors() now asks the
activating player's controller what it expects to spend. PlayerController answers 0 — bit
identical to what the empty payingMana loop produced — so humans and every controller that does
not override it are unchanged.

Where the answer lives. AiCache, keyed on the player and the spell, beside
evaluateBoardPosition and aiLifeInDanger. Its clear() already runs at the top of
chooseSpellAbilityToPlay, so an answer lasts exactly one decision and needs no reset of its own.
Nothing is parked on the spell and AiController is not touched. Non-converge cards are turned
away before the solve, since every card that reads its own payment arrives here.

X announces colours, not size. On a converge card the useful X is the least one that still
reaches as many colours as the board allows, not every mana the AI can find. PermanentAi already
walked X this way; that walk moves into the mana code so spells reach it too, and so it is solved
once per decision rather than once per caller — every step of the walk is a full payment solve.

Sweep the Skies is the case no walk reached: TokenAmount$ Y reads Count$Converge off a
different SVar than X announces, and the count is read before anything announces it. One thopter on
master, five here.

Four Count$Converge overrides are gone. Each computed the amount with calculateAmount, then
discarded it and recomputed, because calculateAmount read zero before payment. It no longer does.

And it tutored for the worst card. chooseCardToHiddenOriginChangeZone reads
Destination$ Exile as getting rid of something, so searching your own library took getWorstAI —
Bring to Light preferred a Runeclaw Bear to a Serra Angel. Narrowed to the decider searching their
own library, so exiling an opponent's library is untouched. Beseech the Mirror, Emergent Ultimatum,
Portent of Calamity and three others reach the same line.

Engineered Explosives stays flagged — its counters pick which mana value the wipe hits, so most
colours is usually the worst live choice.

Source is +90/-34. Suite 715, 0 failures.

🤖 Implemented with the assistance of Claude Code (Opus 5).

[tool4ever]

don't like having to create extra fake Mana

what are the alternatives?

[liamiak]

You're right — I missed this one on the first pass. Working it now, expect another push.

[liamiak]

Pushed. Went with predicting the colours rather than the payment: a byte predictedPayingColors on SpellAbility that getPayingColors() ORs in, set and cleared on the same lines that already borrow and return castSA.

payingMana is a multiset with provenance and the prediction is only a set of colours, so the Mana objects were right about colour and invented the rest — one entry per colour, each sourced from the spell itself, which is what Count$Adamant, Count$TotalManaSpent and Count$EachSpentToCast read.

On the redundancy point: getSunburst() had no callers left once getConvergeCount was rewritten — it was ColorSet.fromMask(sunburstMap).countColors() over the same field getColorsPaid() returns — so I removed it. Happy to put it back if you'd rather keep the accessor.

To be straight about the scope: the mask is only set for hasConverge() cards, and none of the 31 in the pool use ManaColorsPaid or ManaSpent, so Count$Converge is the only reader that can observe a non-zero value today. Outside the AI's own evaluation getPayingColors() is unchanged.

[tool4ever]

Mhn, I'll think about it though a lot of work has previously gone into removing AI only stuff from the main rules engines...

[liamiak]

Coming back to this properly - you were right, and that field is gone.

What is left in forge-game is +17/-4 across three files: ManaCostBeingPaid loses getSunburst() (getColorsPaid() already returned the mask), PlayerController gains a default getExpectedPayingColors(sa) returning 0, and getPayingColors() asks it only while payingMana is still empty.

So nothing AI-side is stored in the rules engine now - it gained a question it can ask, in the same shape as the rest of PlayerController. PlayerControllerAi is the only override, so on the human path the answer is 0 and ColorSet.fromMask(0) is exactly what the old loop returned over an empty payingMana: unchanged rather than merely equivalent.

On runtime: every getPayingColors() call site is behind a script-level guard (ManaSpent, ManaNotSpent, Count$Converge), which is 65 of 33,664 card scripts, so the controller is never asked in a loop. The AI half also moved from computing this up front to working it out on first read, and returns 0 immediately unless the card actually has converge.

Rebased onto current master; suite 356/0/6.

Sorry this sat - it was reworked on the 16th and I answered in a general comment rather than here, so the thread still read as open.

[tool4ever]

don't mind a more generic solution but then you need to replace the specific variant above
otherwise we end up with duplicated logic that also wastes runtime
though the ones with X will probably make things tricky 🤔

[liamiak]

Agreed, and that's done as of the push after your review — getConvergeCount is now just ColorSet.fromMask(getConvergeColors(sa, ai)).countColors(), so there's one payment simulation rather than two. Same result either way: getSunburst() was already ColorSet.fromMask(sunburstMap).countColors() and getColorsPaid() returns that same mask.

The X ones are in that push too — X gets announced before the colours are measured, since on those cards X is what buys them.

[tool4ever]

well the code duplication is gone but the method is still used and calculates again for no reason?

[liamiak]

You're right, and it goes further than that — the method can go entirely.

All four callers had the same shape: work out the amount with calculateAmount, then override it for Count$Converge. That override exists because calculateAmount returned zero before anything had been paid, which is the problem this branch set out to fix. With the prediction in place the engine's own expression gives the right number, so the override was running a full payment simulation to reach a value the line above it already held.

I checked rather than assumed, since it would be easy to see the right number coming from resolution instead of evaluation. Instrumenting the Count$Converge branch in AbilityUtils and tagging each call with its caller: 21 of them arrive via ChangeZoneAi.hiddenOriginCanPlayAI — the AI deciding whether to cast — with castSA set and the count correct.

PermanentAi's converge loop was already replaced by setXForBestConverge earlier on this branch, so removing these four leaves getConvergeCount with no callers and it goes too. That commit is +1/-20.

Verified on the paths I could reach. Painful Truths on the DrawAi path and Unified Front on TokenAi both get cast and sized correctly with the override gone — three cards and three life, four tokens. I added the first as a regression test. DamageAllAi I could not exercise: its checkApiLogic is never reached for Radiant Flames, the only DamageAll converge card in the pool. It is the same one-line pattern as the other two so I would not expect it to differ, but it is untested here rather than verified.

339 tests, 0 failures.

[liamiak]

Pushed two commits.

reuse the converge colours the X search already found — the recompute you spotted. setXForBestConverge now returns the mask it settled on instead of the caller working it out again: 13 → 12 payment simulations casting Chamber Sentry off five lands.

offer the converge prediction through PlayerController instead — an alternative for your point about AI-only state in the rules engine. Not meant to land alongside the field version; take whichever you prefer and I'll drop the other.

It parks nothing on the spell. getPayingColors() asks the activating player's controller what it means to spend while nothing has been spent; PlayerController answers 0 so humans are unchanged, and PlayerControllerAi forwards to what AiController worked out. One seam still serves Converge, Count$ManaColorsPaid and both ManaSpent checks, and there's no mask left to go stale.

Scoped twice — canPlayAndPayFor hands back whatever the outer spell was expecting, so a nested evaluation can't lose the outer prediction, and chooseSpellAbilityToPlay resets it next to AiCache.clear() and predictedCombat. HELD_MANA_SOURCES_FOR_NEXT_SPELL four lines up is the same species of state: good for one spell, kept AI side, reset at priority.

The honest trade: the field version is +17/−21 across three files, this is +60/−17 across six, and the prediction becomes controller-scoped rather than spell-scoped, so a runWithController swap loses it. Both cases degrade to 0, which is the answer before any of this.

[tool4ever]

my recompute was still meant for something else but I'll try to explain if I can also figure out good prediction storing to revisit it

I think for now I'll just cherry pick the fix in PermanentAi while keeping the early loop exit around

[liamiak]

Rebased — #11515 is in, so the PermanentAi loop is out of this branch.

I re-ran the tests against plain master to see what the rest is still worth: Chamber Sentry and
Skyrider Elf now pass without any of this, so those two tests stopped proving anything and I have
stopped claiming them. Only Bring to Light still fails. I added one more for Sweep the Skies, whose
TokenAmount$ Y reads Count$Converge off a different SVar than X announces — one thopter on
master, five with the prediction.

On prediction storing: I had a go while this was waiting. It sits on AiController keyed by spell
identity, reset at priority next to AiCache.clear(), and canPlayAndPayFor snapshots and restores
it so a nested evaluation cannot clobber the outer spell's answer. Keeping the announced X with the
colours is also what settles your recompute — Chamber Sentry off five lands went 12 payment solves
to 6, and off nine lands, with a five-colour early exit, 20 to 6.

No rush from me. If you would rather land your own version of the storing, say so and I will drop
this to just the Sweep the Skies and tutor pieces.

[tool4ever]

yea that looks too messy :/
at the same time performing the converge count before any api class logic takes away the opportunity of doing any quick sanity checks first
so overall it's rather questionable if it needs to be this roundabout...

it might be easier to implement a new generic helper that can also cover other scenarios, e.g. Fling or if there are any similar mechanics where a payment decision has such high impact on the effect?
also worth noting that in test mode payManaCost doesn't create mana objects (as I thought something could probably be reused there) - just temporarily overwriting the SVars directly could be another approach though that wouldn't be as comprehensive as working with the original SA fields in the end either

[liamiak]

Both good calls, and the first changed the shape of it.

Eager → lazy. getExpectedPayingColors now solves on the first read and remembers by spell identity, so predictConvergePayment and its call in canPlayAndPayFor are gone, along with the snapshot and restore that only existed because the eager call mutated state ahead of a nested evaluation. A converge spell rejected by a cheap check no longer pays for a payment solve. AiController is the same size either way (+66), so this buys ordering rather than size, and I haven't timed it.

My first attempt left the getter announcing X as a side effect of a read, which is worse than what it replaced. Deleting that branch failed the Sweep the Skies test, so it was load-bearing rather than dead — TokenAi works in checkPhaseRestrictions, and spawnToken reads the count before anything announces X. Moving the announcement above spawnToken fixes it at the source, and the getter is a pure read now.

Sweep the Skies. TokenAi only announces X when TokenAmount is literally "X"; that card reads its count off a separate SVar, so nothing announced its X and it made one thopter off any board. The fix is gated on hasConverge() — unguarded it also caught Fractal Summoning, Wild Hypothesis, Slime Molding and Wurmcalling, and the last two have TokenPower$ X, so they would have solved for X twice per evaluation. I also dropped the setXManaCostPaid(setMaxXValue(...)) idiom here: setMaxXValue already settles on the converge-best X, and keeping its return value put the largest affordable X back over it — five thopters for one less mana now.

Worth knowing: that piece stands alone. Those five lines on plain master, with none of the prediction, already get Sweep the Skies to five thopters (announcing max X rather than the converge-best X, so one mana more). Happy to split it out if you would rather take it separately.

On overwriting the SVars: that already exists, ChangeZoneAi:1773, in a better form than mutating the card — it substitutes into a local copy of the SVar string, so there is no restore and nothing leaks into LKI copies. It cannot reach either case here though: Sweep the Skies has no AI-side read to substitute at, and ManaSpent$/ManaNotSpent$ are trigger and condition params rather than SVars.

On payManaCost in test mode: confirmed, ComputerUtilMana:647. The throwaway list only ever receives pool mana and is refunded before return — mana from tapping is never objectified, since predictManafromSpellAbility returns a string straight into the ManaCostBeingPaid. So the colour mask is the only complete artifact. The reusable thing currently discarded is the returned paymentList, and two of the otherwise unpredictable reads (AbilityUtils:1852, :1895) only want Mana::getSourceCard.

On a generic helper, the family already splits three ways:

• yes/no ("was {R} spent?") — already generic: SpellAbilityAi.checkConditions probes canPayManaCost for the colours and blanks the requirement.
• count ("how many colours?") — nothing today, and it cannot be a yes/no probe, which is what this PR is.
• identity ("which card was sacrificed?") — Fling, handled ad hoc by the substitution above.

A helper covering all three would be a controller-answered "what do you intend to pay", with colours as the first instance. Glad to take that on separately rather than widen this PR into it speculatively.

On the tests, so you don't have to work out which is which: reverting the source to master and keeping the tests, two of the four fail — Bring to Light (not cast at all) and Sweep the Skies (one thopter). The Chamber Sentry and Painful Truths ones pass either way; they guard the removed PermanentAi and DrawAi overrides rather than demonstrate a fix.

Rebased on master; suite 356/0/6, checkstyle clean, and the same 356/0/6 on the branch without these two commits.

[liamiak]

Went back at "too messy" and you were right about where the weight was. Two commits on top,
no rebase.

The prediction no longer needs storage of its own. It was four fields, four methods and a
reset hook on AiController, remembering two values keyed by one spell. AiCache already does
that — evaluateBoardPosition and aiLifeInDanger use it — and its clear() runs at the top of
chooseSpellAbilityToPlay, the line I had put the reset next to. Holding the colours and the
announced X there instead leaves AiController untouched by this branch, and lets
PlayerControllerAi call ComputerUtilMana directly, so ComputerUtilMana no longer downcasts
to PlayerControllerAi to find somewhere to put things. Source is +91/-34 rather than +285/-34.

setXForBestConverge is void now — both callers only wanted the announcement — and the
reentrancy guard is gone: instrumented across the desktop suite the getter never re-enters
itself, since it answers 0 once mana has been spent and a converge cost never evaluates
Count$Converge. Payment solves are unchanged, counted per test: 1, 1, 5, 6 either way.

One of the tests was asserting a coin flip. convergeSizesATokenSpellOnTheXPath failed about
one run in five, always with Sweep the Skies still in hand and nothing tapped — TokenAi casts a
token spell TOKEN_GENERATION_ABILITY_CHANCE of the time, 80 on the default profile. It fixes
the random stream now and restores it after; six clean full-suite runs against two failures in
five before. Sorry for leaving that sitting in CI's way.

And one could not fail at all. convergeSizesTheSpellThatBoughtTheColours passed on master,
with the prediction severed, and on the branch. PermanentAi already reaches X=5 for Chamber
Sentry, so the branch swaps the mechanism without changing that card. Dropped; a replacement I
tried passes on master too.

Three left, each failing for its own reason: Bring to Light for the prediction, Painful Truths
for the DrawAi override this deletes, Sweep the Skies for the announcement.

Still happy to split the Sweep the Skies piece out — measured again on current master, it stands
alone without any of the prediction.

[tool4ever]

spacing looks off, also seems like a bad idea to build our tests around assumptions on such minor implementation details

[liamiak]

Spacing fixed.

On the seed: the test is checking that the AI sizes X by the colours it will pay (5 Thopters from Sweep the Skies). But on the default profile TokenAi only casts a token spell 80% of the time, so unseeded, one run in five the spell is never cast and the test fails without reaching the sizing. Pinning the random stream is there to take that coin flip out, not to assert anything about it.

Happy to delete this test if you'd rather not have it. If there's a cleaner way to force the cast, I'll switch to that.