Fix addSimultaneousDamage to avoid adding damage events to existing DamagedBatchForOnePlayerEvent instances when they shouldn't

[jimga150]

Bug discovered in #11841

Currently, addSimultaneousDamage checks to see if a given GameEvent in simultaneousEvents is a DamagedBatchEvent to add it to that batch, BEFORE doing the check to see if that batch event is actually a DamagedBatchForOnePlayerEvent and the target matches. because if that condition isn't satisfied, the DamagedEvent instance should NOT be added to that batch event

This results in DamagedEvent instances being added to DamagedBatchForOnePlayerEvent instances fired in the same timing window regardless of whether the targets match, possibly conglomerating more damage into the DamagedBatchForOnePlayerEvent than it should have.

This should fix that behavior.

I want to write a test to show this works, but i see no cards that would be affected by this bug. Most use "when combat damage to player happens, do <thing that doesn't care about how much damage it was>", and the ones that care about how much damage it was (Quartzwood Crasher, Anowon, the Ruin Thief, and Lolth, Spider Queen) still only care about combat damage, which wont let me test a scenario where different players are dealt damage by different targets at the same time.

Maybe there is no way that scenario could happen normally, but this behavior will be important for implementing DamageBatchForOnePermanent, where this happens all the time. Thoughts?

[xenohedron]

Nice find. Thanks for the investigation.

[xenohedron]

Probably we should add more comments here to clarify that this covers DamagedBatchForPermanentsEvent and DamagedBatchForPlayersEvent

[xenohedron]

Existing test coverage from ImodaneThePyrohammerTest and ThopterSpyNetworkTest shows that the events with affected logic still work properly.

[jimga150]

Existing test coverage from ImodaneThePyrohammerTest and ThopterSpyNetworkTest shows that the events with affected logic still work properly.

This is for the multiple permanent and player damage batches, which is good, but i wonder if there's a way to see that the fix this does actually works as well.

[xenohedron]

It is possible to add custom abilities to cards in unit test framework if you want to try. I don't think it's really necessary, assuming that #11841 will include an analogous test for the one permanent batch event.

[JayDi85]

Original reason in wrong inheritance:

Damaged batch for one player must not extend damaged class. It must be independent from another damaged batches (extend batch event):

Also it potentially buggy (duplicate triggers) for cards/code like:

So real fix must be:

• keep old batch logic (one group to split by damage type, another group to filter player damage and split it by diff players);
• refactor one player damaged batch to independent class;

[xenohedron]

I don't see how existing cards would be bugged - checkEventType will prevent duplicate triggers because only one type of batch event will be checked.

Changing the inheritance structure would also have solved this issue but I don't see why it's a better or worse solution.