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

[don't merge - see #11841 instead] [WHO] Implement Donna Noble #11833

Closed
wants to merge 4 commits into from

Conversation

jimga150
Copy link
Contributor

@jimga150 jimga150 commented Feb 22, 2024

#10653

I initially tried modifying DealtDamageToSourceTriggeredAbility, but i don't know how to get a single ability to do two different checks for two trigger conditions. This is relevant because:

If Donna is paired with another creature and they are both dealt damage at the same time, the second ability triggers twice. (2023-10-13)

So if Donna is paired with a creature, and something like Cinderclasm is cast (with no kick), Donna should trigger twice for 1 damage both times, rather than 2 damage once.

This is currently the case, but the way i implemented it uses the method in Wrathful Raptors. This uses the DAMAGED_PERMANENT event trigger, which causes multiple triggers if its damaged by multiple sources at the same time, like being blocked by multiple creatures. This isnt how it should work according to the MTG wiki:

If multiple sources deal damage to a creature with an enrage ability at the same time, most likely because multiple creatures blocked that creature, the enrage ability triggers only once.

Question is, how can this be implemented to use the DAMAGED_BATCH_FOR_PERMANENTS event, while causing separate damage events for the damages dealt to donna and the (possibly) paired creature? Is there an example of a card causing multiple effects targeting different things, one or more of which might not trigger and therefore doesnt need a target?

@github-actions github-actions bot added the cards label Feb 22, 2024
Copy link
Member

@JayDi85 JayDi85 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Possible ideas (from bad to good):

  • use number of triggers event to change it;
  • use two triggers, one for paired, one for damage;
  • use one trigger for both events: batch for damage, normal for paired;
  • use one batch trigger for one event but do damage two times in effect (for two saved id).

@jimga150
Copy link
Contributor Author

jimga150 commented Feb 22, 2024

Possible ideas (from bad to good):

* use number of triggers event to change it;

* use two triggers, one for paired, one for damage;

* use one trigger for both events: batch for damage, normal for paired;

* use one batch trigger for one event but do damage two times in effect (for two saved id).

Trying the last one now. I'm not sure how to add a target to an ability while having the player possibly not have to resolve that target?

Edit: i think i cracked it.

Copy link
Contributor

@xenohedron xenohedron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this is clever, but it's still a workaround. Here you have two effects in a single trigger to emulate two separate triggers.

I think what is really needed is a new type of damage batch event for one permanent. I will accept the workaround if that is problematic, but might be worth trying.

Would you be willing to write a unit test to check the possible cases here?

@jimga150
Copy link
Contributor Author

So this is clever, but it's still a workaround. Here you have two effects in a single trigger to emulate two separate triggers.

I think what is really needed is a new type of damage batch event for one permanent. I will accept the workaround if that is problematic, but might be worth trying.

Would you be willing to write a unit test to check the possible cases here?

Sure yeah--we're talking about adding a new type of game event?
Might be a good chance to try unit tests

@jimga150
Copy link
Contributor Author

I'm not sure what that would look like specifically. this would be a batch event for damaging a single permanent, how would this function differently than DAMAGED_PERMANENT?

@xenohedron
Copy link
Contributor

Existing example: DAMAGED_BATCH_FOR_ONE_PLAYER

Need to adjust addSimultaneousDamage in GameState to work with new batches.

The idea is that all simultaneous damage must be collected into batches split by permanent id.

(note: I haven't thought through this all the way so not 100% sure this is workable)

@JayDi85 anything to keep in mind for this direction?

@xenohedron
Copy link
Contributor

(Maybe best to do that direction in separate PR as alternative to this one)

@JayDi85
Copy link
Member

JayDi85 commented Feb 23, 2024

@xenohedron

Nope, its bad idea to split batch events. Thats split returns to current buggy group event logic with duplicated events. Use single batch, just process it in diff triggers or diff use cases.

DAMAGED_PERMANENT > BATCH_DAMAGED_PERMANENTS
DAMAGED_PLAYER > BATCH_DAMAGED_PLAYERS
any > BATCH_DAMAGED_ANY

or like that. All events for a single player/permanent can be extracted from a batch by ability.

As idea: use watcher to monitor single events (try to find own and another damages) and use 1 or 2 triggers for batch/final event to execute (if watcher contains data the trigger can be activated).

@xenohedron
Copy link
Contributor

@xenohedron

Nope, its bad idea to split batch events. Thats split returns to current buggy group event logic with duplicated events. Use single batch, just process it in diff triggers or diff use cases.

As idea: use watcher to monitor single events (try to find own and another damages) and use 1 or 2 triggers for batch/final event to execute (if watcher contains data the trigger can be activated).

I don't understand. It needs to trigger twice to follow mtg rules. Doesn't that require that there be two events to check independently? As far as I know checkTrigger is only called once per event.

@JayDi85
Copy link
Member

JayDi85 commented Feb 23, 2024

Yes, one event - one trigger check - one triggered ability. Number of triggers can be changed by special event only (it’s not that use case).

Example implementation: 1:

  • watcher on DAMAGED_PERMANENT: save id, damage and type (from own or from another);
  • trigger 1 on damaged batch event: look in watcher for damage from own (trigger first time)
  • trigger 2 on damaged batch event: look in watcher for damage from other (trigger second time);

Example implementation 2 (I don't like it but it possible):

  • trigger on single damage event, check damage type and save used status to game state (e.g. is it triggered for damage type or not);
  • reset state value on any post damage event (group, batch, maybe another);
  • so single trigger will be checked multiple times, but it will be filtered 1 or 2.

@jimga150
Copy link
Contributor Author

Yes, one event - one trigger check - one triggered ability. Number of triggers can be changed by special event only (it’s not that use case).

Example implementation: 1:

* watcher on DAMAGED_PERMANENT: save id, damage and type (from own or from another);

* trigger 1 on damaged batch event: look in watcher for damage from own (trigger first time)

* trigger 2 on damaged batch event: look in watcher for damage from other (trigger second time);

Example implementation 2 (I don't like it but it possible):

* trigger on single damage event, check damage type and save used status to game state (e.g. is it triggered for damage type or not);

* reset state value on any post damage event (group, batch, maybe another);

* so single trigger will be checked multiple times, but it will be filtered 1 or 2.

The first idea sounds like a similar result to what is already implemented for this card--that the ability will trigger once for each batch damage event, but then will cause up to two "damage player" effects to trigger. Or am i misunderstanding?

@jimga150
Copy link
Contributor Author

jimga150 commented Feb 23, 2024

@xenohedron

Nope, its bad idea to split batch events. Thats split returns to current buggy group event logic with duplicated events. Use single batch, just process it in diff triggers or diff use cases.

DAMAGED_PERMANENT > BATCH_DAMAGED_PERMANENTS DAMAGED_PLAYER > BATCH_DAMAGED_PLAYERS any > BATCH_DAMAGED_ANY

or like that. All events for a single player/permanent can be extracted from a batch by ability.

As idea: use watcher to monitor single events (try to find own and another damages) and use 1 or 2 triggers for batch/final event to execute (if watcher contains data the trigger can be activated).

Has a bug like this presented for the DAMAGED_BATCH_FOR_ONE_PLAYER event? It seems like the new event would just be that but for permanents.

I thought having batch events and singleton events already established the pattern of having a given damage occurrence spawn more than one corresponding events, and that triggers are responsible for not double-counting things? Adding another kind of batch would just add to that right?

@JayDi85
Copy link
Member

JayDi85 commented Feb 23, 2024

Current implementation with two “player.damage” calls is not compatible with rules:

If Donna is paired with another creature and they are both dealt damage at the same time, the second ability triggers twice. (2023-10-13)

Rules require two triggers on stack, not one.

@xenohedron xenohedron changed the title [WHO] Implement Donna Noble [don't merge - see #11841 instead] [WHO] Implement Donna Noble Mar 15, 2024
@xenohedron xenohedron closed this Mar 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants