[fixed]Jayemdae Tome AI activates with no mana infinite loop

[FastEddie]

Describe the Bug:
The AI plays Jayemdae Tome and ends up in an infinite loop trying to activate it. So far this bug occured exclusively with Jayemdae Tome, but I think the issue might be the way available mana is counted. Maybe Soldevi Excavations also play a role.

Which card behaved improperly?
Jayemdae Tome (although it is probably not the guilty party)

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

What exactly should be the correct behavior/interaction?
The AI should not try to activate Jayemdae Tome as it has no mana.

Are any other cards possibly affected by this bug?
Other artifacts with an activation cost, maybe only artifacts where activation cost is equal to casting cost.

Edit: added another example that involved Martyr of Ashes. Same behaviour, AI had 1 mana open but tried to activate for 2.

[Korath]

Which commit, exactly? Never mind, seeing this in b35b47c8b.

Autosaves aren't very helpful for dev versions; they usually can't be loaded by anyone except yourself. The game state can be extracted from them, but it's a pain to do. Screenshots work better.

Soldevi Excavations isn't necessary; something that calls declare_mana_available_hex() is. count_mana() isn't clearing raw_mana_available_hex[1][0], so it fills up with e.g. {1,COLOR_TEST_RED|COLOR_TEST_WHITE}=(1<<16)|(0x30)=65584 when it the AI has a Plateau. x/50dw 0x4ef2e8 under gdb shows the problem definitively.

[FastEddie]

Didn't see your reply, Korath. I thought the save games work for you guys if I use the latest version? Screenshot wouldn just show the error message I'm afarid.

[Aswan jaguar]

I loaded your savegame and can confirm this with it and without. I don't know of course if the route problem is what Korath says but having ai_modifier() just when event == EVENT_ACTIVATE like it was seems it lets AI go on loop and also tap lands to pay the cost even if AI couldn't pay full cost.
Code was:

Code: Select all

int card_jayemdae_tome(int player, int card, event_t event){
/* CARD_ID_JAYEMDAE_TOME   131
Jayemdae Tome   |4
Artifact
|4, |T: Draw a card. */

   if( event == EVENT_ACTIVATE )
        ai_modifier += (count_subtype(player, TYPE_LAND, -1) * 8);

   if( event == EVENT_RESOLVE_ACTIVATION ){
      draw_cards(player, 1);
   }

   return 0;
}

Change to this and no issue happens:

Code: Select all

int card_jayemdae_tome(int player, int card, event_t event){
/* CARD_ID_JAYEMDAE_TOME   131
Jayemdae Tome   |4
Artifact
|4, |T: Draw a card. */

    if( event == EVENT_CAN_ACTIVATE ){
      return generic_activated_ability(player, card, event, GAA_UNTAPPED, MANACOST_X(4), 0, NULL, NULL);
   }

   if( event == EVENT_ACTIVATE ){
       if( generic_activated_ability(player, card, event, GAA_UNTAPPED, MANACOST_X(4), 0, NULL, NULL)){
          ai_modifier += (count_subtype(player, TYPE_LAND, -1) * 8);
       }
   }

   if( event == EVENT_RESOLVE_ACTIVATION ){
      draw_cards(player, 1);
   }

   return 0;
}

[Korath]

That just stops encouraging the AI to try to activate it repeatedly whether activation is successful or not. It doesn't disallow trying to activate it, which it the bug here. It also no longer encourages the AI to activate when activation is successful, since generic_activated_ability() never returns nonzero for EVENT_ACTIVATE; you want to check whether cancel == 1 afterward instead.

[gnomefry]

Playing today on ee04575f I was seeing this behavior with Treasure Trove. For several turns the AI was gassing up the activations with a series of attempts, eventually easing off and letting the game proceed. I'm attaching a save from the turn it went infinite.

version 10-2020 1.1 ee04575f - gauntlet

[Korath]

Well, yes. It'll affect anything that checks whether the AI has mana, so long as the AI has had ever controlled anything that can add more than one color. Nothing to do with Treasure Trove or Jayemdae Tome or any other specific activated ability.

[drool66]

Fixed by clearing raw_mana_available_hex in count_mana(). Since it has 100 elements I just used:

memset( raw_mana_available_hex, 0, sizeof(raw_mana_available_hex) );

[Korath]

That makes it impossible for mana to be declared. Values of 0 filling that array don't mean empty, they mean each slot is taken by a mana producer that can produce 0 mana of any color in COLOR_TEST_NONE. As you'll see if you test, even as player 0 now, with trying to cast or activate anything with only dual lands to make mana with. You've got to look at what these globals actually do, not just flail about until something happens to work once.

Code: Select all

//----- (0049D240) --------------------------------------------------------
void
declare_mana_available_hex(int player, color_test_t colors, int amount)
{
  int i; // [sp+Ch] [bp-8h]
  int done; // [sp+10h] [bp-4h]

  if (colors > 0)
    {
      done = 0;
      for (i = 0; i < 50 && !done; ++i)
        if (raw_mana_available_hex[player][i] == -1)
          {
            done = 1;
            raw_mana_available_hex[player][i] = colors | (amount << 16);
            raw_mana_available_hex[player][i + 1] = -1;
          }
    }
}

and it's read similarly in check_mana_multi().

The way to clear it is to set both raw_mana_available_hex[0][0] and raw_mana_available_hex[1][0] to -1, not just raw_mana_available_hex[0][0]. You can set every element to -1 if you want, but there's no need to.

OK, I'm very confused. My testing showed results consistent with raw_mana_available_hex[1][0] not being cleared, and I'd've sworn when I looked at count_mana()'s recent move into C it wasn't. But it is, and has been since the first commit there. Investigating some more. (But memsetting to 0's definitely wrong.)

[Korath]

The problem isn't that raw_mana_available_hex[1][0] isn't being set to -1, because it is. The problem's that raw_mana_available_hex[][]'s dimensions are misdeclared in manalink.h - it's an array of [2][51], not [2][50], and so &(raw_mana_available_hex[1][0]) should be 0x4ef21c + (4 * 51) = 0x4ef2e8. (The source for these addresses is dissassembly of Magic.exe, 0x472400-0x47243a.) So when produce_mana.c tries to write into raw_mana_available_hex[1][0], it's being written to address 0x4ef2e4, which is raw_mana_available_hex[0][50] (or, equivalently, raw_mana_available_hex[1][-1]) instead.

The real wonder is that its other uses in ManalinkEh.dll ever even sort of worked. There aren't many. The ones in oath_of_the_gatewatch.c made it so the first multiple-type mana source the AI has can't be seen for the purposes of finding colorless-only mana. The ones in legends.c are incorrect either way; they're reading raw_mana_available_hex[0 or 1][7] and expecting it to be the total amount of mana, when it's really the colors and amount of that player's eighth-declared multiple-type mana source.

[drool66]

Whoa crazy
I set the dimensions in manalink.h, which clears up the fault here;

in OGW it looks like I just have to change the two relevant lines to:
for (i = 0; i < 51 && raw_mana_available_hex[player][i] != -1; ++i){

Legends was just me being cute on the ai modifier for Land's Edge. Turns out that array does not work remotely the way I thought it did at the time. Changed it to get_total_available_lands(player) which is a little less accurate than I wanted there, but I don't think we need to do a whole for loop like in OGW for just an ai modifier.

Looks like that should take care of it, right?

[Korath]

You still want to stop at i < 50. The functions adding data there (just declare_mana_available_hex()) and the other functions reading it from there (check_mana_multi(), has_mana()) do - nothing ever (intentionally) writes to or reads from raw_mana_available_hex[0 or 1][50]. No change required in OGW; the bug there's entirely the result of the misdeclaration.

For legends, if you just want the total amount of generic mana player can muster, has_mana(player, COLOR_ANY, 1) will give it to you. It returns the amount available, not just nonzero.

[drool66]

Committed in 194d9e8

[Aswan jaguar]

...It also no longer encourages the AI to activate when activation is successful, since generic_activated_ability() never returns nonzero for EVENT_ACTIVATE; you want to check whether cancel == 1 afterward instead.

You mean in the case AI has the means to untap it, a Twiddle in hand or some other card that untaps others? because otherwise it is a tapped ability and can't be activated again until your next turn.

[drool66]

I think he's referring to this:

Code: Select all

   if( event == EVENT_ACTIVATE ){
       if( generic_activated_ability(player, card, event, GAA_UNTAPPED, MANACOST_X(4), 0, NULL, NULL)){
          ai_modifier += (count_subtype(player, TYPE_LAND, -1) * 8);
       }
   }

the second line will always return false since generic_activated_ability() never returns nonzero for EVENT_ACTIVATE; therefore the third line is never read. What can be nonzero in that case is "cancel". What I think he suggests is something like:

Code: Select all

   if( event == EVENT_ACTIVATE ){
       generic_activated_ability(player, card, event, GAA_UNTAPPED, MANACOST_X(4), 0, NULL, NULL);
       if( cancel != 1 ){
          ai_modifier += (count_subtype(player, TYPE_LAND, -1) * 8);
       }
   }