[fixed]Treva, the Renewer 's ability doesn't trigger

[Aswan jaguar]

Describe the Bug:
Treva, the Renewer 's ability doesn't trigger whenever it deals combat damage to a player.

Which card did behave improperly?
Treva, the Renewer

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?


Are any other cards possibly affected by this bug?
Similarly bugged to Darigaaz, the Igniter here:
viewtopic.php?f=86&t=15397#p220860
EDIT: The bug affects all invasion set dragons.

[gnomefry]

Confirming that Treva's combat ability still doesn't trigger.

[FastEddie]

This is an interesting one and I hope that someone can enlighten me why things are the way they are. A quick fix might be feasible but I don't want to break something.

The issue is within the invasion_dragon function which is called by all legendary invasion dragons:

Code: Select all

static void invasion_dragon(int player, int card, event_t event, int cless, int black, int blue, int green, int red, int white){

   if (damage_dealt_by_me(player, card, event, DDBM_MUST_DAMAGE_PLAYER | DDBM_TRACE_DAMAGED_PLAYERS | DDBM_MUST_BE_COMBAT_DAMAGE))
   {
      card_instance_t* instance = get_card_instance(player, card);
      int times_damaged[2] = { BYTE0(instance->targets[1].player), BYTE1(instance->targets[1].player) };
      instance->targets[1].player = 0;

      APNAP(p,{
         if( times_damaged[p] ){
            char buffer[100];
            if( p == player ){
               scnprintf(buffer, 100, "Activate %s (you were damaged)", cards_ptr[get_id(player, card)]->name);
            }
            else{
               scnprintf(buffer, 100, "Activate %s (opponent was damaged)", cards_ptr[get_id(player, card)]->name);
            }
            while( times_damaged[p] && has_mana_multi(player, cless, black, blue, green, red, white) ){

// This is where things fall apart...
               int choice = DIALOG(player, card, event, DLG_RANDOM, DLG_NO_STORAGE, DLG_NO_CANCEL,
                              buffer, 1, p != player ? 10 : 1,
                              "Decline", 5);
// .. because we always get a 0.

               if( choice == 1 ){
                  charge_mana_multi(player, cless, black, blue, green, red, white);
                  if( spell_fizzled != 1 ){
                     SET_BYTE1(instance->info_slot) = p;
                     call_card_function(player, card, EVENT_DAMAGE_TRIGGER_ABILITY);
                     instance->info_slot = 0;
                  }
               }
               times_damaged[p]--;
times_damaged[p] = %d", times_damaged[p])
            }
         }
      };
      );
   }
}


The DIALOG macro calls dialog_impl which in turns checks the event type and works only for certain events, namely EVENT_CAN_CAST, EVENT_CAST_SPELL, EVENT_RESOLVE_SPELL, EVENT_CAN_ACTIVATE, EVENT_ACTIVATE, and EVENT_RESOLVE_ACTIVATION. Call it with another event and it returns 0. Now the event after damaging a player is... EVENT_RESOLVE_TRIGGER, which explains why nothing happens. I could overwrite the event with EVENT_ACTIVATE or EVENT_RESOLVE_ACTIVATION, say, to force a dialog but I don't know whether this is a good solution and why dialog_impl has this restrictions in the first place (or whether it would be better to adjust dialog_impl to accept this additional event and adjust the code afterwards, starting from line 433 in dialog.c).

[Aswan jaguar]

I could overwrite the event with EVENT_ACTIVATE or EVENT_RESOLVE_ACTIVATION, say, to force a dialog but I don't know whether this is a good solution

That's not a good idea because this is a trigger after all not an activation event and you will cause other issues. I have fixed other such bugs with bad use of dialoq(), unfortunately I hadn't found the cause when trying to fix this bug about a year ago but later. And I tried a lot to fix this.

why dialog_impl has this restrictions in the first place

I don't know why Korath did it only for those but probably had a good reason for it (maybe because it couldn't be used without causing issues both with events and with triggers). If you see how much he did for dialog_impl() he certainly
programmed it with many functions and features a couple of them I believe they are not even used and others I don't know how to use them correctly. We are certainly don't use it fully. That's why I think it wasn't because he was bored or something.
In my opinion we should just replace dialoq() with the inferior do_dialoq() which works though in all cases and triggers like I did for others.

[drool66]

Good call, FastEddie.
Fixed in a645e15d7 ("Fix Invasion Dragons' trigger dialog", 2020-08-14) by switching to do_dialog() which has fewer options but is more flexible in when it can be used. Also got rid of the card name in dialog so the options fit better on screen. Looks like this if you're interested:

Code: Select all

static void invasion_dragon(int player, int card, event_t event, int cless, int black, int blue, int green, int red, int white){

   if (damage_dealt_by_me(player, card, event, DDBM_MUST_DAMAGE_PLAYER | DDBM_TRACE_DAMAGED_PLAYERS | DDBM_MUST_BE_COMBAT_DAMAGE))
   {
      card_instance_t* instance = get_card_instance(player, card);
      int times_damaged[2] = { BYTE0(instance->targets[1].player), BYTE1(instance->targets[1].player) };
      instance->targets[1].player = 0;
      APNAP(p,{
               if( times_damaged[p] ){
                  char buffer[100];
                  if( p == player ){
                     scnprintf(buffer, 100, " Activate (you were damaged)\n Cancel");
                  }
                  else{
                     scnprintf(buffer, 100, " Activate (opponent was damaged)\n Cancel");
                  }
                  while( times_damaged[p] && has_mana_multi(player, cless, black, blue, green, red, white) ){
                        int choice = do_dialog(player, player, card, -1, -1, buffer, p != player ? 0 : 1);
                        if( choice == 0 ){
                           charge_mana_multi(player, cless, black, blue, green, red, white);
                           if( spell_fizzled != 1 ){
                              SET_BYTE1(instance->info_slot) = p;
                              call_card_function(player, card, EVENT_DAMAGE_TRIGGER_ABILITY);
                              instance->info_slot = 0;
                           }
                        }
                        times_damaged[p]--;
                  }
               }
            };
      );
   }
}

EDIT: whoops, forgot to mark the topic fixed - thanks, AswanJaguar

[FastEddie]

Thank you guys. Looking at dialog_impl I had the impression that there is much more than meets the eye, so I didn't even dare to bother with it. Someone (Korath as I know now) spend a lot of time and effort and probably for a very good reason.

And I tried a lot to fix this.

I know what you mean... unfortunately, this kind of bug can't be tackled with code inspection as the code looks (and mostly is) ok. Since this is a nice use case for the logger I outlined below what I did and why so you can try it at home if you are interested (will also add this to the documentation).

It took several iterations. I plugged everything in the code below for ease of explanation.

First, you need to inlcude the header. I do this in manalink.h as it is included everywhere. Mine looks like this:

Code: Select all

#include "defs.h"

// DEBUG
#include "macrologger.h"
#define LOG_LEVEL   DEBUG_LEVEL

#ifdef __cplusplus
extern "C" {
#endif


Looking at Treva, the Renewer and Trygon Predator (which has a similar mechanic but works) I thought that the issue is probably within the invasion_dragon function as the rest of Treva, the Renewer never got called. So I prepared it like this (in several steps):

Code: Select all

static void invasion_dragon(int player, int card, event_t event, int cless, int black, int blue, int green, int red, int white){

   if (damage_dealt_by_me(player, card, event, DDBM_MUST_DAMAGE_PLAYER | DDBM_TRACE_DAMAGED_PLAYERS | DDBM_MUST_BE_COMBAT_DAMAGE))
   {
      card_instance_t* instance = get_card_instance(player, card);
      int times_damaged[2] = { BYTE0(instance->targets[1].player), BYTE1(instance->targets[1].player) };
      instance->targets[1].player = 0;

      // #1
      LOG_DEBUG("Damage dealt by me, player = %d", player);
      LOG_DEBUG("times_damaged = {%d, %d}", times_damaged[0], times_damaged[1]);
      
      APNAP(p,{
         if( times_damaged[p] ){
            char buffer[100];
            if( p == player ){
               scnprintf(buffer, 100, "Activate %s (you were damaged)", cards_ptr[get_id(player, card)]->name);
            }
            else{
               scnprintf(buffer, 100, "Activate %s (opponent was damaged)", cards_ptr[get_id(player, card)]->name);
               // #2
               LOG_DEBUG("Opponent was damaged");
            }
            // #3
            LOG_DEBUG("times damaged[p] = %d", times_damaged[p]);
            LOG_DEBUG("has_mana_multi(player, cless, black, blue, green, red, white) = %d", has_mana_multi(player, cless, black, blue, green, red, white));
            while( times_damaged[p] && has_mana_multi(player, cless, black, blue, green, red, white) ){
               // #4
               LOG_DEBUG("Entering while");
               // #6
               LOG_DEBUG("event = %d", event);
               int choice = DIALOG(player, card, event, DLG_RANDOM, DLG_NO_STORAGE, DLG_NO_CANCEL,
                              buffer, 1, p != player ? 10 : 1,
                              "Decline", 5);
               // #5
               LOG_DEBUG("choice = %d", choice);
               if( choice == 1 ){
                  charge_mana_multi(player, cless, black, blue, green, red, white);
                  if( spell_fizzled != 1 ){
                     SET_BYTE1(instance->info_slot) = p;
                     call_card_function(player, card, EVENT_DAMAGE_TRIGGER_ABILITY);
                     instance->info_slot = 0;
                  }
               }
               times_damaged[p]--;
               // #4
               LOG_DEBUG("Leaving while, times_damaged[p] = %d", times_damaged[p]);
            }
         }
      };
      );
   }
}


After compiling I run the code either through bash (Linux subsystem under Windows) or the cmd prompt. The syntax is

bash: ./Magic.exe 2>Treva.log
cmd: Magic.exe 2>Treva.log

Once I attacked with Treva, the Renewer I ended the program to fully write the log file and have a look. The function is called several (30 or so) times, so I just look at the last entries. Usually the time stamp of those is a bit (a second or so) later. This went as follows.

#1 logging:
To see whether the function is entered, the player is right and the damage is assigned correctly. My first suspicion was that there is an issue with times_damaged and Trygon Predator works whereas it shouldn't.

#1 output:
2020-08-14 07:23:00 | DEBUG | invasion.c | invasion_dragon:472 | Damage dealt by me, player = 0
2020-08-14 07:23:00 | DEBUG | invasion.c | invasion_dragon:473 | times_damaged = {0, 1}

#2 logging:
Seems to be ok. Let's see whether this squares with the damage assignment.

#2 output:
2020-08-14 07:23:00 | DEBUG | invasion.c | invasion_dragon:513 | Opponent was damaged

#3 logging:
Works again. So maybe we don't enter the while loop. I don't know what has_mana_multi does precisely, maybe this causes an issue. I log times_damaged for good measure so that I don't miss anything (just because you're paranoid...).

#3 output:
2020-08-14 07:23:00 | DEBUG | invasion.c | invasion_dragon:513 | times damaged[p] = 1
2020-08-14 07:23:00 | DEBUG | invasion.c | invasion_dragon:513 | has_mana_multi(player, cless, black, blue, green, red, white) = 1

#4 logging:
Both 1, so we enter the while loop. If everything works as I think we should be inside only once. Let's check this.

#4 output:
2020-08-14 07:23:00 | DEBUG | invasion.c | invasion_dragon:513 | Entering while
2020-08-14 07:23:00 | DEBUG | invasion.c | invasion_dragon:513 | Leaving while, times_damaged[p] = 0

#5 logging:
As expected. Good. It is likely that choice != 1. Let's see.

#5 output:
2020-08-14 07:23:00 | DEBUG | invasion.c | invasion_dragon:513 | choice = 0

#6 logging:
BINGO! Looking up DIALOG and dialog_impl tells me that only certain events work. No idea what event is used when calling DIALOG but obviously not the right one. Let's see which one it is.

#6 output:
2020-08-14 07:23:00 | DEBUG | invasion.c | invasion_dragon:513 | event = 126

Coda:
126 is 0x7E (I use the windows calculator for the conversion). Looking in defs.h I find the event code in line 230, it is called EVENT_RESOLVE_TRIGGER (which makes intuitive sense).

NB: I didn't use the debugger as I learned in the meanwhile that these functions are called several times. Always continuing the exection while running the risk to miss the proper call is cumbersome and errorprone, so this is not the preferred approach in a case like this.

Edit. I just saw that you could make your life easier during the last step with something like this, printing the event id in hex:

Code: Select all

LOG_DEBUG("event = %04x", event);

[Korath]

I could overwrite the event with EVENT_ACTIVATE or EVENT_RESOLVE_ACTIVATION, say, to force a dialog but I don't know whether this is a good solution

That's not a good idea because this is a trigger after all not an activation event and you will cause other issues. I have fixed other such bugs with bad use of dialoq(), unfortunately I hadn't found the cause when trying to fix this bug about a year ago but later. And I tried a lot to fix this.

why dialog_impl has this restrictions in the first place

I don't know why Korath did it only for those but probably had a good reason for it (maybe because it couldn't be used without causing issues both with events and with triggers). If you see how much he did for dialog_impl() he certainly
programmed it with many functions and features a couple of them I believe they are not even used and others I don't know how to use them correctly. We are certainly don't use it fully. That's why I think it wasn't because he was bored or something.
In my opinion we should just replace dialoq() with the inferior do_dialoq() which works though in all cases and triggers like I did for others.

DIALOG() was aimed at the extremely-common case (in particular, all cards with more than one activated ability) of quickly checking whether any choices are legal, making a decision at announcement, and referencing the choice at resolution. Previously this had been implemented by cut, paste, and pray: if you look through old bug reports, you'll find literally thousands of cases where choices don't match up, or choices made for one mode impact the behavior of later activations.

Using DIALOG() as a do_dialog()-with-extra-features is ok, but you have to tell it you're doing so. This is because the common case requires storage space somewhere to, at resolution, remember the choice made at activation. This is typically in card_instance_t::info_slot, though there's options to move it elsewhere. It's deliberate that it fails quickly and obviously (by doing nothing) if you give it a non-supported event, so that you know you're doing something wrong. Also, the initial intent was that you could just call DIALOG() at the start of a card function without checking event first, but that turned out to be impractical.

If you want to use it in such a case, pass DLG_NO_STORAGE and event=EVENT_CAST_SPELL (or any of the other six supported events) - this both prevents overwriting info_slot, which this card also uses for something else, and setting spell_fizzled if "Cancel" is chosen, which breaks trigger handling.