[fixed]Meandering Towershell creates rukh token eot

[Nexhro]

Describe the Bug:
At the end of a turn during which Meandering Towershell attacked, its controller puts a 4/4 red Rukh creature token with flying onto the battlefield and Meandering Towershell is exiled.

Which card did behave improperly ?
Meandering Towershell

Which update are you using?(date,name)Which type(Duel,Gauntlet,Sealed Deck)
KOT, duel

What exactly should be the correct behavior/interaction ?
Meandering Towershell

Are any other cards possibly affected by this bug ?
-

[Gargaroz]

It's the same problem of "effect" cards not receiving the EVENT_PHASE_CHANGED.
Stay tuned.

[Aswan jaguar]

Is this fixable now?

[gnomefry]

Just a friendly bump to mention that Meandering Towershell's legacy card is still improperly resolving as the 'ruhk egg' effect at end of turn. Meaning, it puts a 4/4 flying ruhk token into play and your giant turtle stays on permanent vacation.

Current version savegame attached.

[drool66]

Fixed in d3bcead
The issue is just that the exe assigns certain functions to legacies based on some triggers. Giving a legacy an upkeep trigger (I think) results in a Naf's Asp legacy, for example. Adding a beginning of combat trigger apparently results in a Rukh Egg legacy. In that case, you're limited to using the event, in this case EVENT_BEGIN_COMBAT (or EVENT_MUST_ATTACK, take your pick).
I also had to clean up some arguments and add an info_slot hack, otherwise it kept exiling itself again as soon as it came into play attacking.

[Korath]

That's got a kernel of truth in it, but it's not right.

The original game had a different effect card for almost every different (real) card that needed one. (It did combine stuff like "the creature this is attached to gets ±something/±somethingelse until end of turn" and "the permanent this is attached to gets/loses someability until end of turn"; the graveyard trigger effect for Onulet, Personal Incarnation, Creature Bond, Volcanic Eruption, and Abu Ja'far shared an effect card, since they were all looking for the same trigger; and Berserk, Gaea's Liege, Island Sanctuary, and Time Walk shared an effect card for no readily-apparent reason.)

Since adding an entirely different card for each effect isn't scalable - especially back when we couldn't have more than 2000 cards in the game - someone hijacked Rukh Egg's effect card to run an arbitrary card function in. Much later, I did the same with Nafs Asp's effect card for activateable effects - we still need a different effect card for each combination of different flags, like EA_ACT_ABILITY in this case, that need to be set. That function's address is stored in card_instance_t::info_slot. (And that's why essentially no savegames are compatible between different builds, let alone releases - the address of each effect implementation function will be different every time you compile.)

Since the MicroProse implementation of Rukh Egg and I think Nafs Asp were still being used, and to keep an improperly setup effect card from just segfaulting, both effect cards fall back to the original implementation if info_slot < 4096. You can see that directly in the Nafs Asp effect in functions.c:legacy_effect_activated(), and Rukh Egg's in magic.exe is the equivalent of:

Code: Select all

int
fx_rukh_egg(int player, int card, event_t event) // 0x457e10
{
  card_instance_t* inst = get_card_instance(player, card);
  if (inst->info_slot > 4096)
    ((void (*)(int, int, event_t))inst->info_slot)(player, card, event);
  else if (event == EVENT_CLEANUP)
    {
      create_token_and_put_it_into_play(player, CARD_ID_RUKH);
      kill_card(player, card, KILL_EXILE);
    }
  return 0;
}

What I expect is happening based on the symptoms but not reading your source is that the Towershell, or some function it's calling, is changing the effect card's info_slot to something low or negative, likely 0. Don't do that. You can change an effect card's info_slot to change what it does, but only to another function address, and it's a poor practice.

Since Rukh Egg and Nafs Asp have both been rewritten in C, and I doubt anything else is using the original functionality of those effects (but you'd have to check exhaustively!), it's probably safe to remove it and just replace both with

Code: Select all

int
fx_generic_effect(int player, int card, event_t event)
{
  return ((int (*)(int, int, event_t))get_card_instance(player, card)->info_slot)(player, card, event);
}

Of course, that would make it segfault if you do evil things like change its info_slot to something besides a function address, but there's plenty to be said for failing deadly instead of silently.

[drool66]

Yes, the info_slot I was referring to was that of Meandering Towershell itself, not the legacy.

But now I see that is the issue with using beginning_of_phase() (which Meandering Towershell was calling via beginning_of_combat() ) in legacies - it uses info_slot as a marker so that it doesn't trigger multiple times in a phase, and thereby overwrites the function ptr. stored there. Thank you very much for the write-up and the insight.

[FastEddie]

Thanks for the insight Korath. I was scratching my head why save games break not even between releases but also when I change some pieces of code.

Thinking aloud, would it be feasible to store the function pointers for the effect cards in a lookup table that is populated at runtime? In this case you would not store the pointer (to be precise, the address that is prone to change) but a lookup value that remains constant. Or is this pure overkill for an otherwise inconsequential issue?

but there's plenty to be said for failing deadly instead of silently

Words of great wisdom.

[Aswan jaguar]

Now there are at least 3 other issues according to rules.
1- It works vice versa with raid ability. On the turn Meandering Towershell attacks and is exiled, raid abilities don't see it as a creature that attacked and on the turn Meandering Towershell enters the battlefield attacking, raid abilities do.
Rule:
9/20/2014 On the turn Meandering Towershell attacks and is exiled, raid abilities will see it as a creature that attacked. Conversely, on the turn Meandering Towershell enters the battlefield attacking, raid abilities will not.
2- Doesn't return to controller if attacks stolen. (Used Control Magic to test if it matters) When I attacked with Meandering Towershell it made a legacy on my battlefield but on my next turn and attack phase the legacy was gone but Meandering Towershell not on my battlefield tapped as it does when you own it.
Relevant rule:
9/20/2014 If you attack with a Meandering Towershell that you don’t own, you’ll control it when it returns to the battlefield.
3- It doesn't respect this rule either (except for it's own triggered ability):
9/20/2014 If Meandering Towershell enters the battlefield attacking, it wasn’t declared as an attacking creature that turn. Abilities that trigger when a creature attacks, including its own triggered ability, won’t trigger.

[Korath]

Thinking aloud, would it be feasible to store the function pointers for the effect cards in a lookup table that is populated at runtime?

If the goal is to make savegames compatible between versions, this isn't going to be (nearly) enough. Lots and lots of other things change between releases, from the id of Rules Engine to what individual flags on cards mean.

Making savegames portable across a compile, over a short time, would be convenient, but not enough to offset the additional hassle of maintaining your lookup table. You could automate it with, say, precompilation, or compiling as C++ and doing too-clever things with static object constructors hidden behind macros like I do with Shandalar's CARD(), but it still strikes me as too much effort for too little gain. Plus, there's still nothing to stop you from accidentally overwriting an effect card's info_slot or wherever you store your index and suddenly be calling an entirely different effect's implementation, one that's a lot less obviously wrong than making a Rukh token.

(On the other hand, properly-versioned, forward-compatible savegames can be a powerful tool for automated testing.)

[drool66]

2- Doesn't return to controller if attacks stolen. (Used Control Magic to test if it matters) When I attacked with Meandering Towershell it made a legacy on my battlefield but on my next turn and attack phase the legacy was gone but Meandering Towershell not on my battlefield tapped as it does when you own it.
Relevant rule:
9/20/2014 If you attack with a Meandering Towershell that you don’t own, you’ll control it when it returns to the battlefield.

This one is easy enough to fix, and I've done so locally.

1- It works vice versa with raid ability. On the turn Meandering Towershell attacks and is exiled, raid abilities don't see it as a creature that attacked and on the turn Meandering Towershell enters the battlefield attacking, raid abilities do.
Rule:
9/20/2014 On the turn Meandering Towershell attacks and is exiled, raid abilities will see it as a creature that attacked. Conversely, on the turn Meandering Towershell enters the battlefield attacking, raid abilities will not.
...
3- It doesn't respect this rule either (except for it's own triggered ability):
9/20/2014 If Meandering Towershell enters the battlefield attacking, it wasn’t declared as an attacking creature that turn. Abilities that trigger when a creature attacks, including its own triggered ability, won’t trigger.
---

These are the same - basically it shouldn't fire any attack triggers. I overrode this for its own attack trigger using its info_slot, but I failed to consider other attack triggers. The problem I'm running into is that all other (at least that I could find) "etb tapped & attacking" effects fire on another creature's attack trigger, which means that there isn't a chance for the entering creature's attack triggers to fire (and they shouldn't). With Meandering Towershell, the legacy needs to fire at the beginning of combat even if no other creatures are attacking, which still gives Towershell a chance to fire off any attack triggers, including its own. Anyone have any ideas?

[FastEddie]

If the goal is to make savegames compatible between versions, this isn't going to be (nearly) enough. Lots and lots of other things change between releases, from the id of Rules Engine to what individual flags on cards mean.

Ah, I was afarid you would say something along those lines... at least there is a new item for my "to read" list.

(On the other hand, properly-versioned, forward-compatible savegames can be a powerful tool for automated testing.)

This was an initial pipe dream I had when I first saw the sheer amount of code. But based on what you just said this is probably even further away than sorting out the targets (establishing different targets for different purposes - I hope I got this right). Plus that someone has to code all the tests needed, which in itself is probably a full-time job for a couple of weeks.

(Explanation for the less nerdy inclined: automatic tests are little programs that run your functions with a given parameter set and expect predefined results. Here you could run cards with certain events or triggers set, for example. This would give a timely indication if old code broke accidentially or other weird things happened.)

[drool66]

Fixed control issue in 6249fa7 ("Doesn't return to controller if attacks stolen.")

This may fix everything with Meandering Towershell. In particular, I believe all of the issues here have been resolved. Provisionally marking this fixed unless anyone has any objections