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

Individual triggers on mass damage events can't be handled correctly #13057

Open
ciaccona007 opened this issue Nov 4, 2024 · 5 comments
Open
Labels
bug Bugs and errors refactoring Developers topics about code and programming

Comments

@ciaccona007
Copy link
Contributor

There are a few permanents that trigger whenever another permanent deals damage. These include Justice, Noble Purpose, and new card in FDN Quilled Greatwurm.

Currently, these effects are implemented by checking a GameEvent and seeing if it is an instance of either GameEvent.EventType.DAMAGED_PERMANENT or GameEvent.EventType.DAMAGED_PLAYER. However, this means that when a creature deals damage to two permanents/players, the ability will trigger twice, when it should only trigger once.

Replicate with the following init.txt:

battlefield:Human:Noble Purpose:1
battlefield:Human:Grand Melee:1
battlefield:Human:Hill Giant:1
battlefield:Computer:Gray Ogre:2

In this scenario, Human attacks with Hill Giant, it's double blocked, and it deals 2 damage to the first Gray Ogre and 1 to the second. Noble Purpose should only trigger once and gain 3 life, but instead it triggers twice to gain 2 and 1 life in succession.

This is a pretty minor issue but it's a bit more noticeable with the new card Quilled Greatwurm, where players might care about the correct behavior. One example is abilities that care about the number of +1/+1 counters being added to permanents. An example is Winding Constrictor which will add too many counters to creatures that deal damage, if the current implementation is used.

I tried working around this issue using GameEvent.EventType.DAMAGED_BATCH_FOR_ALL, but I couldn't get it to work. Maybe I haven't dug deep enough though, so let me know if there's a better workaround!

@JayDi85
Copy link
Member

JayDi85 commented Nov 4, 2024

GameEvent.EventType.DAMAGED_BATCH_FOR_ALL, but I couldn't get it to work

What’s wrong with batch event usage? Double batches, miss event, etc?

@ciaccona007
Copy link
Contributor Author

As far as I can tell, the batched event only triggers once per entire combat. I couldn't find a way to get one trigger per creature when using that, because there's only one event getting fired for all the creatures.

I could definitely be missing something! But I was assuming that this isn't possible currently because all the other cards that should use the correct implementation don't work properly.

@JayDi85
Copy link
Member

JayDi85 commented Nov 4, 2024

DAMAGED_BATCH_FOR_ONE_PERMANENT -- all damages for one creature:

shot_241105_012218

@JayDi85
Copy link
Member

JayDi85 commented Nov 4, 2024

ok, I see the problem. You need group damage events per attacking creature, not per target.

Justice:

Whenever a red creature or spell deals damage, Justice deals that much damage to that creature’s or spell’s controller.

Oracle:

If a single source does damage to multiple targets at once, Justice will add up all the damage done and deal damage to the source’s controller at one time (not multiple separate damagings). (2004-10-04)
The damage done by Justice is done as a triggered ability, which is added to the stack just after the red creature or spell deals its damage. (2004-10-04)

So there are must be one trigger per triggered/attacking creature

@JayDi85
Copy link
Member

JayDi85 commented Nov 4, 2024

Well, there are possible two solutions:

  1. Recommended. Add new batch event and related code: DAMAGED_BATCH_FOR_ONE_ATTACKER -- same as DAMAGED_BATCH_FOR_ONE_PERMANENT but group by attacker id and created in addSimultaneousDamageToBatchForAll method;
  2. Override trigger method to simulate multiple calls (will put multiple triggers on stack):
    @Override
    public void trigger(Game game, UUID controllerId, GameEvent triggeringEvent) {
        // simulate multiple triggers (damages collected on checkTrigger) 
        this.damages.forEach(damage -> {
            this.getEffects().clear();
            this.getEffects().add(new GainLifeEffect(damage));
            super.trigger(game, controllerId, triggeringEvent);
        });
    }

@JayDi85 JayDi85 added refactoring Developers topics about code and programming bug Bugs and errors labels Nov 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Bugs and errors refactoring Developers topics about code and programming
Projects
None yet
Development

No branches or pull requests

2 participants