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

Card: differ between Game and Layer Timestamp #4840

Merged
merged 4 commits into from
Mar 21, 2024
Merged

Conversation

Hanmac
Copy link
Contributor

@Hanmac Hanmac commented Mar 19, 2024

Smaller PR than #117

@tool4ever can you debug why exactly Kalitas is failing? and why only one of them?

@tool4ever
Copy link
Contributor

Hmn, I think this might come down to a logic error with the new changeZoneLKI structure
If we want to retrieve the previous LKI for Object X, then we need to use the game timestamp it had in its old zone
or in most cases nothing will be found...

@tool4ever
Copy link
Contributor

tool4ever commented Mar 20, 2024

So there might be two use cases for changeZoneLKI:

  • A - we have the new object and want to know its previous LKI
  • B - we reference the old object but want its "final" LKI (mostly because of undone statics)

I think A might be rarer?:

  1. Used by ReplacementHandler

B:
2. CardProperty: this gets used only partially with focus on controller and zone checks, I remember it fixed some things
3. DamageDeal effects: I think this fixes something like Lifelink by static missing e.g. if a creature dies before it's ability resolves
4. TriggerChangesZone DamageReceivedCondition$: I'll check that one, looks more like a incomplete LKI problem?

  1. some other less common parts

in your old branch you refactored 1. to avoid the usage, should still be safe
if we don't need A without it then that could be the complete fix for the test

@Hanmac
Copy link
Contributor Author

Hanmac commented Mar 20, 2024

"Used by ReplacementHandler" I need to look at this part

It probably can be changed to use LastStateBattlefield instead

Fix LKI for Replacement
@Hanmac
Copy link
Contributor Author

Hanmac commented Mar 20, 2024

@tool4ever so Replacement Handler is updated

@tool4ever
Copy link
Contributor

For triggers we have 2 "problem cases":

  • TriggerDestroyed - this uses old zone card, which means Karmic Justice incorrectly triggers for enchantments animated by Opalescence
  • TriggerSacrificed - this triggers with LKI, which means cards like It That Betrays need to use Defined$ TriggeredCard to work (which can be broken if you exile and then return it to Graveyard)

So they either

  • need both in runParams like TriggerChangesZone
  • some AbilityUtils logic that falls back to changeZoneLKI

But that's something that can be decided later

@Hanmac Hanmac marked this pull request as ready for review March 20, 2024 21:54
@Hanmac Hanmac merged commit abfb74e into master Mar 21, 2024
4 checks passed
@Hanmac Hanmac deleted the timestampReworkV2 branch March 21, 2024 08:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants