[don't merge] Fix saga first chapter zcc

[JayDi85]

This is a cleaned version of #11619, discussion in #13710

This makes it so that if a permanent is entering as the ability's zcc is being updated, then the zcc is the permanent's instead of the spell's. An ability that happens "as ~ enters" will not have a changed zcc, as it isn't put into the entering list until after those abilities have been processed. The new SigardasSplendorTest verifies this behavior.

This also centralizes the updating of ability zcc, though addDelayedTriggeredAbility is unchanged due to to d8959f1. The only logic change is in the getPermanentEntering check.

Fixes #11728

[Grath]

Can we please just merge this? It's blatantly the correct approach and this bug is blocking me from making a [[Garnet, Princess of Alexandria]] deck because of saga first chapters having bugs with the ZCC, which will be encountered much more frequently with a commander that specifically removes lore counters from sagas.

[github-actions]

Garnet, Princess of Alexandria - (Gatherer) (Scryfall) (EDHREC)

{G}{W}
Legendary Creature — Human Noble Cleric
2/2
Lifelink
Whenever Garnet attacks, you may remove a lore counter from each of any number of Sagas you control. Put a +1/+1 counter on Garnet for each lore counter removed this way.

[xenohedron]

@JayDi85 what are you trying to accomplish by reverting necessary fixes? None of the other devs understand the downside of merging this change. Your obstinate behavior is not serving the users.

[JayDi85]

To fix original problem instead introduce new lock hacks that will interfere with it

[ssk97]

To fix original problem instead introduce new lock hacks that will interfere with it

What do you see as the original problem? The abilities must not use the Saga card's new zcc if it's moved before the ability resolves. This change is short and contained to a single function so if a better solution is found this will be easy to remove.

I agree that this isn't the perfect solution, but the only better solution I could think of was to make the change affect all "As ~ enters" abilities. That would make some abilities' implementations a little cleaner but it seemed like a lot of work to update all the relevant cards for no benefit since that zcc is consistently the spell's so there's no bugs for that change to fix.

[theelk801]

I agree, this really needs to be merged already

[Grath]

I'm manually cherrypicking this commit into my weekly build. If I need to, I will continue doing so but if (when) everything works correctly tonight I intend to re-merge this because literally every developer except JayDi wants this merged, and JayDi - if you come up with a better solution, you can just as easily fix forward rather than insisting that a substantial bug remains in the program indefinitely while you think about it.

[Grath]

to be precise: Saga first chapters MUST function the same whether they trigger while the saga is entering the battlefield or when it is already on the battlefield, particularly due to Garnet being printed which explicitly is designed to let you indefinitely re-use Sagas, and this simple fix which is restricted to one function makes the zone change counter of an ability that triggers when a permanent enters the battlefield be the zone change counter of the permanent as it exists on the battlefield, which makes logical sense. Perhaps this fix will be mooted by the outcome of #13737 but for now, we should actually fix the bug rather than sitting on [don't merge].

[Grath]

Seeing no further development on this from JayDi, I intend to merge this on Thursday unless he comes up with an alternative solution AND merges it by then.

[JayDi85]

@Grath nope, don't merge. I'm finishing more priority task with targeting rework and AI stability fixes for new release. After that I'll take current PR.

[Grath]

Or, hear me out on this crazy idea: We merge this so that the bugs stop happening, with Final Fantasy introducing a lot of new Sagas and Saga-related cards - if you do a release of the Final Fantasy cards without fixing this bug, there WILL be a bunch of bug reports of this same issue - and then when you have free time you can investigate a better solution and/or the code ends up being reworked anyways during the course of #13737

[ssk97]

Testing on the Alpha server uncovered a bug with token sagas. This has now been fixed. It's in the same spot as the rest of this zcc fix, potentially the token zcc itself could be what is fixed instead.

[Susucre]

Testing on the Alpha server uncovered a bug with token sagas. This has now been fixed. It's in the same spot as the rest of this zcc fix, potentially the token zcc itself could be what is fixed instead.

I would be in favor of having the token zcc change when a spell copy becomes a permanent. For a long time copying spells on the stack was reserved to non-permanent and copying permanent spells (that become token permanents with 608.3f). Is there something requiring that tokens have zcc at 0?
Do you know how copy of permanent card being cast behave relative to zcc? Stuff like [[Kaervek, the Punisher]]. Keeping it as close as possible as the non-copy zcc handling would prevent future problems, if that is not already aligned.

[github-actions]

Kaervek, the Punisher - (Gatherer) (Scryfall) (EDHREC)

{1}{B}{B}
Legendary Creature — Human Warlock
3/3
Whenever you commit a crime, exile up to one target black card from your graveyard and copy it. You may cast the copy. If you do, you lose 2 life. (Targeting opponents, anything they control, and/or cards in their graveyards is a crime. Copies of permanent spells become tokens.)

[ssk97]

I would be in favor of having the token zcc change when a spell copy becomes a permanent. For a long time copying spells on the stack was reserved to non-permanent and copying permanent spells (that become token permanents with 608.3f). Is there something requiring that tokens have zcc at 0? Do you know how copy of permanent card being cast behave relative to zcc? Stuff like [[Kaervek, the Punisher]]. Keeping it as close as possible as the non-copy zcc handling would prevent future problems, if that is not already aligned.

Token copies currently copy the source's zcc, plus one. However, this zcc is updated while the token is being created before it is put into the EnteringPermanents list, whereas normal permanents get their zcc updated after that. A cast permanent copy should behave the same as copying the spell on the stack. AFAIK the zcc behavior involved with being put onto the battlefield is the same between token copies and non-copied tokens.

[Grath]

Token copies currently copy the source's zcc, plus one. However, this zcc is updated while the token is being created before it is put into the EnteringPermanents list, whereas normal permanents get their zcc updated after that. A cast permanent copy should behave the same as copying the spell on the stack. AFAIK the zcc behavior involved with being put onto the battlefield is the same between token copies and non-copied tokens.

Alternative thought: What about updating the ZCC of non-token sources before they're put into the EnteringPermanents list, and then instead of "adjust the ZCC slightly-jankily" it's just "Entering Permanents have the ZCC of being on the battlefield already"?

[ssk97]

Alternative thought: What about updating the ZCC of non-token sources before they're put into the EnteringPermanents list, and then instead of "adjust the ZCC slightly-jankily" it's just "Entering Permanents have the ZCC of being on the battlefield already"?

I've considered that possibility, the problem with it is that it would require a rework of hundreds of cards like [[Sigarda's Splendor]] where the "as ~ enters" code runs before the permanent is placed on the EnteringPermanents list.

... Actually, I should check that token copies of cards like those aren't currently broken in master.

[github-actions]

Sigarda's Splendor - (Gatherer) (Scryfall) (EDHREC)

{2}{W}{W}
Enchantment
As this enchantment enters, note your life total.
At the beginning of your upkeep, draw a card if your life total is greater than or equal to the last noted life total for this enchantment. Then note your life total.
Whenever you cast a white spell, you gain 1 life.

[Grath]

Now that we've finally gotten a beta update after six months of people asking "why is this card missing", please can we just merge this for the next update and then you can evaluate a possible better solution at your leisure @JayDi85 ?

[Grath]

Two more bug reports in Discord that would be resolved by just merging this. Unless you come back with at least a proposal for what you're going to do instead of this, I plan on merging this on Thursday the 16th.

[JayDi85]

@Grath report new bugs here or create new issues about it. Discord refs are useless here.

[Grath]

It's two more reports of the bug fixed by this pull request - Summon Ixion (and a bunch of other Sagas) don't work right.

[xenohedron]

It is not accurate to label this PR as a draft. It is a verified and necessary fix. Nothing else is required for it to function. Players have been continually reporting bugs that this PR fixes. Five other devs have endorsed it. Three of those devs have independently added the commit to the builds they maintain, and those builds have been successfully played for months now.

[JayDi85]

I'll research it on current weekend and will merge it (if no better solution).

[Grath]

If you don't merge it this weekend, I will merge it on Monday.