[fixed]Mimic Vat issue beyond 1 creature imprinted

[Aswan jaguar]

Describe the Bug:
Mimic Vat imprints only the first time you use the trigger. Next time you try to imprint another creature it doesn't get imprinted, the 1st creature doesn't return from exile to owner's graveyard and Mimic Vat can't create a token.

Which card did behave improperly?
Mimic Vat

Which update are you using? (date, name)Which type? (duel, gauntlet, sealed deck)
dev b5544df

What exactly should be the correct behavior/interaction?
Imprint — Whenever a nontoken creature dies, you may exile that card. If you do, return each other card exiled with Mimic Vat to its owner's graveyard.

Are any other cards possibly affected by this bug?
-

[FastEddie]

Some more colour...

Mimic Vat works, but only for the first creature imprinted. I attached two save games, with the first it doesn't work whereas with the second it does if you imprint the Killer Bees (which was the first creature I imprinted).

[FastEddie]

Fixed and sent to Aswan Jaguar. Diff is attached, the relevant portion of the code can be found below.

Code: Select all

   int result = resolve_gfp_ability(player, card, event, RESOLVE_TRIGGER_AI(player));
   if( result ){
      card_instance_t *instance = get_card_instance(player, card);
      int amount = BYTE0(result);
      int dead[2][19];
      int i;
      // If no card has been imprinted yet previous_iid == -1
      int previous_iid = instance->targets[9].card;
      for (i = 0; i < amount; i++){
         dead[0][i] = instance->targets[i].player;
         dead[1][i] = instance->targets[i].card;
         instance->targets[i].player = instance->targets[i].card = -1;
      }
      test_definition_t test;
      new_default_test_definition(&test, TYPE_CREATURE, "Select a creature card to imprint.");
      int selected = select_card_from_zone(player, player, dead[1], amount, 0, AI_MAX_VALUE, -1, &test);
      if( selected != -1 ){
         int owner = dead[0][selected];
         int iid = dead[1][selected];
         // Store previously imprinted card if there is one
         int foundImprintedCard = ( previous_iid > -1 );
         int imprintedCardPlayer = -1;
         int imprintedCardCard = -1;
         if ( foundImprintedCard ) {
            imprintedCardPlayer = instance->targets[9].player;
            imprintedCardCard = instance->targets[9].card;
         }
         // Card going to the graveyard is imprinted
         for(i = count_graveyard(owner)-1; i > -1; i--){
            if( get_grave(owner)[i] == iid ){
               instance->targets[9].player = owner;
               instance->targets[9].card = iid;
               rfg_card_from_grave(owner, i);
               break;
            }
         }
         // If a card has been imprinted it goes to the graveyard
         if( foundImprintedCard ){
            int position = find_iid_in_rfg(imprintedCardPlayer, imprintedCardCard);
            if( position != -1 ){
               from_exile_to_graveyard(imprintedCardPlayer, position);
            }
         }
      }
   }


[Aswan jaguar]

Inserted above fix in commit b228629. FastEddie congratulations on your first fix and commit.

[FastEddie]

[drool66]

I hate to rain on the parade, but FastEddie, do you realize targets[9].card will be overwritten:

card_instance_t *instance = get_card_instance(player, card);
int amount = BYTE0(result);
- int dead[2][9];
+ int dead[2][19];
int i;
+ // If no card has been imprinted yet previous_iid == -1
+ int previous_iid = instance->targets[9].card;
for (i = 0; i < amount; i++){
dead[0][i] = instance->targets[i].player;
dead[1][i] = instance->targets[i].card; <----------------HERE

if more than 10 cards go to the GY? I think you can still use info_slot or maybe even eot_toughness to store iids. Also, targets[10]-[18] aren't really free for general use as they are used for global effects (one of the suckier things about coding for Manalink). I can provide more info on this if you'd like.

[Aswan jaguar]

You mean if more than 10 creatures go to graveyard at once eg if a Damnation kills more than 10 creatures?

[drool66]

Yes - when you resolve the gfp ability and fill the dead[][] array, it will overwrite the targeting array up to the number of creatures sent to gy. This could also overwrite, among other things, targets[9].card, used to store the imprinted id, and targets[11], which is used universally to keep track of permanents leaving the bf which you can see it used in Mimic Vat's EVENT_GRAVEYARD_FROM_PLAY (although by the time it's overwritten, it's already been read by Mimic Vat, but still can't be good)

[FastEddie]

This was original code so there is at least one other issue with it. Maybe info_slot is the better way to go, but we still have the issue that target[] is limited... let me look into that again.

[FastEddie]

A shot from the hip:

As a quick Fix limit the number of targets by using a min function. I was wondering about targets[] for a while...

If we could deprecate one target position, ideally #19, but any would do we could squeeze in a list<target_t>*. 64 bits of space should do. This could store any number of targets. Having a fixed length array and a potentially unlimited number of targets is a nice highway to hell... either cutting the array off at the end or making the target_t a union would allow to plug in a list*.

I know this is an ungodly hack but... info_slot seems to get overwritten, see Semblance Anvil, and eot_toughness is used elsewhere according to the header. Besides I don't see 64 bits worth of unused space together in card_instance.

[FastEddie]

And yes, I would love to learn more and to document it somewhere, either in the code or in the dev forum.

[FastEddie]

Finally, if we go down the main route I would add a pragma like in future_sight.c so that a warning is given at compile time. This is the least we could do.

[drool66]

Yes, the original code limited dead creatures to 9, filled those into targets[0]-[8], and kept track of the imprinted creature in [9]. info_slot usually doesn't get overwritten; like I said, I don't know why it does in Semblance Anvil, at least not by looking at it for the two minutes I took to do it (fwiw one of info_slot's main uses is to hold the value of )
Here is my best breakdown of the targeting array & cc[2]; I think only 1 or 2 could possibly be ripe for depreciation & reclamation.

If we could deprecate one target position, ideally #19, but any would do we could squeeze in a list<target_t>*. 64 bits of space should do. This could store any number of targets[...] either cutting the array off at the end or making the target_t a union would allow to plug in a list*.

Ya, if you can figure out how to have persistent storage of an ~unlimited number of targets, I say go for it; it would be a huge breakthrough. Backward compatibility though...

Having a fixed length array and a potentially unlimited number of targets is a nice highway to hell...

Haha, welcome to Manalink development

[FastEddie]

Thanks for the write-up on targets.

I have a nagging suspicion that you deliberately direct me to one rabbit hole after another... Anyway, I created a new thread here to get the discussion going (this will be a major change if it is done):

https://www.slightlymagic.net/forum/viewtopic.php?f=56&t=29885

[FastEddie]

Yes, the original code limited dead creatures to 9, filled those into targets[0]-[8], and kept track of the imprinted creature in [9]. info_slot usually doesn't get overwritten

You haven't been a long-time guest in the Korath Parallel Universe where words like "usually" loose their meaning . Joking aside, I re-wrote the imprint mechanic to accomodate for such and worse cases. The commit(s) is/are 1c94ef199 (original) and dae11776f (merge) as I had to merge which required another one.
To convince yourself that the current implementation is not quite optimal simply run Korath's Die Hard Test Battery ((TM), patent pending) and give yourself a couple of Twiddle to keep untapping;

1. Imprint a Grizzly Bears you own onto your Mimic Vat.
2. Animate your Vat.
3. Cytoshape it into a copy of a Crimson Hellkite, and activate it to deal damage.
4. Cytoshape it into a copy of Magus of the Candelabra and activate it to untap 10 lands.
5. Cytoshape it into a copy of Admonition Angel and let it trigger for a bunch of Gray Ogres.
6. Let the copy effects wear off.
7. Activate your Vat.
8. Pull from Eternity the Grizzly Bears.
9. Tormod's Crypt your graveyard.
10. Activate your Vat.

Q1: Do you get a Grizzly Bears token after step 7?
Q2: Do you get a Grizzly Bears token after step 10?
Q3: If you killed another creature before step 8 and let your Vat trigger, did you get your Grizzly Bears back? How about any of the Gray Ogres?

If your answers are anything other than "yes", "no", and "no and no", then your design doesn't work.

Depending on how much damage Crimson Hellkite deals the current implementation breaks after step 3 or 4. 4 is definitely the end.

I also reworked Semblence Anvil and added a option to show the imprinted card. Clone Shell was also in the works but doesn't work properly yet (which is progress as the original implementation shouldn't work at all but this is nothing I am proud of).

I will look at the other cards with imprint that don't use Korath's exileby package and adjust them accordingly. Right now I stopped Mimic Vat from gnawing at my conscience and this thread can be finally archived .

One final word (ok, several): I added some comments in case someone implements Teysa Karlov some day. Since I don't know how her mechanic will work I didn't prepare anything as there are at least two ways to accomodate the coming change. I guess this is why so far info_slot was used to count the number of targets.