-
Notifications
You must be signed in to change notification settings - Fork 745
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
Refactor Batch TriggeredAbility #12173
base: master
Are you sure you want to change the base?
Conversation
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 |
ca79e0b
to
569546c
Compare
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. |
Jace, Cunning Castaway - (Gatherer) (Scryfall) (EDHREC)
|
Alright, was only missing two due to some blind spot of the Verify Test: Jace, Cunning Castaway and [[Tamiyo, Field Researcher]]. |
Tamiyo, Field Researcher - (Gatherer) (Scryfall) (EDHREC)
|
b409e99
to
85e84de
Compare
f6b1824
to
2b55e94
Compare
* <p> | ||
* Note: if calling this, make sure the event does pass TriggeredAbility::checkEventType | ||
*/ | ||
Stream<T> filterBatchEvent(GameEvent event, Game 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.
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?
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.
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.
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.
Yes, must use overrideable method like filterEvent
from xenohedron above -- so all implementation will override only simple boolean method and no needs in streams at all. Also I recommends to return List in find method, not stream -- it's a standard code style for xmage.
.getEvents() | ||
.stream() | ||
.filter(DamagedPlayerEvent::isCombatDamage) | ||
.filter(e -> e.getPlayerId() == getControllerId()); |
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.
compare uuid with .equals
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 usually get those right those days. Good catch.
@@ -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()) { |
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.
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?
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.
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)
2b55e94
to
02bf101
Compare
02bf101
to
fe3e2f0
Compare
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. |
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.
Checked game engine changes only (without abilities logic). I prefer to use a shared BatchTriggeredAbility class instead interface. See related comments with example.
@@ -2426,15 +2447,31 @@ public boolean chooseTarget(Outcome outcome, Target target, Ability source, Game | |||
if (!targetDefinition.startsWith("targetPlayer=")) { | |||
continue; | |||
} | |||
checkTargetDefinitionMarksSupport(target, targetDefinition, "="); | |||
String playerName = targetDefinition.substring(targetDefinition.indexOf("targetPlayer=") + 13); |
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.
String target = "targetPlayer="; | ||
for (int i = 0; i < targetPlayers.length; ++i) { | ||
if (i > 0) { | ||
target += "^"; |
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 can cause bugs (see above about dynamic targets) and must be removed. Or use addTarget per player instead ^ here.
@@ -1719,6 +1719,10 @@ private void compareClassRecursive(Object obj1, Object obj2, Card originalCard, | |||
} | |||
} | |||
|
|||
if (obj1 instanceof Ability) { | |||
checkAbility(originalCard, (Ability) obj1, msg); |
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.
* <p> | ||
* Note: if calling this, make sure the event does pass TriggeredAbility::checkEventType | ||
*/ | ||
Stream<T> filterBatchEvent(GameEvent event, Game 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.
Yes, must use overrideable method like filterEvent
from xenohedron above -- so all implementation will override only simple boolean method and no needs in streams at all. Also I recommends to return List in find method, not stream -- it's a standard code style for xmage.
|
||
@Override | ||
public boolean checkTrigger(GameEvent event, Game game) { | ||
return filterBatchEvent(event, game).findAny().isPresent(); |
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.
See above about List. Example:
BatchTriggeredAbility
extendsTriggeredAbilityImpl
;- it's has default method
filterEvent
withreturn true
; - it's has final method like
List findFilteredEvents(Event, Game)
that calls implementations offilterEvent
; - it's has default implementation of
checkTrigger
thatreturn !findFilteredEvents(event, game).isEmpty
;
- it's has default method
- All implementations of
BatchTriggeredAbility
:- overrides
filterEvent
method to filter only needed; - override
checkTrigger
method to setup dynamic targets or other additional code.
- overrides
That's all will work same way as other triggered abilities, but with additional middleware (filterEvent);
P.S. I don't like generic usages all around. If it possible to use standard classes then it will be better (extends basic ability, extends basic event, etc). If it (generic) can simplify some code then can be keep.
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.
Well I won't do all that additional work that doesn't accomplish much.
So either someone else take the PR from here, or we can close it and leave the bugs.
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 guess I'll work on that then.
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:
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