[fixed] Bazaar of Baghdad

[joseluiselblanco]

Describe the Bug:
Not sure it's a bug but when I double click to automatically pay the cost of a spell, if I have a Baazar of Baghdad out, it taps the Baazar of Baghdad even though it doesn't produce any mana.

Which card did behave improperly?
Bazaar of Baghdad

Which update are you using? (date, name)Which type? (duel, gauntlet, sealed deck)
Latest version, duel

[Korath]

Yes, this is a bug.

You can't start flagging non-mana-sources as EA_MANA_SOURCE without A) telling the mana payment AI that they aren't really, and B) making their non-mana activated abilities unactivateable during mana payment. Otherwise - aside from the irritating autotapping behavior reported here - you can do illegal things like using Bazaar of Baghdad to discard a card you're in the middle of casting (and fail, but not get the other mana you spent on it back) or Diamond Valley to sacrifice a creature like Centaur's Herald while activating its ability with a sacrifice-self cost (and succeed), and you'll make the AI not consider casting spells or activating abilities because it doesn't like that it has to activate those non-mana sources too as a side effect.

The proper place for A is in can_autotap() at 0x42fda0, which you'll have to move out of magic.exe - a good idea anyway, since it still doesn't consider Wastes or snow-covered lands basic. Let me know if you need help; but don't try using the Shandalar version, which has different semantics.

B has to be done in every misflagged card's function, or, much better, something they all call.

[drool66]

Yep, I did this back before I knew the consequences of anything.
B is no trouble.
Does this look right?

Code: Select all

int can_autotap(int player, int card, int autotap_flags)
{
   // 0x42fda0
  card_instance_t* inst = in play(player, card);
  if( !inst )
   return 0;
  iid_t iid = inst->internal_card_id;
  color_test_t colors = (inst->card_color & (COLOR_TEST_ANY | COLOR_TEST_ARTIFACT));
  if( is_what(player, card, TYPE_LAND) )
   colors |= mana_colors_added[player];
  if ( colors <= COLOR_TEST_0
    || inst->state & STATE_TAPPED
    || !(cards_data[iid].extra_ability & EA_MANA_SOURCE || (is_what(player, card, TYPE_LAND) && mana_colors_added[player] > COLOR_TEST_0))
    || cards_data[iid].extra_ability & EA_PAID_MANA_SOURCE )   //huh, really?
    return 0;
  if ( (autotap_flags & AUTOTAP_NO_BASIC_LANDS) && is_basic_land(player, card) )
    return 0;
  if ( (autotap_flags & AUTOTAP_NO_NONBASIC_LANDS) && is_what(player, card, TYPE_LAND) && !is_basic_land(player, card) )
    return 0;
  if ( (autotap_flags & AUTOTAP_NO_DONT_AUTO_TAP) && (inst->state & STATE_NO_AUTO_TAPPING) )
    return 0;
  if ( (autotap_flags & AUTOTAP_NO_ARTIFACTS) && is_what(player, card, TYPE_ARTIFACT) )
    return 0;
  if ( (autotap_flags & AUTOTAP_NO_CREATURES) && is_what(player, card, TYPE_CREATURE) )
    return 0;
  return 1;
}

[Korath]

Code: Select all

  if ( colors <= COLOR_TEST_0

It doesn't make sense to compare bitfields with < <= > >=. Use one of !colors, colors != COLOR_TEST_0, or !(colors & COLOR_TEST_ANY).

Code: Select all

    || inst->state & STATE_TAPPED

This'll prevent activated abilities of tapped cards that don't require tapping to be used when a human double-clicks a card, and to be used at all by the AI. "can_autotap()" isn't quite precisely named in that regard.

I very vaguely recall this being problematic at one point. We may well have patched it out before we started keeping track of the changes we made directly to magic.exe. There's no such check in the current version nor has been for years, just in case the decompilation you're reading has it.

Code: Select all

    || !(cards_data[iid].extra_ability & EA_MANA_SOURCE || (is_what(player, card, TYPE_LAND) && mana_colors_added[player] > COLOR_TEST_0))

It's hard to imagine a card that would pass the right half of the logical-or without passing the left, unless you plan on combining this with data changes.

Code: Select all

    || cards_data[iid].extra_ability & EA_PAID_MANA_SOURCE )   //huh, really?

This is actually the only place EA_PAID_MANA_SOURCE is used - it lets a card be flagged EA_MANA_SOURCE (so it displays with colored stripes) but prevents it from being auto-activated for mana. Celestial Prism and so on just don't ever get autotapped; it only gets activated with the usual spontaneous activation AI, i.e. the computer mostly only uses its mana by accident.

Code: Select all

  if ( (autotap_flags & AUTOTAP_NO_NONBASIC_LANDS) && is_what(player, card, TYPE_LAND) && !is_basic_land(player, card) )

This prevents Gem Bazaar and Mishra's Workshop from being preferentially activated, compared to the exe version. (Which is ok if intentional.) The idea behind AUTOTAP_NO_NONBASIC_LANDS isn't so much "activate basic lands first" as "activate sources that have no other conceivable purpose" first. Non-lands were probably only excluded from the check because of Winter Orb, and there just weren't any other lands without any activated abilities besides producing a single color of mana.

[drool66]

Code: Select all

    || !(cards_data[iid].extra_ability & EA_MANA_SOURCE || (is_what(player, card, TYPE_LAND) && mana_colors_added[player] > COLOR_TEST_0))

It's hard to imagine a card that would pass the right half of the logical-or without passing the left, unless you plan on combining this with data changes.

Yes - the idea is to remove EA_MANA_SOURCE from lands with no mana abils that I added in 50611a9eb ("Lands with added mana production can be tapped directly, +Riftstone Portal", 2020-08-24), and also add this same kind of check in count_mana(). The point is to allow them to be autotapped and declare their mana available with an Urborg, Tomb of Yawgmoth et al. in play. I'm not sure if that will gum up anything else though.

Code: Select all

  if ( (autotap_flags & AUTOTAP_NO_NONBASIC_LANDS) && is_what(player, card, TYPE_LAND) && !is_basic_land(player, card) )

This prevents Gem Bazaar and Mishra's Workshop from being preferentially activated, compared to the exe version. (Which is ok if intentional.)

So more like the exe does?

Code: Select all

  if ( (autotap_flags & AUTOTAP_NO_BASIC_LANDS) && (is_basic_land(player, card) || id == CARD_ID_GEM_BAZAAR || inst->mana_color == COLOR_TEST_ARTIFACT) )
   return 0;
etc...

Any better way to do that other than those two highly specific checks, maybe like "!(cards_data[iid].extra_ability & EA_ACTIVATE)" or something?

[Korath]

Yes - the idea is to remove EA_MANA_SOURCE from lands with no mana abils that I added in 50611a9eb ("Lands with added mana production can be tapped directly, +Riftstone Portal", 2020-08-24), and also add this same kind of check in count_mana(). The point is to allow them to be autotapped and declare their mana available with an Urborg, Tomb of Yawgmoth et al. in play. I'm not sure if that will gum up anything else though.

I'd be very wary. Very many things check for EA_MANA_SOURCE for very many reasons. Manastripe display. Drain Power, and Power Sink, and lots of cards like Crackdown Construct that misuse it as a proxy for whether the particular ability was a mana ability. Whether they can be activated while mana is being charged, and whether activation can be responded to. AI valuation. They're all a problem going in the other direction, too, but generally a more mitigable one - Strip Mine's always been there, for example, so there's almost always an out after the EA_MANA_SOURCE check to exclude a card.

Any better way to do that other than those two highly specific checks, maybe like "!(cards_data[iid].extra_ability & EA_ACTIVATE)" or something?

Well, you've already seen what trying to special-case everything does to mana cost modification.

EA_ACT_ABILITY won't work either. Consider Lake of the Dead and Ruins of Trokair: all abilities are mana abilities, so they should be EA_MANA_SOURCE and not EA_ACT_ABILITY; but you want to delay activating them if you can, so you can see whether you need to use their more expensive ability. Ghost Town is an example in the other direction - it does have an activated, non-mana ability, but tapping it for mana doesn't make any difference in whether you can bounce it. And cards like Exotic Orchard or Meteor Crater are reasonably likely to have more options the longer you put off activating them.

The usual solution to this kind of problem is to ask the object what to do, and if you look at Shandalar's version of can_autotap(), you'll see that's what I settled on. It's ok that there are surely mana sources that don't respond to QUERY_IS_PLAIN_MANA_PRODUCER; it just means the AI doesn't perform as well as it could, and as long as the five original basics and Mishra's Workshop (and Gem Bazaar, I suppose, if you're completist enough) do respond, you're not any worse off than you were before.

[drool66]

I believe this is fixed in adf2081 & 286dc33