Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Refactor Batch TriggeredAbility #12173

Open
wants to merge 17 commits into
base: master
Choose a base branch
from
Open

Refactor Batch TriggeredAbility #12173

wants to merge 17 commits into from

Conversation

Susucre
Copy link
Contributor

@Susucre Susucre commented Apr 23, 2024 •
edited

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:

  • Add in verify tests that Triggered Ability checking batch events implement the BatchTriggeredAbility interface. (That's the intent with the boolean added on EventType)
  • Do the same change for others Batch events
  • 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 Susucre changed the title Refactor batch triggers Refactor Batch TriggeredAbility Apr 23, 2024
@Susucre
Copy link
Contributor Author

Susucre commented Apr 23, 2024

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

image

@xenohedron
Copy link
Contributor

I like the direction here.

@Susucre Susucre marked this pull request as ready for review April 23, 2024 21:46
@Susucre
Copy link
Contributor Author

Susucre commented Apr 23, 2024

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.

@xenohedron
Copy link
Contributor

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

@Susucre
Copy link
Contributor Author

Susucre commented Apr 27, 2024

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
Copy link
Contributor Author

Susucre commented Apr 28, 2024

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.

@github-actions GitHub Actions
Copy link

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.

@Susucre
Copy link
Contributor Author

Susucre commented Apr 28, 2024

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.

@github-actions GitHub Actions
Copy link

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

@Susucre Susucre force-pushed the refactor-batch-triggers branch 2 times, most recently from b409e99 to 85e84de Compare April 28, 2024 12:24
This was referenced Apr 28, 2024
Open
Open
@Susucre Susucre mentioned this pull request Apr 30, 2024
Open
xenohedron
xenohedron reviewed May 1, 2024
View reviewed changes
Mage/src/main/java/mage/abilities/BatchTriggeredAbility.java
* <p>
* Note: if calling this, make sure the event does pass TriggeredAbility::checkEventType
*/
Stream<T> filterBatchEvent(GameEvent event, Game game);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer if we could split the stream logic and the filter logic into separate methods, something like:

boolean checkEvent(T event, Game game);

default Stream<T> filterBatchEvent(BatchEvent<T> event, Game game) {
    return event.getEvents()
                .stream()
                .filter(e -> checkEvent(e, game))
}

with the checkTrigger methods starting with

filterBatchEvent((TypeOfBatchEvent) event, game)

but I'm not sure, will the generic types work for this sort of usage?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uhm I'll try something like that. May need another method to do the event casting a safe way, as the NUMBER_OF_TRIGGERS will not have direct access to the batch event type.

Mage/src/main/java/mage/abilities/common/CombatDamageDealtToYouTriggeredAbility.java
.getEvents()
.stream()
.filter(DamagedPlayerEvent::isCombatDamage)
.filter(e -> e.getPlayerId() == getControllerId());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

compare uuid with .equals

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I usually get those right those days. Good catch.

Mage/src/main/java/mage/abilities/common/OneOrMoreDealDamageTriggeredAbility.java
@@ -59,12 +61,12 @@ public boolean checkEventType(GameEvent event, Game game) {
}

@Override
public boolean checkTrigger(GameEvent event, Game game) {
public Stream<DamagedPlayerEvent> filterBatchEvent(GameEvent event, Game game) {
DamagedBatchForOnePlayerEvent dEvent = (DamagedBatchForOnePlayerEvent) event;
if (onlyCombat && !dEvent.isCombatDamage()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wouldn't it make more sense for these sorts of checks (on info about the batch event rather than the sub events) to be in checkTrigger before calling this method for the stream?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I'll do a cleanup pass for the combat damage, and the target (resp. source) when the Batch event is one that has a shared target (resp. source)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JayDi85 JayDi85 JayDi85 left review comments

@xenohedron xenohedron xenohedron left review comments

Assignees
No one assigned
Labels
cards engine tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Kaya Spirits' Justice
4 participants
@Susucre @xenohedron @JayDi85 @PurpleCrowbar