[confirmed]Coffin Queen dumps

[FastEddie]

Describe the Bug:
Coffin Queen dumps when activated. I looked into it and found a few interesting things, but didn't get any further.

The dump itself is caused by charge_mana() which is called by charge_mana_multi() to charge the activation cost. Up to that point all parameters look good, i.e. generic_activated_ability_with_target_in_grave and everything coming afterwards uses the correct card id (that of Coffin Queen) and the correct player.

The first paramter of in_play in the dump is the index of the card in the graveyard, 0 for the first card, 1 for the second, etc. while this should be the id of the player (0 or 1). So somewhere between calling charge_mana() and calling in_play() things get mixed up.

There is one more issue which I (obviously) couldn't test. I think the code inside Coffin Queen's EVENT_RESOLVE_ACTIVATION should rather look like this:

Code: Select all

if (selected != -1){
   // generic_activated_ability_with_target_in_grave returns graveyard in targets[0] and card in targets[1]
   int zombie = reanimate_permanent(player, card, instance->targets[0].player, selected, REANIMATE_DEFAULT | REANIMATE_NO_CONTROL_LEGACY);
   
   if( zombie > -1 ){


Originally it uses targets[2}, but to my understanding this shouldn't be set and anyway the graveyard id is in targets[0].

| Open

bad parameters
in_play(4, 234886677)
0: 0x02550738
1: 0x0250315B
2: 0x0251E44C
3: 0x0216DEE5
4: 0x02560C55
5: 0x0253C0CD
6: 0x0253CC0E
7: 0x00435A13
8: 0x02510482
9: 0x00472273
10: 0x004379C4
11: 0x0049CBFA
12: 0x0250F4F9
13: 0x02526AA6
14: 0x02533534
15: 0x025393C9
16: 0x0216DF98
17: 0x02560C55
18: 0x0253C0CD
19: 0x0253C7F6
20: 0x004344B2
21: 0x0043C147
22: 0x004399BD
23: 0x0047902C
24: 0x004946E9
25: 0x77A56359
26: 0x77E27C24
27: 0x77E27BF4
28: 0x00000000

Which card behaved improperly?
Coffin Queen

Which update are you using? (date, name)Which type? (duel, gauntlet, sealed deck)
Version 10-2020 (bug existed also under version 08-2020)
Duel

What exactly should be the correct behavior/interaction?
Coffin Queen should not dump and return the proper creature.

Are any other cards possibly affected by this bug?
Depends on what the issue is. There could be other cards using a similar graveyard machanic.

[Aswan jaguar]

There seems to be some code of choose_to_untap() that conflicts with GS_GRAVE_RECYCLER_BOTH_GRAVES mode in generic_activated_ability_with_target_in_grave(). If you disable either of those it will not error and crash and you will get to see the mana prompt. If you disable choose_to_untap() and if you fix the bug in reanimate_permanent() as said in post above you will see another bug that the animated creature will not be exiled if Coffin Queen untaps or leaves play.

The EMN version of Coffin Queen worked fine. If we don't manage to fix it using current method I will see if that still works.

[drool66]

There seems to be some code of choose_to_untap() that conflicts with GS_GRAVE_RECYCLER_BOTH_GRAVES

Looks like it. generic_activated_ability_with_target_in_grave() with GS_GRAVE_RECYCLER_BOTH_GRAVES uses select_target_from_either_grave() with ret_location == 1; choose_to_untap() continuously checks in_play(targets[1].player, targets[1].card) - hence the bad instance. I can't grok how exactly choose_to_untap() works, especially what the targets[1] marker is for. Maybe someone else can take a look?

[FastEddie]

Thanks guys. I will have another look later this week.

[drool66]

Well, I looked at this when I was pretty tired last night, and the targets[1] thing is right there in the comments. The choose_to_untap() function could use some work. The way it works now, if targets[1] is not in play, the AI will untap the permanent. One problem in addition to the one at hand is that few of the cards that use this function actually load their target to targets[1]. That's why the AI's Old Man of the Sea will release its target every turn. Additionally, targets[1] just seems like a very unsafe place to store this sort of thing, as we see with Coffin Queen. FastEddie, if you want to take this on, maybe think about storing it in an attached legacy? Lmk if you would rather I take care of it.
I think you mentioned you don't have a working grep, so here are the hits for choose_to_untap():
alliances:3772
antiquities:377, 1594, 2359, 2428
arabian_nights: 2102
fallen_empires:975, 3491
fifth_dawn:4115
homelands:1487
ice_age:3888, 5436
journey_into_nyx:4129
legends:6879, 9005
mercadian_masques:3836
mirage:671, 2897
scars_of_mirrodin:5448
tempest:1020, 2668
the_dark:2486
urza_legacy:2187

[FastEddie]

I will take care of that. My suspicion was that this is water on Korath's mills, namely that storing data inside cards is usually a bad idea (but not always). My idea was indeed a targeted legacy and I will add a find function to make searching for those easier.
I saw something similar at Leonin Arbiter and added a find function for legacy effects, but this isn't committed yet. With create and find functions storing data in legacies should be as simple as storing data in cards and way safer to boot.

[FastEddie]

Re grep, I never bothered as find in files in the IDE does the job for me, but thanks for the effort!

[Aswan jaguar]

drool66 you have committed some changes to choose_to_untap() are those a temporary fix, until it gets fixed with a legacy?

[drool66]

No, I committed them by mistake because I was working on three or four things at once. I don't think they do anything and they definitely don't belong in that commit. They'll almost definitely be overwritten once this bug is squashed though.

[FastEddie]

The lady made some progress but in return I got weird side effects.

Coffin Queen looks like this now:

Code: Select all

int card_coffin_queen(int player, int card, event_t event){
 /* CARD_ID_COFFIN_QUEEN   3106
 Coffin Queen   |2|B
 Creature - Zombie Wizard 1/1
 You may choose not to untap ~ during your untap step.
 |2|B, |T: Put target creature card from a graveyard onto the battlefield under your control. When ~ becomes untapped or you lose control of ~, exile that creature.*/

   // Call this first, otherwise no legacy will be created
   choose_to_untap(player, card, event);

   if( ! IS_GAA_EVENT(event) ){
      return 0;
   }

   test_definition_t test;
   new_default_test_definition(&test, TYPE_CREATURE, "Select target creature card.");

   if (event == EVENT_RESOLVE_ACTIVATION){
      card_instance_t* instance = get_card_instance(player, card);
      int selected = validate_target_from_grave_source(player, card, instance->targets[0].player, 1);
      if (selected != -1){
         // generic_activated_ability_with_target_in_grave returns graveyard in targets[0] and card in targets[1]
         int zombie = reanimate_permanent(player, card, instance->targets[0].player, selected, REANIMATE_DEFAULT | REANIMATE_NO_CONTROL_LEGACY);

         if( zombie > -1 ){
            int legacy = create_targetted_legacy_effect(instance->parent_controller, instance->parent_card, &cq_effect, player, zombie);
            card_instance_t *leg = get_card_instance(player, legacy);
            leg->targets[0].player = instance->parent_controller;
            leg->targets[0].card = instance->parent_card;
            leg->number_of_targets = 1;
//            card_instance_t *parent = get_card_instance(instance->parent_controller, instance->parent_card);
//            parent->targets[1] = instance->targets[0];
            set_choose_to_untap_target(player, card, player, zombie);
         }
      }
   }

   return generic_activated_ability_with_target_in_grave(player, card, event, 0, MANACOST_XB(2,1), 0, NULL, NULL,
                                             0, &test, GS_GRAVE_RECYCLER_BOTH_GRAVES);
}


choose_to_untap now uses a legacy to store the targets[1] and I added two other functions, one to manually change the target as this is needed by the AI and we cannot rely any longer on the cards setting targets[1] themselves and one to easily find targetted legacies.

Code: Select all

/* Find a targetted legacy effect created by create_targetted_legacy_effect(player, card, func_ptr, target_player, target_card).
 * Player, function pointer: same as used for create_legacy_effect.
 * Card: id returned by create_legacy_effect.
 * Return value: card id if legacy was found, otherwise -1. */
int find_targetted_legacy_effect(int player, int card, int (*func_ptr)(int, int, event_t), int target_player, int target_card){
   int result;
   card_instance_t* leg_inst;
   for ( result=0; result < active_cards_count[player]; result++ ) {
      if( (leg_inst = in_play(player, result))
         && leg_inst->internal_card_id == LEGACY_EFFECT_CUSTOM && leg_inst->info_slot == (int)func_ptr
         && leg_inst->damage_source_player == player && leg_inst->damage_source_card == card
         && leg_inst->targets[0].player == target_player && leg_inst->targets[0].card == target_card
         ){
         return result;
      }
   }
   return -1;
}

/* Make untapping card optional. Used for "As long as card ~ is tapped do something to another card" effects.
 * Call at the beginning of the card code.
 * Choose_to_untap tracks a target card (see also set_choose_to_untap_target). The AI will untap if this card leaves play. */
void choose_to_untap(int player, int card, event_t event){

   if( event == EVENT_CAST_SPELL && affect_me(player, card) ){
      set_special_flags2(player, card, SF2_COULD_NOT_UNTAP);
   }
   
   if ( event == EVENT_RESOLVE_SPELL ) {
      // Create a targetted legacy as storage for target card (AI will untap when this card leaves play)
      int legacy = create_targetted_legacy_effect(player, card, &empty, player, card);
      ASSERT(legacy != -1); // This should never happen but... better safe than sorry and it says more than a generic get_card_instance dump
      card_instance_t *legacy_inst = get_card_instance(player, legacy);
      legacy_inst->targets[1].player = legacy_inst->targets[1].card = -1; // Should be unnecessary but since we rely on this make it explicit
      // TODO: make legacy invisible
      return;
   }

   // Check whether target left play in the meanwhile
   int legacy = find_targetted_legacy_effect(player, card, &empty, player, card);
   if (legacy != -1) {
      // Do not assert here as the legacy may not yet be in play
      card_instance_t *legacy_inst = get_card_instance(player, legacy);
      if( legacy_inst->targets[1].player > -1 && ! in_play(legacy_inst->targets[1].player, legacy_inst->targets[1].card) ){
         legacy_inst->targets[1].player = -1;
      }
   }

   card_instance_t *instance = get_card_instance(player, card);

   if (event == EVENT_UNTAP && affect_me(player, card) && is_tapped(player, card)){
      instance->untap_status &= ~2;
   }

   if (current_phase == PHASE_UNTAP && affect_me(player, card)){
      if (event == EVENT_TRIGGER){
         if (instance->untap_status & 1){ // Card it tapped ...
            if (!(instance->untap_status & 2)
               && !(cards_data[instance->internal_card_id].type & types_that_dont_untap)){ // ... and not marked for untapping or that it cannot untap
               card_instance_t *legacy_inst = get_card_instance(player, legacy);
               if ((player != 1 || (trace_mode & 2)) && ai_is_speculating != 1){
                  event_result |= 1;   // Human: untapping is optional
               } else if (legacy_inst->targets[1].player > -1 && in_play(legacy_inst->targets[1].player, legacy_inst->targets[1].card)){
                  ;   // AI: leave tapped if target card is still in play
               } else {
                  event_result |= 2;   // AI: target card left play, untap
               }
            }
         }
      } else if (event == EVENT_RESOLVE_TRIGGER){
         instance->untap_status |= 2; // Mark card as "shall untap"
      }
   }
}

/* Set the target card tracked by choose_to_untap.
 * Cards using choose_to_untap must do this manually as there is no other way to know when the target changed.
 * player, card: who is calling set_choose_to_untap_target
 * target_player, target_card: new target */
void set_choose_to_untap_target(int player, int card, int target_player, int target_card) {
   int legacy = find_targetted_legacy_effect(player, card, &empty, player, card);
   if ( legacy != -1 ) {
      // Do nothing if no legacy exists as this function might have been called from anywhere.
      card_instance_t *legacy_inst = get_card_instance(player, legacy);
      legacy_inst->targets[1].player = target_player;
      legacy_inst->targets[1].card = target_card;
   }
   return;
}


If you want to try this at home add the following lines to manalink.h:

Code: Select all

int find_targetted_legacy_effect(int player, int card, int (*func_ptr)(int, int, event_t), int targ_player, int target_card); // Find a legacy effect created by create_targetted_legacy_effect.
void set_choose_to_untap_target(int player, int card, int target_player, int target_card); // Manually set the target tracked by choose_to_untap so that the AI can track it


What I encounter now is this: Coffin Queen reanimates a creature as it should. But... instantanously afterwards Coffin Queen gets exiled and I skip my draw phase. When I kill the animated creature Coffin Queen comes back and so does the draw phase.

Any ideas what could cause this?

[drool66]

I might have some idea - I don't see the code for cq_effect(), can you post that please
[EDIT]
I tried out the code - two changes fix the card:
in cq_effect, you need to change the kill_card to kill instance->damage_target_player/card, that's why coffin queen was dying - she was in that spot instead. She was dying on resolution because you were missing GAA_UNTAPPED in your generic_activated_ability_with_target_in_grave(), and so the !is_tapped() conditional returned true. You skipped your draw phase because the remaining cq_legacy killed the next card with the recycled Coffin Queen player, card designation, which was the card drawn on your draw phase.

You'll see I made "static int find_attached_legacy_effect()" for Muldrotha the Gravetide, which is the same as your "find_targetted_legacy_effect()". If you do commit, would you please tidy that all up for both cards so that we don't have redundant functions?

Otherwise, very impressive work!

[FastEddie]

You'll see I made "static int find_attached_legacy_effect()" for Muldrotha the Gravetide, which is the same as your "find_targetted_legacy_effect()". If you do commit, would you please tidy that all up for both cards so that we don't have redundant functions?

Sure, I will take care of that.

Making progress one bug at a time... the cq_effect bug was already there but I guess the card never got that far, there was another one in the way.

In the meanwhile the Queen became a one shot lady. You can activate it, everything works, untap, the animated creature is released but then it cannot be activated anymore. GAA_UNTAPPED makes sure it can be activated the first time, but then? If I change it to GAA_NONE when the card is still tapped nothing happens (as the name indicates...) and with GAA_TAPPED the game crashes (after highlighting the card that it can be activated). The last part looks like this now (clearly wrong):

Code: Select all

// Mark as GAA_UNTAPPED so generic_activated_ability can tap it, otherwise the legacy gets released immediately
generic_activated_ability_flags_t gaa_mode = is_tapped(player, card) ? GAA_NONE : GAA_UNTAPPED;
return generic_activated_ability_with_target_in_grave(player, card, event, gaa_mode, MANACOST_XB(2,1), 0, NULL, NULL,
                                          0, &test, GS_GRAVE_RECYCLER_BOTH_GRAVES);


Do you have any hints?

[Aswan jaguar]

I don't understand what are you talking about, like drool66 said with the below code for cq_effect():

Code: Select all

int cq_effect(int player, int card, event_t event){

   if( get_card_instance(player, card)->targets[0].player > -1 ){
      card_instance_t *instance = get_card_instance(player, card);
      int cq_player = instance->targets[0].player;
      int cq_card = instance->targets[0].card;
      if( ! is_tapped(cq_player, cq_card) || other_leaves_play(player, card, cq_player, cq_card, event) ){
         kill_card(instance->damage_target_player, instance->damage_target_card, KILL_EXILE);
      }
   }

   return 0;
}

The rest of the code that you have posted and with GAA_UNTAPPED in generic_activated_ability_with_target_in_grave() that I have already fixed upstream days ago because it was an obvious mistake, the card works flawlessly:

Code: Select all

   return generic_activated_ability_with_target_in_grave(player, card, event, GAA_UNTAPPED, MANACOST_XB(2,1), 0, NULL, NULL, 0, &test, GS_GRAVE_RECYCLER_BOTH_GRAVES);

[FastEddie]

The rest of the code that you have posted and with GAA_UNTAPPED in generic_activated_ability_with_target_in_grave() that I have already fixed upstream days ago

See, that I didn't know. In the branches I am tracking, master and origin/manalink/master_sep2020 I can see no change.
When choose_to_untap has been changed the other cards using it must also be fixed and last time when I tested the AI still untapped Phyrexian Gremlins every turn, so there might be still another issue.

[Aswan jaguar]

The important part of my comment above was more to emphasize that Coffin Queen works fine while you said you still got issues. Is it now fine for you, too?
The only think to improve is that the legacy attached on Coffin Queen (or any creature using this function) should be invisible.