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

Recent ETB Changes causes problems with LKI #668

Closed
Hanmac opened this issue Jun 12, 2022 · 8 comments
Closed

Recent ETB Changes causes problems with LKI #668

Hanmac opened this issue Jun 12, 2022 · 8 comments
Assignees

Comments

@Hanmac
Copy link
Contributor

Hanmac commented Jun 12, 2022

This Section there by #663 causes problems:

https://github.com/Card-Forge/forge/blob/master/forge-game/src/main/java/forge/game/ability/AbilityUtils.java#L223-L225

Without the setting for LKI the test fails, but in game it works as it should
With the setting for LKI the tests pass, but in game it becomes broken

Can one of you guys check what should be done there? @Agetian @tool4ever @Northmoc
somehow fix the tests?

@Hanmac Hanmac self-assigned this Jun 12, 2022
@tool4ever
Copy link
Contributor

This one is also caused by the same PR but not fixed by that line:

image

@Hanmac
Copy link
Contributor Author

Hanmac commented Jun 12, 2022

the trigger is probably something else to be fixed too, it should only indirect related to this issue

@kevlahnota
Copy link
Contributor

This one is also caused by the same PR but not fixed by that line:

image

I pushed 613 snaps that reverts #663 on my branch, so users will stop spamming the etb issue on discord. Gonna update future snaps until this is resolved.

@Hanmac
Copy link
Contributor Author

Hanmac commented Jun 13, 2022

i also need to check if this part is still valid, and if it is maybe it can be done better:

if (toBattlefield && !copied.getEtbCounters().isEmpty()) {
    for (final ReplacementEffect re : copied.getReplacementEffects()) {
       re.setSuppressed(true);
    }
}

i tested it with this line enabled or disabled, it doesn't seems to have a difference

@Hanmac
Copy link
Contributor Author

Hanmac commented Jun 15, 2022

fixed the bugs for now

but there might be some corner cases when there is something like Grumgully that does additional +1/+1 counters
and then two Corpsejack Menace (what replaces the amount of counters are put onto it) enter at the same time.

the corner case there would be that they only be affected by themselves (i think) and not the other Corpsejack Menace.

@Hanmac
Copy link
Contributor Author

Hanmac commented Jun 15, 2022

i also need to check if this part is still valid, and if it is maybe it can be done better:

if (toBattlefield && !copied.getEtbCounters().isEmpty()) {
    for (final ReplacementEffect re : copied.getReplacementEffects()) {
       re.setSuppressed(true);
    }
}

i tested it with this line enabled or disabled, it doesn't seems to have a difference

I finally found the corner case i have been looking for, because i was only testing it with Mowu, Loyal Companion:

Mowu, Loyal Companion
Because Mowu’s replacement effect affects only Mowu, it will apply if Mowu somehow enters the battlefield with one or more +1/+1 counters on it and give Mowu an additional +1/+1 counter.

This one should apply

Conclave Mentor
Conclave Mentor’s first ability doesn’t apply to itself if it’s somehow entering the battlefield with a +1/+1 counter on it.

This should not

Winding Constrictor
Winding Constrictor’s effect can’t apply to itself as it’s entering the battlefield or to any other permanent entering the battlefield at the same time as it.

This also not

for this, the PutCounter Replacement needs to check if it is ETB Counters, and then needs to check lastStateBattlefield stuff

especially if multiple of such would enter at the same time

@tool4ever
Copy link
Contributor

@Hanmac
can this be closed?

@Hanmac
Copy link
Contributor Author

Hanmac commented Jul 12, 2022

closed for now

@Hanmac Hanmac closed this as completed Jul 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants