magefree / magefree/mage

Zone param in OrTriggeredAbility

Open
#11,279 0 comments 0 reactions 0 assignees View on GitHub
Developers Discussion refactoring
Dominant language
Java
Stars
2.4k
Forks
940
Avg merge
2d 12h
Merged PRs (30d)
160

Description

This occurred to me while reviewing Crack in Time:

https://github.com/magefree/mage/blob/00121021012ed1b35537a069b3ddd47e5cfaa22e/Mage.Sets/src/mage/cards/c/CrackInTime.java#L38-L44

EntersBattlefieldTriggeredAbility needs Zone.ALL due to the following comment (I haven't fully investigated why, taking it for granted), so I was thinking the OrTriggeredAbility might also.

https://github.com/magefree/mage/blob/a9fce4b05e3794c85ee7a85e7c87f61eb8fc77a8/Mage/src/main/java/mage/abilities/common/EntersBattlefieldTriggeredAbility.java#L18-L22

The zone is referenced in `isInUseableZone()`, which is called early on in `TriggeredAbilities::checkTrigger`, which would cause it to return there before calling the `checkTrigger` method of the individual ability.

However, I believe if we use Zone.ALL in `OrTriggeredAbility` then the zones of the child abilities don't end up getting checked. So unless I'm misunderstanding the logic (there are a lot of layers here, so it's possible), there's no parameter choice that would give correct behavior.

I think we have to rework this structure so that `OrTriggeredAbility` overrides `isInUseableZone()` to figure it out from its child abilities rather than having its own Zone parameter. Doing this correctly might be quite tricky.

Comments welcome.

Contributor guide

No contributing guide indexed for this repository

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.