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.

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

Fix Awakened Skyclave & Grond, The Gatebreaker #13229

Merged
merged 2 commits into from
Jan 15, 2025

Conversation

Cguy7777
Copy link
Contributor

At first glance, it looked like this would be a quick fix: just specify CardType.LAND as the card type to add in Awakened Skyclave. However, after doing so, it still did not become a land when on the battlefield.

It turned out that AddCardTypeSourceEffect was unnecessarily checking the affectedObjectList for durations where that was not needed. Adding a check to bypass this fixed the issue. I discovered that Grond, the Gatebreaker was also affected by this: if you controlled an Army during your turn, it would not continue being an artifact creature after zone changes.

I also added checks to ensure that AddCardTypeSourceEffect (and the similar AddCardTypeTargetEffect) require at least one card type to be supplied to them, and that dependencies with Blood Moon are handled properly when CardType.LAND is supplied.

Closes #13225.

@@ -45,7 +50,8 @@ public void init(Ability source, Game game) {
@Override
public boolean apply(Game game, Ability source) {
Permanent permanent = game.getPermanent(source.getSourceId());
if (permanent != null && affectedObjectList.contains(new MageObjectReference(permanent, game))) {
if (permanent != null
&& (affectedObjectList.contains(new MageObjectReference(permanent, game)) || !duration.isOnlyValidIfNoZoneChange())) {
Copy link
Member

Choose a reason for hiding this comment

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

For info:

I don't like existing of isOnlyValidIfNoZoneChange at all. It's wrongly mark effects with WhileOnBattlefield as independent from zone change, but it depends in reality. isOnlyValidIfNoZoneChange used for effect discard only:
shot_250114_125604

Original problem in continues effects logics -- it's init on game start, not on spell resolve. So AddCardTypeSourceEffect->affectedObjectList will contains card's zcc, not a permanent. That's why apply failed after cast.

Good implementation for complex multi-purpose effects (that can be used in both activated and static abilities) is GainAbilityTargetEffect -- it use waitingCardPermanent and add real permanent to affected list on resolve/dynamic.

Even such implementation is bad due too complex logic -- it's better to split such effects in two.

Anyway duration.isOnlyValidIfNoZoneChange() usage as workaround is fine here, but you must add comments about the reason (example: workaround to support abilities like as long as xxx is on the battlefield).

P.S. BTW there are other effects with same buggy logic in static ability usage: AddCardSubTypeSourceEffect and others, search code by:

  • affectedObjectList.add(new MageObjectReference(source.getSourceId(), game));

@JayDi85 JayDi85 merged commit 5d4112c into magefree:master Jan 15, 2025
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants