[fixed]Coastal Breach v. Oblivion Ring

[Korath]

Describe the Bug:
If I Oblivion Ring a permanent owned by the AI, then cast Coastal Breach (on my turn), the permanent exiled by Oblivion Ring ends up in the AI's hand.

Which card behaved improperly?
Coastal Breach.

Which update are you using? (date, name)Which type? (duel, gauntlet, sealed deck)
5b9c0b07d master.

What exactly should be the correct behavior/interaction?
Everything should be bounced as close to simultaneously as can be managed, leaving the previously-exiled permanent on the bf. The closest Manalink can come is to mark all objects that would be affected, then bounce those. new_manipulate_all() does exactly that, but it's defeated here by being wrapped in an APNAP. This makes it mark all of the active player's objects, bounce them (and deal with Manalink's instantaneous triggers and effects), then mark all of the non-active player's objects, then bounce those.

The sad part is that new_manipulate_all() internally marks and acts on objects it matches in active-not-active order, so the APNAP() wrapping doesn't get us anything even in principle.

Are any other cards possibly affected by this bug?
Every single hit found by "git grep -n APNAP.*new_manipulate_all". A few of the ones found by "git grep -5n -e APNAP --and --not -e APNAP.*new_manipulate_all|grep -5 new_manipulate_all" too.

Upheaval is a particularly awful case, in that it APNAPs bouncing all enchantments, then APNAPs bouncing all permanents, using four new_manipulate_all() calls when the only correct thing to do is to call it exactly once. The way you fix Auras going to the graveyard instead of bouncing is by temporarily suppressing that specific behavior, not by doing things materially different from what the card says to do.

[drool66]

Wow thats a lot of hits. I'll go through ahead of the next patch.

[drool66]

The way you fix Auras going to the graveyard instead of bouncing is by temporarily suppressing that specific behavior, not by doing things materially different from what the card says to do.

I'm not sure exactly how to suppress that behavior, or even where auras are destroyed to SBAs when their targets leave the bf. The best I can do is to sort the marked[] array with auras at the front, like so:

Code: Select all

int new_manipulate_all(int player, int card, int t_player, test_definition_t* this_test, actions_t raw_action)
{
   int result = 0;
   int marked[2][100];
   int mc[2] = {0, 0};

   // First step: mark the cards which passes the test.
   int i, c;
   for (i = 0; i < 2; i++){
      int p = i == 0 ? current_turn : 1-current_turn;
      if (t_player == ANYBODY || p == t_player){
         counter_t counter_type = COUNTER_invalid;
         int counter_num = 0;
         actions_t action = cook_action(p, raw_action, &counter_type, &counter_num);
         if (action == (actions_t)-1)      // action invalid for both players
            return result;
         else if (action == (actions_t)-2)   // action invalid for this player, maybe not for the other
            continue;

         for (c = active_cards_count[p] - 1; c >= 0; --c){
            if(((this_test->zone == TARGET_ZONE_IN_PLAY && in_play(p, c) && is_what(p, c, TYPE_PERMANENT) && !check_state(p, c, STATE_CANNOT_TARGET))
               || (this_test->zone == TARGET_ZONE_HAND && in_hand(p, c)))
               && new_make_test_in_play(p, c, -1, this_test)
               && (this_test->not_me == 0 || !(player == p && c == card))
              ){
               marked[p][mc[p]] = c;
               mc[p]++;
            }
         }
      }
   }

   //move Auras to the front of the line if we're bouncing permanents
   if( raw_action == ACT_BOUNCE ){
      int p;
      for( p=0;p<2;p++){
         for( i=0;i<(mc[p]-1);i++){
            if( !has_subtype(p, marked[p][i], SUBTYPE_AURA) && has_subtype(p, marked[p][i+1], SUBTYPE_AURA) ){
               int j, stored_card = marked[p][i+1];
               for( j=i;j>0;j-- ){
                  marked[p][j+1] = marked[p][j];
               }
               marked[p][0] = stored_card;
            }
         }
      }
   }

...then move on to doing the action on the cards


It's an extra loop, but at least better than another call to new_manipulate_all(). Anyone have any thoughts?

[Korath]

It's in destroy_attached_auras_and_obliterate_card().

[drool66]

Fixed in 0dbd223 and 03f753c using implementation above - see comments