[fixed]Skaab Ruinator doesn't return if creatures below

[Aswan jaguar]

Describe the Bug:
1- Skaab Ruinator can be cast from graveyard at instant speed,opponents turn.
2- Sometimes you pay the cost to cast it from graveyard exiling 3 creatures but Skaab doesn't return to battlefield.Also 1 time I activated his ability from graveyard and it was returned to me Mayor of Avabruck that was in graveyard.
Which card did behave improperly ?
Skaab Ruinator

Which update are you using?(date,name)Which type(Duel,Gauntlet,Sealed Deck)
Manalink 2014/11/14: Khans of Tarkir,duel

What exactly should be the correct behavior/interaction ?
1- Skaab Ruinator can only be cast from graveyard at your main phases.
2- you always get back from graveyard Skaab Ruinator when you exile 3 other creatures.

Are any other cards possibly affected by this bug ?
-

[Gargaroz]

Bug 1 fixed in a4725ee
Bug 2 is more troublesome, stay tuned.

[Aswan jaguar]

What is wrong here was mentioned by Korath in another topic-a part quoted:

Korath said:
Skaab Ruinator is unrelated. I expect it fails iff any of the removed creatures were below it in the graveyard, since it doesn't make any attempt to track its own changed position.

viewtopic.php?f=110&t=17520&p=189352&hilit=Skaab+Ruinator#p189352
I confirm that if any of the creatures are below Skaab Ruinator it doesn't return to battlefield and at least sometimes it returns another permanent instead.

[drool66]

Fixed in d395956

[Aswan jaguar]

All previous bugs have been fixed. I can only find that Skaab Ruinator isn't cast from graveyard when you cast it from there as it doesn't trigger cards like Burning Vengeance or Animar, Soul of Elements like Gravecrawler does. Remember that Scrapheap Scrounger correctly doesn't and shouldn't trigger the above cards as it is an activated ability from graveyard and returns to battlefield.

[drool66]

Fixed the last bug (tested with Animar), uncommitted

[Aswan jaguar]

I don't know what is happening but it mostly doesn't work in +20 tries casting Skaab Ruinator from the graveyard Animar, Soul of Elements triggered ability to place counters on it worked only twice and one of those tries there was also a Burning Vengeance in play. The other ability of Animar, Soul of Elements that creatures cast cost less works fine.

[drool66]

Got it - for me it worked the first time I cast from grave, but not thereafter. It was the skaabs() function interfering with the graveyard ability. Fixed in 47fd60b

[Aswan jaguar]

Sorry to ruin it for you but the trigger for cards like Animar, Soul of Elements is still erratic, in my tests mostly doesn't work.

[drool66]

It worked three times in a row in three games, so I figured it was fixed - my bad

It looks like whether TRIGGER_SPELL_CAST is dispatched ultimately depends on whether put_card_on_stack() returns true or not. (ref: real_play_spell_for_free() ). I really can't untangle that function at 0x4338F0 with its 18 variables when I don't even know what the constants it's referencing are, so I can't work backwards any further. It seems to me that the oversight here is that real_put_into_play() dispatches EVENT_CAST_SPELL but not TRIGGER_SPELL_CAST. Without getting into the shortcomings of the play_spell_for_free system (and there are plenty), I think adding this trigger dispatch is a good idea - after doing so, I cannot get Skaab Ruinator not to trigger. Any thoughts either way?

[Korath]

Isn't real_put_into_play() what Manalink uses for cards like Elvish Piper? Those shouldn't be dispatching TRIGGER_SPELL_CAST. Or, really, EVENT_SPELL_CAST, which is probably just a hack so auras attach.

Shandalar's put_card_on_stack() no longer much resembles the original, so no help in readability there. A quick cleanup from the decompilation looks like

this | Open

Code: Select all

int
put_card_on_stack2(int player, int card)   // 0x433850
{
  int changed_interrupts_only_global = 0;

  if (current_trigger_or_event == TRIGGER_SPELL_CAST)
    {
      interrupts_only = 1;
      changed_interrupts_only_global = 1;
    }

  if (put_card_on_stack(player, card, 0)
      && put_card_on_stack(player, card, 1)
      && put_card_on_stack3(player, card))
    {
      if (changed_interrupts_only_global)
        interrupts_only = 0;
      return 1;
    }
  else
    {
      if (changed_interrupts_only_global)
        interrupts_only = 0;
      return 0;
    }

  return result;
}

int
put_card_on_stack(int player, int card, int second_call)   // 0x4338f0
{
  /* This is essentially two different functions always called in succession, split apart so that the AI can speculate
   * in between (when they're called from main_phase()).  They share a very brief prologue and epilogue.
   *
   * The first call ensures the card is playable, charges mana, and dispatches EVENT_CAST_SPELL.
   *
   * The second allows interrupts and fixes up the copy of the card on the stack. */

  // initialization common to both calls
  card_instance_t* instance = get_card_instance(player, card);

  int iid = instance->internal_card_id;
  if (iid < 0)
    return 0;

  card_data_t* cd = card_data_pointers[iid];
  color_t color = single_color_test_bit_to_color_t(cd->color);

  int stash_card_on_stack_iid;
  int stash_card_on_stack_controller;
  int stash_card_on_stack;
  if (!second_call)
    {
      if (trace_mode & 2)
        {
          int count = dword_60EC40++;
          char str[500];
          sprintf(str, "%d: Player %d is playing %s(%d).\n", count, player, cd->name, card);
          append_to_trace_txt(str);
        }

      if (!compute_and_check_casting_cost(player, player, card))
        return 0;

      ai_branch_value_byte0_card_byte1_player = -1;
      suppress_charging_spell_cost = 0;
      max_x_value = -1;
      if (((cd->type & (TYPE_SPELL | TYPE_ENCHANTMENT))
           || cd->id == CARD_ID_VESUVAN_DOPPELGANGER
           || cd->id == CARD_ID_CLONE_EXE)
          && !dispatch_event_to_single_card(player, card, EVENT_CAN_CAST, 1 - player, -1))
        return 0;

      cancel = -1;

      // how never to use globals.
      stash_card_on_stack_iid = card_on_stack_iid;
      stash_card_on_stack_controller = card_on_stack_controller;
      stash_card_on_stack = card_on_stack;

      card_on_stack_iid = iid;
      card_on_stack_controller = player;
      card_on_stack = card;

      put_card_or_activation_onto_stack(player, card, EVENT_RESOLVE_SPELL, player, 0);

      increment_mana_spent_index();
      if (!suppress_charging_spell_cost)
        charge_spell_cost(cd, player, card);

      max_x_value = -1;
      if (iid != -1)
        {
          int p;
          if (!(instance->token_status & STATUS_OBLITERATED))
            {
              p = player;
              --hand_count[p];
            }
          // else p is left unitialized - probably broken when charge_spell_cost() was replaced

          type_t typ = cd->type;
          if (typ & TYPE_CREATURE)
            ++creature_count[p];
          if (typ & TYPE_ARTIFACT)
            ++artifact_count[p];
          if (typ & TYPE_ENCHANTMENT)
            ++enchantment_count[p];

          types_of_cards_on_bf[p] |= typ;

          redraw_libraries();
          instance->state |= STATE_SUMMON_SICK | STATE_INVISIBLE;
          land_can_be_played |= LCBP_SPELL_BEING_PLAYED;

          card_on_stack_iid = stash_card_on_stack_iid;
          card_on_stack_controller = stash_card_on_stack_controller;
          card_on_stack = stash_card_on_stack;

          if (cancel == 1)
            decrement_mana_spent_index();
          else
            {
              if (dispatch_event(player, card, EVENT_CAST_SPELL))
                cancel = 1;

              if (cancel == 1)
                refund_spent_mana_to_pool(player);

              decrement_mana_spent_index();

              if (cancel != 1)
                {
                  if (player == 1 && !(trace_mode & 2) && ai_is_speculating == 1
                      && cd->cc[1] == -1)
                    ai_modifier -= 12 * get_ai_modifier_for_x_spell(1);
                  return 1;
                }
            }
        }
    }
  else
    {
      stash_card_on_stack_iid = card_on_stack_iid;
      stash_card_on_stack_controller = card_on_stack_controller;
      stash_card_on_stack = card_on_stack;

      card_on_stack_iid = iid;
      card_on_stack_controller = player;
      card_on_stack = card;

      if (!(cd->type & TYPE_LAND))
        recopy_card_onto_stack(1);

      type_t typ = cd->type;
      if (!(typ & TYPE_LAND)
          && !(typ == TYPE_INTERRUPT && (cd->extra_ability & EA_MANA_SOURCE)))
        {
          if (dword_628C7C)
            dword_628C7C |= 2u;

          sprintf(global_all_purpose_buffer, str_PROMPT_CHECKFEPHASE_1 /* "Trying to cast %s" */,
                  get_displayable_cardname_from_player_card(card_on_stack_controller, card_on_stack));
          allow_response(-2, current_phase, global_all_purpose_buffer, TRIGGER_SPELL_CAST);

          if (dword_628C7C & 2)
            {
              schedule_call_to_redisplay_all();
              sleep(3000);
            }
        }

      card_on_stack_iid = stash_card_on_stack_iid;
      card_on_stack_controller = stash_card_on_stack_controller;
      card_on_stack = stash_card_on_stack;

      if (instance->internal_card_id == -1)
        cancel = 1;

      land_can_be_played &= ~LCBP_SPELL_BEING_PLAYED;

      /* STATE_POWER_STRUGGLE is set on cards that are controlled by player 1 absent any change-of-control effects,
       * like STATE_OWNED_BY_OPPONENT is set for cards currently controlled by player 1.  Neither of their names are
       * remotely right. */
      instance->state |= (player ? STATE_POWER_STRUGGLE | 0) | STATE_JUST_CAST;

      if (ai_is_speculating != 1)
        set_stack_damage_targets();

      if (cancel != 1)
        {
          if (player == 1 && ai_is_speculating != 1 && !dword_62C36C && !(cd->type & TYPE_LAND))
            {
              load_text(0, "PROMPT_CAST1");

              char str[300];
              sprintf(str, text_lines[0] /* "%s plays..." */, opponent_name);
              if (cd->cc[1] == -1)
                sprintf(str, text_lines[1] /* "%s plays...\nX is %d." */, opponent_name, x_value);

              target_t tgt;
              if (instance->number_of_targets == 1)
                tgt = inst->targets[0];
              else
                tgt.player = tgt.card = -1;

              raw_do_dialog(1, card, tgt.player, tgt.card, str, 0);
            }

          schedule_call_to_redisplay_all();
        }
    }

  // epilogue common to both calls
  if (cancel == 1)
    {
      int p;
      if (instance->token_status & STATUS_OBLITERATED)
        p = kill_card(player, card, KILL_BURY);
      else
        {
          p = player;
          if (instance->internal_card_id != -1)
            ++hand_count[player];
        }

      type_t typ = cd->type;
      if (typ & TYPE_CREATURE)
        --creature_count[p];
      if (typ & TYPE_ARTIFACT)
        --artifact_count[p];
      if (typ & TYPE_ENCHANTMENT)
        --enchantment_count[p];

      redraw_libraries();

      instance->state &= ~(STATE_SUMMON_SICK | STATE_JUST_CAST | STATE_INVISIBLE | STATE_IN_PLAY);

      if (player == 1 && !(trace_mode & 2))
        dword_7A372C = 1;

      cancel = 0;

      obliterate_top_card_of_stack();

      land_can_be_played &= ~LCBP_SPELL_BEING_PLAYED;

      return 0;
    }
  else
    {
      set_timestamp(player, card);

      return 1;
    }
}

[drool66]

Isn't real_put_into_play() what Manalink uses for cards like Elvish Piper? Those shouldn't be dispatching TRIGGER_SPELL_CAST.

Oh yeah, it is. I just saw EVENT_SPELL_CAST and assumed it was in the right place without considering it any further than that. TRIGGER_SPELL_CAST should then probably be dispatched from real_play_spell_for_free(), immediately after real_put_into_play(), correct? Tangentially, I've been meaning to look into adding a "mode" argument to these play for free and put into play functions, such as whether it was really cast for free, was really cast, etc., which would govern which flags are added, which events are dispatched and so on.

Or, really, EVENT_SPELL_CAST, which is probably just a hack so auras attach

If that is indeed true, would it be better to check for SUBTYPE_AURA via has_subtype_by_id(), and if so call_card_function(arg1, arg2, EVENT_SPELL_CAST); instead of a full dispatch? Or perhaps dispatch_event_to_single_card()?

A quick cleanup from the decompilation looks like

Thank you very much, this is really helpful. Any way for a scrub like me to do this or is it just a matter of cataloging the functions & variables? The best I have is this.

[drool66]

Added TRIGGER_SPELL_CAST to real_play_spell_for_free() in d62f8b7, marking fixed unless anyone has any objections