mage icon indicating copy to clipboard operation
mage copied to clipboard

Refactor Batch TriggeredAbility

Open Susucre opened this issue 10 months ago • 10 comments

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

Susucre avatar Apr 23 '24 07:04 Susucre

New verify test check: non-BatchTriggeredAbility should not accept the EventType that are flagged as batch ones.

image

Susucre avatar Apr 23 '24 09:04 Susucre

I like the direction here.

xenohedron avatar Apr 23 '24 13:04 xenohedron

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.

Susucre avatar Apr 23 '24 21:04 Susucre

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

xenohedron avatar Apr 25 '24 02:04 xenohedron

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.

Susucre avatar Apr 27 '24 16:04 Susucre

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.

Susucre avatar Apr 28 '24 09:04 Susucre

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.

github-actions[bot] avatar Apr 28 '24 09:04 github-actions[bot]

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.

Susucre avatar Apr 28 '24 10:04 Susucre

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."

github-actions[bot] avatar Apr 28 '24 10:04 github-actions[bot]

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.

Susucre avatar May 21 '24 12:05 Susucre