-
Notifications
You must be signed in to change notification settings - Fork 741
Consider more cards playable in main1 (#8589) #8590
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Consider more cards playable in main1 (#8589) #8590
Conversation
eeb03f4
to
9fbd606
Compare
final String triggerZones = trigger.getParam("TriggerZones"); | ||
if (!"Battlefield".equals(triggerZones)) continue; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not all Trigger has TriggerZones set (unless you want to exclude ETB and LTB on purpose)
like for ChangedZone Trigger, it is set there, to the getActiveZone
set:
protected void correctZones() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Afaik a permanent with TriggerZones "Battlefield" catches any other card entering or leaving the battlefield, and cards with ETBs are handled elsewhere.
// 1312+ cards match `Mode\$ ChangesZone.*?TriggerZones\$ Battlefield` | ||
// enter/leave triggers | ||
// Admonition Angel etc | ||
if (mode == TriggerType.ChangesZone || mode == TriggerType.ChangesZoneAll) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not all ETB trigger would be positive
Like Hunted Bonebrute
would create tokens under opponents control, and would be better to be played after Combat?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think a human deck creator would have anticipated that, but the AI is probably less discerning. I'll see if there are more negative than positive effects or the negative ones can be filtered out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check if the AI would play the Triggered Ability if it wasn't mandatory
this should filter out the bad ones
(see getDamageFromETB
and checkETBEffects
)
Also see AiController.doTrigger
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hunted Bonebrute
matches Mode\$ ChangesZone.*?(?!TriggerZones\$ Battlefield)
and such ETBs are handled elsewhere according to #8590 (comment)
// 1312+ cards match `Mode\$ ChangesZone.*?TriggerZones\$ Battlefield` | ||
// enter/leave triggers | ||
// Admonition Angel etc | ||
if (mode == TriggerType.ChangesZone || mode == TriggerType.ChangesZoneAll) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
too generic, besides PermanentCreatureAi
already checks hasETBTrigger()
if you're worried about Landfall try improving the logic around PlayBeforeLandDrop
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PlayBeforeLandDrop
still makes sure to drop land later in main1.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is Ajani's Chosen worth playing in M1? Ajani's Welcome is marked as such, maybe because it's only 1 mana.
|
||
// 1009+ cards match `Mode\$ SpellCast.*?ValidActivatingPlayer\$ You` | ||
// Aetherflux Reservoir etc | ||
if (mode == TriggerType.SpellCast && "You".equals(trigger.getParam("ValidActivatingPlayer"))) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sequence wise this function should decide if there is something important enough to cast before combat (vs. leaving mana open for combat tricks)
Aetherflux Reservoir does not qualify, it should just be cast before other spells ideally so that's more of a use case for AIPriorityModifier
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spellslinger decks run such and prowess, right? So why not play Aetherflux Reservoir in main1?
|
||
// 9+ cards match `S:Mode\$ IgnoreLegendRule` | ||
// allow cloning your commander with Mirror Gallery; Myriad etc | ||
if (mode.contains(StaticAbilityMode.IgnoreLegendRule)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
too situational as default:
would at least need to check you have no non-legendary
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Etherium Sculptor is not legendary. Your suggested addition breaks this combo: https://www.youtube.com/shorts/KlYmlTzA9kw?feature=share
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's nice but we're not adding very broad checks in general AI logic in the hopes of maybe playing a deck with some relevant combo...
FWIW allow me to demonstrate how you're managing to break very basic AI behaviour:
in case it's not clear enough - thanks to your change AI would prefer to lose from attack next turn instead of grabbing the win so Mirror Gallery can look threatening 💩
it's up to you to ensure you're not making too many such assumptions, otherwise your changes aren't mergeable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the example. Can't that be solved by getSpellAbilityPriority
?
// 3+ cards match `S:Mode\$ ActivateAbilityAsIfHaste.*ValidCard\$[^|]*You` | ||
// Thousand-Year Elixir etc | ||
if (mode.contains(StaticAbilityMode.ActivateAbilityAsIfHaste)) { | ||
if (validCard.contains("You")) return true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
too inaccurate
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only 3 cards match. Why put any in the deck if they're not useful? card.isAbilitySick()
checks for this very ability.
|
||
// 43+ cards match `K:Bestow` | ||
final String bestow = nma.getParam("Bestow"); | ||
if (bestow != null && bestow.equals("True")) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's the point of casting in main1 just because it has Bestow?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because Bestow is like an aura, and auras are already played in main1 because they boost creatures.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
then you should test it
your logic does nothing because it's gonna be treated as Attach spell
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Attach spells are also permanents, but I see Bestow is already checked in attachAIPumpPreference
.
|
||
// 201+ cards match `S:Mode\$ ReduceCost.*?Activator\$ You` | ||
// reduce cost to enable more plays | ||
if (mode.contains(StaticAbilityMode.ReduceCost) && "You".equals(sta.getParam("Activator"))) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
again not worth the risk:
ai might just waste mana before combat
maybe they should get a slight boost in getSpellAbilityPriority()
though (at least during early game)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mana rocks are typically played first, and cost reduction should be even better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you aren't understanding what I'm saying:
this function is for "main1 before main2"
hijacking it to add your "permanent A before B" logic just isn't supposed to happen here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean? Don't you think Alisaie Leveilleur
, and Jet Medallion
are worth playing in M1?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, Jet Medallion does not help your combat directly
you'd need to at least be able to afford two additional playMain1 spells just to reach break even on mana spent vs. the risk of tapping out too early
this means on average it's a bad play
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, but Auriok Steelshaper would help equip and is a creature, and Ballyrush Banneret is a creature as well.
Is spending 1W in M1 worth a body and a 1 discount on Kithkin and Soldier spells? Deck-dependent.
Blood Funnel can pay for itself in monoblack, but requires creatures, which is probably checked when deciding to play it. (as well as it drawbacks). Fluctuator and Catalyst Stone also pay for themselves in the right decks. But all these have AI:RemoveDeck:Random
I'll leave ReduceCost to the card's SVar:PlayMain1
.
9fbd606
to
e055ca7
Compare
Fixes #8589. My Naya deck win rate improved by at least 50% when AI played it against its unpatched self.
I moved 0-mana, Exalted, and Extort checks before the RaidTest check as i don't think they reduce the number of blockers. The checks after RaidTest probably apply to cards that share that property with e.g. Blitz. I'm too tired to check atm.