mage
mage copied to clipboard
Refactor Batch TriggeredAbility
This is doing in a very limited scope the changes explained #12095, in order to have Felix Five-Boots (and next the others NUMBER_OF_TRIGGERS triggers) work properly with batch events.
TODO:
- [x] Add in verify tests that Triggered Ability checking batch events implement the BatchTriggeredAbility interface. (That's the intent with the boolean added on EventType)
- [x] Do the same change for others Batch events
- [x] Rebase and rework the two MILLED Batch events
- Outside of scope of current PR, fix the other NUMBER_OF_TRIGGERS using the Felix' code pattern.
Note: I have Felix's PR code here (tweaked with the changes), but just to make sure the refactor actually works (it does). Can clean that part if want to merge separately.
Fix #12193
New verify test check: non-BatchTriggeredAbility should not accept the EventType that are flagged as batch ones.
I like the direction here.
Alright, all batch events have been reworked.
I plan to add some tests in the next few days, in particular for the triggers I've consolidated into common class.
I wonder if it would be cleaner to have a separate "check event" method that can be a single filter
call in the stream, rather than nesting all the logic
Added some tests (extended the Test Suite to be allow to choose the players for Sower of Discord), and fixed Kaya & wrong sacrifice effect for Phyrexian Totem & Phyrexian Negator.
Should be good to merge. Can merge the Felix's PR first, it is altered here to fix it. The final step will be done later on to fix the 10-ish NUMBER_OF_TRIGGERS cards just like Felix, to be working properly with batch events.
Uhm so DelayedTriggeredAbility probably need the same kind of work around Batch Events. For instance [[Jace, Cunning Castaway]] +1's is broken since it checks the batch event source and not the individual events.
Jace, Cunning Castaway - (Gatherer) (Scryfall) (EDHREC)
{1}{U}{U} Legendary Planeswalker — Jace 3 +1: Whenever one or more creatures you control deal combat damage to a player this turn, draw a card, then discard a card. −2: Create a 2/2 blue Illusion creature token with "When this creature becomes the target of a spell, sacrifice it." −5: Create two tokens that are copies of Jace, Cunning Castaway, except they're not legendary.
Alright, was only missing two due to some blind spot of the Verify Test: Jace, Cunning Castaway and [[Tamiyo, Field Researcher]].
I did look manually at all cards having their own DelayedTriggerAbility
, so hopefully did not miss any other. I like those sanity checks to be automated as much as possible, but here there is no easy solution except with another big refactor, so way out of scope.
Tamiyo, Field Researcher - (Gatherer) (Scryfall) (EDHREC)
{1}{G}{W}{U} Legendary Planeswalker — Tamiyo 4 +1: Choose up to two target creatures. Until your next turn, whenever either of those creatures deals combat damage, you draw a card. −2: Tap up to two target nonland permanents. They don't untap during their controller's next untap step. −7: Draw three cards. You get an emblem with "You may cast spells from your hand without paying their mana costs."
I've played with this in alpha for a few weeks (dozens of cube games notably) without any noticeable issue.
I'll wait for next beta to merge it, just for extra security, and give it a last pass on small code cleanup before then.
Once that's done, I'll rework the tens or so of NUMBER_OF_TRIGGERS cards to handle batch triggers properly, and be unit tested individually.