[fixed]Human's Sun Droplet no trigger on opponent's upkeep

[Aswan jaguar]

Describe the Bug:
Sun Droplet for human doesn't trigger on opponent's upkeep. For AI it triggers correctly on both.

Which card did behave improperly?
Sun Droplet

Which update are you using? (date, name)Which type? (duel, gauntlet, sealed deck)
Manalink dev 778ccb5 version - duel

What exactly should be the correct behavior/interaction?
Sun Droplet for human triggers optionally in each upkeep.

Are any other cards possibly affected by this bug?
-

[Aswan jaguar]

I am way, too much frustrated with this, it seemed an easy fix and probably is but after trying different methods over and over and over I can't figure out why doesn't it trigger on opponents upkeep if any other trigger except RESOLVE_TRIGGER_MANDATORY is put. It seems that Anybody in whose_trigger in void upkeep_trigger_ability_mode() can't co exist with optional triggers.

Code: Select all

int card_sun_droplet(int player, int card, event_t event)
{
  /* Sun Droplet   |2
   * Artifact
   * Whenever you're dealt damage, put that many charge counters on ~.
   * At the beginning of each upkeep, you may remove a charge counter from ~. If you do, you gain 1 life. */

   check_damage_test(ANYBODY, -1, event, DDBM_MUST_DAMAGE_PLAYER | DDBM_MUST_DAMAGE_TRIGGERING_PLAYER | DDBM_REPORT_DAMAGE_DEALT,
                  player, card, 0, NULL);

   int result = resolve_damage_trigger(player, card, event, DDBM_MUST_DAMAGE_PLAYER | DDBM_MUST_DAMAGE_TRIGGERING_PLAYER |
                              DDBM_REPORT_DAMAGE_DEALT, player, card);
   if( result ){
      int amount = get_card_instance(player, card)->targets[16].player;
      get_card_instance(player, card)->targets[16].player = 0;
      add_counters(player, card, COUNTER_CHARGE, amount);
   }

   if( count_counters(player, card, COUNTER_CHARGE) ){
      upkeep_trigger_ability_mode(player, card, event, ANYBODY, RESOLVE_TRIGGER_AI(player));

      if (event == EVENT_UPKEEP_TRIGGER_ABILITY ){
         remove_counter(player, card, COUNTER_CHARGE);
         gain_life(player, 1);
      }
   }

   return 0;
}

and function:

Code: Select all

void upkeep_trigger_ability_mode(int player, int card, event_t event, int whose_upkeep, resolve_trigger_t trigger_mode){
   if( trigger_condition == TRIGGER_UPKEEP && affect_me(player, card) &&
      (whose_upkeep == current_turn || whose_upkeep == 2) &&
      !is_humiliated(player, card)
     ){
      int count = count_upkeeps(current_turn);
      if(event == EVENT_TRIGGER && count > 0){
         if (trigger_mode == RESOLVE_TRIGGER_DUH){
            trigger_mode = duh_mode(player) ? RESOLVE_TRIGGER_MANDATORY : RESOLVE_TRIGGER_OPTIONAL;
         }
         event_result |= trigger_mode;
      }
      else if(event == EVENT_RESOLVE_TRIGGER){
         while( count > 0 && call_card_function(player, card, EVENT_UPKEEP_TRIGGER_ABILITY) != -1){
            count--;
         }
      }
   }
}

[Korath]

Short version:
RESOLVE_TRIGGER_anything_but_MANDATORY really doesn't work for TRIGGER_UPKEEP, except during the triggering card's controller's own upkeep phase. The reason this card works for the AI is because RESOLVE_TRIGGER_AI(player) is identical to RESOLVE_TRIGGER_MANDATORY when player == AI or has duh_mode() on.

Long version:
upkeep_phase(), currently living in functions/engine.c, calls dispatch_trigger(player, TRIGGER_UPKEEP, [other parameters that aren't relevant here]). "player" in upkeep_phase() is the player whose upkeep phase it is; it's always set to current_turn, since WOTC hasn't ever printed a card that lets you have phases during the other player's turn even in un-sets. The first parameter to dispatch_trigger() is who gets to decide which order triggers happen in and, for optional ones, whether they happen at all; it gets exposed to client code in the reason_for_trigger_controller global.

If you compare upkeep_trigger_ability_mode() to anything checking just about any other trigger besides TRIGGER_UPKEEP, you'll see that reason_for_trigger_controller is always checked (and almost always in the form "reason_for_trigger_controller == player", because triggers that are optional for anyone except the controller of the triggering card are quite rare). They can do this because every single other trigger in Manalink that

1. actually represents an in-game trigger, not a replacement effect or other prompt that happens to be implemented with the same system as triggers (so not TRIGGER_REPLACE_CARD_DRAW, TRIGGER_PAY_TO_BLOCK, TRIGGER_MUST_BLOCK, or TRIGGER_MUST_ATTACK) and
2. isn't TRIGGER_DRAW_PHASE (which is broken in exactly the same way as TRIGGER_UPKEEP)

calls dispatch_trigger2() instead, or its near-equivalent in Magic.exe at 0x4371a0.

There are two differences between dispatch_trigger() and dispatch_trigger2(). The first is that dispatch_trigger2() stores the old values of trigger_cause_controller and trigger_cause and sets them to two of its parameters; dispatch_trigger() leaves them untouched, so that either the trigger never explicitly sets them at all, or it requires its caller to do that bit of housekeeping before calling the function. (The exe function at 0x4371a0 works like dispatch_trigger(), not like dispatch_trigger2(), in this regard.) The second is that dispatch_trigger2() dispatches the trigger twice, setting reason_for_trigger_controller to a different player each time.

So the "easy" fix - and, really, the only correct one - is to change the call in functions/engine.c:upkeep_phase() to call dispatch_trigger2() instead. The reason "easy" is in scare quotes is because you've then got to go to every single card that looks for TRIGGER_UPKEEP and make sure that it checks reason_for_trigger_controller properly, because otherwise it'll trigger twice each upkeep. Most in Manalink probably go through upkeep_trigger_ability_mode() or an equivalent, but you've got to check all the ones that eventually end up in their old exe implementations as well. On the one hand, I don't think any actual cards do. On the other, some effect cards do. On the other (where'd I get that third hand?), both of the effect cards that listen for TRIGGER_UPKEEP - PoltergeistFX, originally for Xenic Poltergeist, and Netherlink, originally for Nether Shadow - already check that both reason_for_trigger_controller and current_turn == player, and I think they're both completely unused besides (though that's difficult to verify for effect cards).

[Aswan jaguar]

Fixed in commit 2e36bc28.
Affects bellow cards that couldn't accept optional triggers:
Seance, Tidal Force, Sun Droplet.
Cards that need to check reason_for_trigger_controller so that now
don't trigger twice:
Energy Flux, Shapeshifter + (fix p/t chosen), Island Fish Jasconius,
Rogue Skycaptain, Scourge of Numai, bounce_permanent_at_upkeep (4 cards),
Keeper of Keys, Crown-Hunter Hireling (probably didn't needed it
but can't check as monarch cards crash), Elfhame Sanctuary, rebound_legacy and resolve_graveyard_upkeep_triggers.
There are some cards that didn't have reason_for_trigger_controller and are using TRIGGER_UPKEEP and old functiions but for some reason they don't seem to trigger twice some maybe because they leave the battlefield or end game before 2nd trigger can take place? Should I put reason_for_trigger_controller there is it safe?
These cards are:
Elfhame Sanctuary, Mortal Combat, Mana Vault, Defense of the Heart, Impending Disaster.
I can't test as I don't have the special cards anywhere in my deck builder they don't show at special button so I don't know if reason_for_trigger_controller is needed:
In vanguard.c = int vanguard_card, static int effect_avatar_oni_of_wild_places, static int effect_avatar_prodigal_sorcerer.

[Korath]

Yes, you need to check the global even if the card seems to work without doing so (but test again afterwards too) - I seem to recall having to jump through hoops in Shandalar to get cards that had triggers that were optional for both players to work right, when I was still using this system for triggers instead of just replacement effects. If you don't check, then even if the trigger doesn't happen twice, the wrong player might end up choosing which order the triggers resolve in.

[Aswan jaguar]

Added for the rest in commit d28ed6a1.

[Aswan jaguar]

Fixed in commit 0919293e upkeep_trigger_mode_impl() that I have missed earlier and affected suspend cards and lots more that triggered twice in upkeep.