Improve PumpAi timing logic

docs/Card-scripting-API/AbilityFactory.md

@@ -1,4 +1,4 @@
-AbilityFactory parses differently from the Keyword parser. Your Ability line will look more like a collection of name-value pairs:
+AbilityFactory evaluates differently from the Keyword parser. Your Ability line will look more like a collection of name-value pairs:
 
 `A:<AB/SP/DB/ST>$ {AFSubclass} | <Necessary$ Parameters> | (<Separated$ By> | <Pipes$ Here>) | [Optional$ {Values} [Nested$ Dependency]]`
 
@@ -59,7 +59,8 @@ The SpellDescription for secondary abilities is displayed when (and if) the abil
 `StackDescription$ {String}` is the description the ability will have on the stack. This is automatically generated by the effect, but may be overridden using this parameter. This is sometimes needed with complex effects, when the generated text can't handle some details. Properties of the spell can be accessed like this: `{c:Targeted}`. You can reuse the spell text by just putting `SpellDescription` or `None` to leave it empty.
 
 ## Remember*
-Remembering is often needed when a card becomes a new object, which is then further affected by the ability. Typical example: [Flicker](https://github.com/Card-Forge/forge/blob/master/forge-gui/res/cardsfolder/f/flicker.txt)<br />
+Remembering is often needed when a card becomes a new object, which is then further affected by the ability. Typical example: [Flicker](https://github.com/Card-Forge/forge/blob/master/forge-gui/res/cardsfolder/f/flicker.txt)
+
 Because cards keep their remembered parts when changing zones manual [cleanup](#Cleanup) is usually required.
 
 ## Duration
@@ -81,9 +82,9 @@ Prevents text-changing effects from affecting the ability.
 Only needed if the card text doesn't contain the type/color words but the ability still needs to use them (e.g. "for each color ...").
 
 ## Generic AI params
-- `IsCurse$ True` - for effects that are normally treated positive e.g. Pump
+- `IsCurse$ True` - for effects that are normally treated positive (e.g. Pump) but some advanced behaviour (e.g. not obvious -X/-X) inverts that reasoning
 - `AICheckSVar$ {Count}`
-- `AILogic$ {String}` - this is a special param since it supports some globally available values, however many AI API classes also have very individual logic paths which will be mentioned in their sections 
+- `AILogic$ {String}` - this is a special param since it supports some globally available values to override parts of the default heuristics, however many AI API classes also have very individual logic paths which will be mentioned in their sections (usually when auto-detecting them is too difficult)
 - `AITgts$ BetterThanEvalRating.130` - Normally the AI will only prefer targeting cards that satisfy the constraint. However, you can add `AITgtsStrict$ True` if playing it should only happen when enough of these cards are available, e.g. *Rootwater Matriarch*.
 
 # Factories (in Alphabetical Order)

docs/Card-scripting-API/Card-scripting-API.md

@@ -1,7 +1,7 @@
 A reference guide for scripting cards using the API parsed by the Forge engine.
 
 # Base Structure
-By opening any file in the */res/cardsfolder* directory you can see the basic structure of how the data is created.  
+By opening any file in the */res/cardsfolder* directory you can see the basic syntax of how the data is created.  
 Here's an example of a vanilla creature:
 ```
 Name:Vanilla Creature
@@ -11,7 +11,7 @@ PT:2/2
 Oracle:
 ```
 
-* The name of this card is Vanilla Creature.
+* The name of this card is "Vanilla Creature".
 * It's casting cost is {2}{G}.
 * It has the types Creature and Beast.
 * It has a Power-Toughness of 2/2.
@@ -24,7 +24,7 @@ There are a few other properties that will appear in many cards. These can all b
 | Property | Description
 | - | -
 |`A`|[Ability effect](AbilityFactory.md)
-|`AI`|`RemoveDeck:`<br />* `All`<br />This will prevent the card from appearing in random AI decks. It is applicable for cards the AI can't use at all and also for cards that the AI could use, but only ineffectively. The AI won't draft these cards.<br />* `Random`<br /> This will prevent the card from appearing in random decks. It is only applicable for cards that are too narrow for random decks like *Root Cage* or *Into the North*. The AI won't draft these cards.<br />* `NonCommander`<br />
+|`AI`|`RemoveDeck:`<br />* `All`<br />This will prevent the card from appearing in random AI decks. It is applicable for cards the AI can't use at all and also for cards that the AI could use, but only ineffectively. The AI won't draft these cards and also avoid using them ingame if it somehow gains access to one.<br />* `Random`<br /> This will prevent the card from appearing in random decks. It is only applicable for cards that are too narrow for random decks like *Root Cage* or *Into the North*. The AI won't draft these cards.<br />* `NonCommander`<br />
 |`Colors`|Color(s) of the card<br /><br />When a card's color is determined by a color indicator rather than shards in a mana cost, this property must be defined. If no identifier is needed, this property should be omitted.<br /><br />Example:<br />`Colors:red,green` - Since *Arlinn, Embraced by the Moon* has no mana cost (it's the back of a double-faced card), the red and green indicator must be included.
 |`DeckHints`|AI-related hints for a deck including this card<br /><br />To improve synergy this will increase the rank of of all other cards that share some of its DeckHints types. The following types are supported:<br />* Color<br />* Keyword<br />* Name<br />* Type<br /><br />This helps with smoothing the selection so cards without these Entries won't be at an unfair disadvantage.<br /><br />The relevant code can be found in the [CardRanker](https://github.com/Card-Forge/forge/blob/master/forge-gui/src/main/java/forge/gamemodes/limited/CardRanker.java) class.
 |`DeckNeeds`|This can be considered a stronger variant when the AI should not put this card into its deck unless it has whatever other type is specified. The way this works is "inverted": it will directly decrease the rank of the card unless other cards are able to satisfy its types.<br />If a card demands more than one kind of type you can reuse it:<br />`DeckNeeds:Type$Human & Type$Warrior` will only find Human Warrior compared to `DeckNeeds:Type$Human\|Warrior` which is either
@@ -64,9 +64,7 @@ Examples:
 - Devoid
 - First Strike
 - Flash
-- Haste
 - Indestructible
-- Mentor
 - Provoke
 - Reach
 - Split second
@@ -118,7 +116,7 @@ Examples:
 - Ninjutsu:{cost}
 - Outlast:{cost}
 - Partner:{CardName}
-- Protection:{ValidCards}:{Description}
+- Protection:{ValidCards}:[Description]
 - Prowl:{cost}
 - Rampage:{magnitude}
 - Recover:{cost}
@@ -140,11 +138,9 @@ Only listing the most common ones here so you can recognize them.
 CARDNAME is replaced by the card's name ingame.
 
 - CARDNAME can't attack or block alone.
-- CARDNAME can't block unless a creature with greater power also blocks.
 - CARDNAME must be blocked if able.
 - Remove CARDNAME from your deck before playing if you're not playing for ante.
 - You may choose not to untap CARDNAME during your untap step.
-- CantSearchLibrary
 
 # General SVars
 * `SoundEffect:<file.mp3>`
@@ -164,7 +160,11 @@ number of instant and sorcery cards you own in exile and in your graveyard
 > Context switching
 
 # Common AI specific SVars
-* `AIEvaluationModifier:{ValidAmount}`
+* `AIEvaluationModifier:{XCount}`
+Each creature is assigned a score when AI decides which one to destroy/equip/etc. If the card has some exotic ability it may get judged less than it deserves, so this is a way to manually influence the resulting priority.
+
+* `AIPriorityModifier:{Integer}`
+Same reasoning as above, but applies to all abilities of a card (except lands) when AI decides which one to play.
 
 * `AIPreference:SacCost$Creature.token,Creature.cmcLE2`
 
@@ -220,6 +220,7 @@ SVar:Y:Count$Valid Creature.YouCtrl
 ```
 
 * `NonStackingEffect:True`
+This card's effect isn't cumulative, so AI won't bother with second copies.
 
 * `NoZeroToughnessAI:True`
 

forge-ai/src/main/java/forge/ai/AiCardMemory.java

@@ -66,7 +66,7 @@ public enum MemorySet implements MemoryType<Card> {
     }
 
     public enum MemorySetMana implements MemoryType<ManaCostBeingPaid> {
-        UNPAID_COSTS
+        UNPAID_COSTS // all partial costs AI couldn't pay, stored globally so individual effects don't need to run expensive checks themselves
     }
 
     public interface MemoryType<T> {

forge-ai/src/main/java/forge/ai/ComputerUtilAbility.java

@@ -432,12 +432,12 @@ private static int getSpellAbilityPriority(SpellAbility sa) {
             if (ApiType.DestroyAll == sa.getApi()) {
                 // check boardwipe earlier
                 p += 4;
-            } else if (ApiType.Mana == sa.getApi()) {
+            } else if (sa.isManaAbility()) {
                 // keep mana abilities for paying
                 p -= 9;
             }
 
-            // try to cast mana ritual spells before casting spells to maximize potential mana
+            // try to use mana ritual before casting spells to maximize potential mana
             if ("ManaRitual".equals(sa.getParam("AILogic"))) {
                 p += 9;
             }

forge-ai/src/main/java/forge/ai/SpellAbilityAi.java

@@ -520,6 +520,7 @@ protected boolean setAiEvaluationHost(final SpellAbility sa, final CardCollectio
         if (remember != null) {
             host.addRemembered(remember);
         }
+        // TODO addChangedSVars if Remembered is simply used to substitute some other non-Card field
         return true;
     }
 }

forge-ai/src/main/java/forge/ai/ability/PumpAi.java

@@ -3,11 +3,14 @@
 import com.google.common.collect.Lists;
 import com.google.common.collect.Maps;
 import forge.ai.*;
+import forge.card.CardType;
 import forge.game.Game;
 import forge.game.ability.AbilityUtils;
 import forge.game.ability.ApiType;
 import forge.game.card.*;
+import forge.game.combat.CombatUtil;
 import forge.game.cost.Cost;
+import forge.game.cost.CostTapType;
 import forge.game.keyword.Keyword;
 import forge.game.phase.PhaseHandler;
 import forge.game.phase.PhaseType;
@@ -87,19 +90,28 @@ protected boolean checkPhaseRestrictions(final Player ai, final SpellAbility sa,
     @Override
     protected boolean checkPhaseRestrictions(final Player ai, final SpellAbility sa, final PhaseHandler ph) {
         final Game game = ai.getGame();
-        boolean main1Preferred = "Main1IfAble".equals(sa.getParam("AILogic")) && ph.is(PhaseType.MAIN1, ai);
-        if (game.getStack().isEmpty() && sa.getPayCosts().hasTapCost()) {
-            if (ph.getPhase().isBefore(PhaseType.COMBAT_DECLARE_ATTACKERS) && ph.isPlayerTurn(ai)) {
-                return false;
-            }
-            if (ph.getPhase().isBefore(PhaseType.COMBAT_BEGIN) && ph.getPlayerTurn().isOpponentOf(ai)) {
-                return false;
+        if (game.getStack().isEmpty()) {
+            boolean isBeforeMyAttack = ph.getPhase().isBefore(PhaseType.COMBAT_DECLARE_ATTACKERS) && ph.isPlayerTurn(ai);
+            boolean isBeforeOppCombat = ph.getPhase().isBefore(PhaseType.COMBAT_BEGIN) && ph.getPlayerTurn().isOpponentOf(ai);
+            CostTapType tapType = sa.getPayCosts().getCostPartByType(CostTapType.class);
+            if (tapType != null && (tapType.getType().startsWith("Creature") || CardType.isACreatureType(tapType.getType()))) {
+                return isBeforeMyAttack || isBeforeOppCombat;
+            }
+            Card host = sa.getHostCard();
+            // wait until AI has decided if creature should attack/block instead
+            if (host.isCreature() && sa.getPayCosts().hasTapCost()) {
+                if (isBeforeMyAttack && CombatUtil.canAttack(host)) {
+                    return false;
+                }
+                if (isBeforeOppCombat && CombatUtil.canBlock(host)) {
+                    return false;
+                }
             }
         }
         if (game.getStack().isEmpty() && (ph.getPhase().isBefore(PhaseType.COMBAT_BEGIN)
                 || ph.getPhase().isAfter(PhaseType.COMBAT_DECLARE_BLOCKERS))) {
-            // Instant-speed pumps should not be cast outside of combat when the
-            // stack is empty
+            boolean main1Preferred = "Main1IfAble".equals(sa.getParam("AILogic")) && ph.is(PhaseType.MAIN1, ai);
+            // save tricks until the last moment
             return sa.isCurse() || isSorcerySpeed(sa, ai) || main1Preferred;
         }
         return true;

forge-game/src/main/java/forge/game/spellability/SpellAbilityProperty.java

@@ -158,7 +158,7 @@ public static boolean hasProperty(SpellAbility sa, Player sourceController, Card
             if (root instanceof WrappedAbility wa) {
                 root = wa.getWrappedAbility();
             }
-            if (!sa.equals(Objects.requireNonNullElse(root.getOriginalAbility(), root))) {
+            if (!sa.getRootAbility().equals(Objects.requireNonNullElse(root.getOriginalAbility(), root))) {
                 return false;
             }
         } else if (property.equals("LastChapter")) {

forge-gui/res/cardsfolder/e/emrakul_the_world_anew.txt

@@ -3,7 +3,7 @@ ManaCost:12
 Types:Legendary Creature Eldrazi
 PT:12/12
 K:Flying
-K:Protection:Spell.ThisTurnCast:spells that were cast this turn
+K:Protection:Spell:spells
 K:Protection:Permanent.ThisTurnCast:permanents that were cast this turn
 T:Mode$ SpellCast | ValidCard$ Card.Self | Execute$ TrigGainControl | TriggerDescription$ When you cast this spell, gain control of all creatures target player controls.
 SVar:TrigGainControl:DB$ GainControl | ValidTgts$ Player | TgtPrompt$ Select target player to gain control of all of their creatures | AllValid$ Creature.TargetedPlayerCtrl | NewController$ You