[CLU] Implement Amzu, Swarm's Hunger and batches for zone change events #12686
Conversation
Mage/src/main/java/mage/abilities/common/CardsLeaveGraveyardTriggeredAbility.java
Show resolved
Hide resolved
Mage/src/main/java/mage/abilities/common/CardsLeaveGraveyardTriggeredAbility.java
Outdated
Show resolved
Hide resolved
| // Fire batch event for cards moving to stack as well | ||
| ZoneChangeBatchEvent batchEvent = new ZoneChangeBatchEvent(); | ||
| batchEvent.addEvent(info.event); | ||
| game.fireEvent(batchEvent); |
There was a problem hiding this comment.
Hmmmm.... why is this needed? That is, why does the ZoneChangeBatchEvent code from #11753 not cover it?
Any thoughts @JayDi85 or @DominionSpy ? It was previously stated that batch logic was correct and ZONE_CHANGE_GROUP should be deprecated in favor of ZONE_CHANGE_BATCH (see #11895 and #8009), but seems like it may have been incomplete. Is this change sufficient to cover all necessary usages?
There was a problem hiding this comment.
In this specific case, i found that the batch event wasn't being fired. I know there's code to collect simultaneous events and generate batch events for them, but they wouldn't apply here since things can't be cast simultaneously as far as i know.
Maybe some test cases should be written to check for batch zone change events firing 1:1 with single zone change events covering each instance of making a new zone change event? I could take a shot, but it sounds out of scope for this PR.
There was a problem hiding this comment.
// create a group zone change event if a card is moved to stack for casting (it's always only one card, but some effects check for group events (one or more xxx))
Need more research, I remember there were some use case with not fired batch or like that. Current code looks like a workaround, but it can be a more deep problem with firing and batching events logic (if same event can be called directly and simultaneously in diff use cases). I’ll try to research it.
There was a problem hiding this comment.
In that case, can split this PR and implement the card without reworking the trigger to the batch event. Sorry that ended up being nontrivial.
| // Fire batch event for cards moving to stack as well | ||
| ZoneChangeBatchEvent batchEvent = new ZoneChangeBatchEvent(); | ||
| batchEvent.addEvent(info.event); | ||
| game.fireEvent(batchEvent); |
There was a problem hiding this comment.
// create a group zone change event if a card is moved to stack for casting (it's always only one card, but some effects check for group events (one or more xxx))
Need more research, I remember there were some use case with not fired batch or like that. Current code looks like a workaround, but it can be a more deep problem with firing and batching events logic (if same event can be called directly and simultaneously in diff use cases). I’ll try to research it.
JayDi85
changed the title
#11517