[fixed]Blood Sun doesn't remove non-activated abilities

[Korath]

Describe the Bug:
Blood Sun only prevents non-mana activated abilities:

• City of Brass damages you when it's tapped.
• Teferi's Isle phases.
• Reliquary Tower makes your hand size infinite.
• The Tabernacle at Pendrell Vale imposes upkeep costs.
• Glacial Chasm prevents damage, has cumulative upkeep, and keeps creatures from attacking.
• Azorius Chancery bounces lands.
• Everything I tried that enters the battlefield tapped enters the battlefield tapped.
• Darksteel Citadel, and land enchanted by Consecrate Land, is indestructible.
• Mutavault can't activate to animate itself. ...But it can't activate to add mana, either, unless you're currently being charged mana.
• Celestial Colonnade also can't activate to animate itself, but can tap for mana normally. If already self-animated before Blood Sun enters the battlefield, it has flying and vigilance.

Which card behaved improperly?
Blood Sun.

Which update are you using? (date, name)Which type? (duel, gauntlet, sealed deck)
1804c2644 (master) Ignore manalinkex in "make world" or "make install-world"

What exactly should be the correct behavior/interaction?
"All lands lose all abilities except mana abilities."

Are any other cards possibly affected by this bug?
Not unless you count Mutavault not being able to tap for mana as a separate bug - Blood Sun is the only card that removes all abilities except one specific sort of ability that it's not itself granting.

[drool66]

Ugh, yep, I only thought of activated abilities.
My initial thought is to simply add a check for this player_bits at each of these points. I could do this at the same time I go thorough each nonbasic for the get_global_mana_color_addition() fixes. Do you have any further guidance on this one?
I should be able to mod again sometime next week. These will be the first order of business for me.

[Korath]

I don't have any good news, no. I haven't come up with a way to do it in Shandalar that doesn't require cooperation from every land in the game. In particular, there's no basis for excluding basic lands as you imply - even Swamps can be enchanted by Caribou Range or Farmstead, or animated by Kormus Bell and then flown by Levitation or get a zillion and one other abilities, keyworded or not. Blood Sun removes them all. Or all the ones with earlier timestamps, anyway.

With that much effort expended, you really don't want to have WOTC print a card with a new variant in the very next set after you're done, that you can't support without going through all that effort again. This is what happened with Panharmonicon and Naban, Dean of Iteration (and then eventually Yarok, the Desecrated and Teysa Karlov and Sanctum of All). So, at minimum, I'd make it settable per-object, not just one bit for "all of this player's permanents lose all non-mana abilities".

The model for this sort of effect in Manalink is is_humiliated(). While I've got... all kinds of issues with that, it's easier to adapt to this than the method Shandalar uses for loss of abilities. Right now it only returns 1 (lost all abilities) or 0 (hasn't lost all abilities). I'd suggest making it return a bitfield instead: 1 for "lost all abilities", 2 for "lost all abilities except mana abilities", eventually 4 for "lost all non-loyalty abilities" or whatever. (You'll probably want to add a note that says "isn't necessarily correct for things that aren't lands", though it may be worth your time to doublecheck everything that's flagged mana source.) You'll find that most of what you need has already been done, and you just have to go through and re-enable mana abilities if is_humiliated returns non-zero but bit 2 isn't set.

More extensible, but a bit more work now, is to say 1 is "lost all nonmana abilities" and 2 is "lost all mana abilities". (When WOTC prints a "Enchanted planeswalker loses all nonloyalty abilities", make bit 4 mean "lost all loyalty abilities", update 1 to mean "lost all nonmana, nonloyalty abilities", and implement that card as returning 1|2, and so on.) Then you'd check in client code for specific bits for each ability, not just is_humiliated(). (I'd rename the backend function to something like "lost_abilities()" then, and redefine is_humiliated(args) to just be "return (lost_abilities(args) & 1);" - you definitely don't want to have to go through every card in the game that calls is_humiliated() and change every one.) This would immediately let you also implement "enchanted land loses all mana abilities", for example, and once the lose-all-loyalty-abilities or whatever effect comes in, let you easily add all of "enchanted object loses all non-loyalty abilities", "enchanted object loses all non-loyalty, non-mana abilities", "enchanted object loses all mana and loyalty abilities", etc. Having each bit mean "all abilities except this-one-kind" doesn't have the same sort of flexibility.

[drool66]

So, at minimum, I'd make it settable per-object, not just one bit for "all of this player's permanents lose all non-mana abilities".

So I've managed to make a system that honors is_humiliated() for static & triggered abilities using

if( is_humiliated(args) & 2 )
or 4, or 8, or so on for other types of lost abilities from reclaimed player_bits

(suddenly starting to think I should have made static & triggered abilities separate bits...)

I've applied it to all nonbasics and it is working very well. Loss of activated abilities was working already with the existing machinery.

But it is still set in player_bits. I'm looking for space to store loss of static & triggered abilities per instance. targets[17].playercard is full with loss of non-mana activated/all activated/all abilities. I am definitely willing to map unused space in card_instance_t, but is that the best route? I'm hesitant to do that without a roadmap for how we're going to reclaim that storage.

Thing is, targets[17] uses four bits per ability loss so that each can be incremented up to 16 times; but since every card sets it using bitwise OR, can't we just use one bit for each ability loss? That gives us 12 (right?) instead of 3, which should be plenty.

Thank you again for your help.

is_humiliated() currently looks like this, by the way

Code: Select all

int is_humiliated(int player, int card)
{
  if (player == -1 || card == -1)
   return 0;

  int rv = 0;

  if( is_what(player, card, TYPE_LAND) && (player_bits[player] & PB_NONMANA_ABILITIES_OF_LANDS_DISABLED) )
   rv |= 2;

  if( is_humiliated_by_instance(get_card_instance(player, card)) )
   rv |= 1;

   return rv;
}

[Korath]

...I'd forgotten just how awful this subsystem is. The "all kinds of issues" I mentioned above mostly had to do with how every single card needs to individually accommodate loss of abilities, rather than the engine doing it.

Long-term, you'll want to be able to add arbitrary amounts of per-instance data, like with Shandalar's card_aux_t. This is harder than it looks, and if it doesn't already look hard, you're not ready to do it yet.

Medium-term, loss of abilities (before the complications added for this) shouldn't need more than one bit. It definitely shouldn't have to do anything when it's removed, since it can't be reliably caught - Cytoshape a creature that says "Other creatures lose all abilities", for example, and the count's unavoidably off for at least that turn, and forever if it it dies while it's copying something else. Just clear and set the bit sometime during recalculate_all_cards_in_play(), probably in get_abilities(..EVENT_ABILITIES).

In the short term, what you want to avoid is having to update all the users of this subsystem if the back end changes. So doing this with a player bit for now isn't so bad, so long as nothing checks for it except through is_humiliated(). I would worry that is_humiliated_by_instance() doesn't see it, though. I suggest you check for it in there instead of is_humiliated(). See find_player() in shandalar.h for how to extract a player given a card_instance_t; you can find Manalink's equivalent of the constants in it by taking the address of get_card_instance(0, 0), get_displayed_card_instance(1, 150), and so on, and since those are stored in Magic.exe, they'll be consistent across builds.

[drool66]

Fixed in 603bf75