[fixed]Suspend prevents planewalker abilities

[BAgate]

I thought I remembered this, but couldn't find it so...

Describe the Bug:
When you cast a card using suspend all planewalkers can no longer be used that turn.

Which card did behave improperly ?
suspend mechanic + planewalkers

Which update are you using?(date,name)Which type(Duel,Gauntlet,Sealed Deck)
Manalink 2016/08/27: Eldritch Moon v2, duel

What exactly should be the correct behavior/interaction ?
no interaction

Are any other cards possibly affected by this bug ?
-

[Aswan jaguar]

I thought I remembered this, but couldn't find it so...

I remember the same but can't find the post either.

[Korath]

Same symptoms as this.

[BAgate]

Also prevents leveling.

[Aswan jaguar]

Also suspend stops equip ability that turn at least Knight of Sursi does so.
Manalink dev 778ccb5 version - duel

[drool66]

Suspend is disabling anything that checks can_sorcery_be_played(). An emblem from Teferi, Temporal Archmage will allow loyalty abilities after suspending, as will modifying can_sorcery_be_played() to return 1 if stack_size == 1.

So suspending is leaving stack_size == 1, I think by exiling the card during EVENT_CAST_SPELL. Calling resolve_top_card_on_stack() or decrementing stack_size just before returning in suspend_a_card() will allow loyalty abilities. I'm not sure which is appropriate here. It needs more testing - for example Delay -ing something doesn't have the same issue but calls suspend_a_card(), so maybe something like this at the end of suspend_a_card():

Code: Select all

   if( t_player != -1 ){
      kill_card(t_player, t_card, KILL_EXILE);
   }
   if( player == t_player && card == t_card ){
      EXE_FN(void, 0x436740, void)();   // resolve_top_card_on_stack();
   }
   return 0;
}

[Korath]

That's dangerous. The stack being corrupted to be too large will prevent sorceries until it's reset at the start of the turn. The stack being corrupted to be too small will crash. Just checking that the card is suspending itself isn't sufficient to tell that it needs to be popped off the stack - there's cards like Arc Blade that suspend themselves on resolution, and the play_spell_for_free family of functions call EVENT_CAST_SPELL without putting cards on the stack first.

What's happening is that when, during the dispatch of EVENT_CAST_SPELL late in put_card_on_stack(...second_call=0), a card is removed but cancel isn't set true, the rest of the put_card_on_stack(...0), put_card_on_stack(...1), put_card_on_stack3() sequence is aborted without unsetting LCBP_SPELL_BEING_PLAYED or calling resolve_ or obliterate_top_card_of_stack(). (The exact sequence is the card exiles itself during EVENT_CAST_SPELL, which sets its internal_card_id to -1; put_card_on_stack(...0) returns true; put_card_on_stack(...1) sees internal_card_id < 0 and immediately returns 0; put_card_on_stack3(), which cleans up this up, is never called.)

So put_card_on_stack() needs to be moved into C, at least when second_call is 0. I see that the version in engine.c has already been fiddled with, despite it being disabled (for instance, by setting STATE_SICKNESS_UNUSED, which still needs to be renamed). It needs to be checked carefully against current assembly first, since it's missing at least one patch, patch_make_manasource_interrupts_interruptible.pl.

[drool66]

It needs to be checked carefully against current assembly first, since it's missing at least one patch, patch_make_manasource_interrupts_interruptible.pl.

What do you recommend using to translate asm to C?

[Korath]

Hex-Rays is the industry standard, if you're independently wealthy and don't mind either paying for it again or losing your data every few years. Ghidra's free and reliable, but awkward, slow (especially with long functions), and less accurate.

Both are only good for getting a sense of what's going on. If you need accuracy - and with a function as critical as this, you do - you really need to do it manually. The good news is that it's easier than it seems: x86 assembly isn't especially difficult unless you're dealing with highly-machine-optimized compilation product (and you aren't), and you already have a mostly-accurate approximation in C to check it against (much better than either Hex-Rays or Ghidra will output without a great deal of preliminary work). It is much more verbose than C, especially when dealing with arrays and structures; so I've found it helpful to paste disassembly output from the function's section in objdump -Mintel -d Magic.exe into an editor and go through it line by line translating it into pseudo-C, like in the commentary in individual files in src/patches/.

I've had good experiences with an earlier edition of Paul Carter's PC Assembly Language for learning syntax and what individual instructions do.

[drool66]

Thank you!

Moved into C in b0e4359 sans the changes you suggest above - I'll get to those soon

[drool66]

Korath, I just put in calls to put_card_on_stack3() and schedule_call_to_redisplay_all() at the end of put_card_on_stack(...0) if cancel != 1 && iid == -1. Does that sound right? It does clear up this issue. Looks like this:

Code: Select all

           if (cancel != 1)
            {
              if (player == 1 && !(trace_mode & 2) && ai_is_speculating == 1
                 && cd->cc[1] == 255)
               ai_modifier -= 12 * EXE_FN(int, 0x435940, int)(1);   //get_ai_modifier_for_x_spell(1);
            //begin additions
              if (instance->internal_card_id == -1)
               {
                 put_card_on_stack3(player, card);
                 schedule_call_to_redisplay_all();
               }
            //end additions
              return 1;
            }
         }

[Korath]

I'd call obliterate_top_card_of_stack() and unset LCBP_SPELL_BEING_PLAYED directly. While that's what put_card_on_stack3() currently does for an internal_card_id of -1, there's no guarantee that it'll continue to do so - it's not what a call to put_card_on_stack3() means semantically, which is something similar to "a spell was successfully cast and wasn't interrupted".

That call to charge_spell_cost() is going to break, and very badly. It's got a nonstandard calling convention - it takes player and card on the stack like a normal cdecl, but also a pointer to a card_data_t in the edi register. You're passing it a whole card_data_t on the stack. Right now, for me, the assembly gcc produces to copy the struct onto the stack happens to put its address in the edi register too, but there's nothing constraining it to do so. You'll get different results with different versions of gcc, different optimization levels, etc. More immediately, it changes where player and card are on the stack, so if you run the game with a Magic.exe that has your patches in place (the one you committed doesn't; it just changes data) or use a codepath that calls your versions of the functions directly (like by clicking a Dark Ritual while paying a mana cost, or copying a spell), you'll see it crashes anyway.

[drool66]

(the one you committed doesn't; it just changes data)

Right, I patched and ran it through make in magic_updater, forgetting to overwrite magic.exe in the base dir first. Bummer, because put_card_on_stack() crashes every time it's called - even if charge_spell_cost() is cancelled out. I took some time to import charge_spell_cost() and 0x402593 to standard calls in C, but that didn't work and I have no idea how to keep it from crashing. I'll poke around some more but unless I get a flash of insight I'll revert the patch in b0e4359 and see if I can band-aid this bug in suspend()

[Korath]

Importing both of those (and, yes, 0x402593 is nonstandard too) is the right way to go about it. They were both written in assembly rather than compiled, so the decompilation's going to be unreliable.

But if that doesn't work, the next easiest way to deal with it is to write an asm shim similar to _call_card_fn_impl, which puts its arguments where charge_spell_cost() expects them.

even if charge_spell_cost() is cancelled out.

That part's worrying, though. I'll try to find time to poke at it this weekend.

[Korath]

I can't speak to your rewrite, but with a shim (see branch wip/charge_spell_cost, and feel free to delete it afterwards), the only crash I'm still seeing is caused by the misdeclaration of get_displayable_cardname_from_player_card() as returning a char instead of a char*.