Trigger discussion (was WheneverKeyword reference)

[friarsol]

Right, that complicates things a bit. Not much, just a bit I'll just commit this conversion batch first, then dig in.

Yea, I don't think the type of thing that would cause issues to this is even available right now. Like Diviner's Wand.

[Hellfish]

Yeah; for now, all triggers on facedown cards are ignored.I'mma focus on the WheneverKeyword obsoletion atm.

[friarsol]

So for Triggers that care about the card that triggered them (which it looks like a bunch of the ones that are Wheneverkeyword left are) maybe we can add something to SpellAbility for this. So in SpellAbility we'd have a Card triggeringCard, which most of the time is null.

For things like Orchard Warden would have a SetTriggering parameter. Which would get set after the AF is generated. Then the GainLife ability could use a Defined$ Triggering which could be added into AbilityFactory.getDefinedCards()

[Hellfish]

For Orchard Warden and the like, that's after a characteristic of a card,would we expand Count$ or what do you think? Because that's not really the domain of Defined, is it?

[friarsol]

For Orchard Warden and the like, that's after a characteristic of a card,would we expand Count$ or what do you think? Because that's not really the domain of Defined, is it?

Oh, I guess I grabbed the wrong card name. I meant things like Sigil of the New Dawn to use Defined since it's moving that card.

However, for Orchard Warden it would need to use AF.calculateAmount() like how it's used for Sacrificed or Targeted. Should be a minimal code in AF plus what's needed to record the triggering card.

[friarsol]

Oh and I was looking through the last WheneverKeyword cards and I think these few can be done.

Code: Select all

Akuta, Born of Ash
Brackwater Elemental
Death of a Thousand Stings
Fists of Ironwood
Harbor Guardian


[Hellfish]

Whoops, should've looked through them again. Pretty sure they were not implementable at the time.I'm testing a "Triggering" addition to AF.calculateAmount and AF.getDefinedCards with Orchard Warden and Sigil of the New Dawn right now, will commit for scrutiny in a bit.

Edit: Commited. Sorry it took a while, dinner intervened

[slapshot5]

I'm trying to add Skyfire Kirin like this:

Code: Select all

Name:Skyfire Kirin
ManaCost:2 R R
Types:Legendary Creature Kirin Spirit
Text:no text
PT:3/3
K:Flying
T:Mode$ SpellCast | ValidCard$ Spirit.YouCtrl,Arcane.YouCtrl | TriggerZones$ Battlefield | Optional$ True | Execute$ TrigGainControl | TriggerDescription$ Whenever you cast a Spirit or Arcane spell, you may gain control of target creature with that spell's converted mana cost until end of turn.
SVar:TrigGainControl:AB$GainControl | Cost$ 0 | ValidTgts$ Creature.cmcEQX | TgtPrompt$ Select target creature with converted mana cost equal to X | LoseControl$ EOT
SVar:X:Triggered$CardManaCost
SVar:Rarity:Rare
SVar:Picture:http://www.wizards.com/global/images/magic/general/skyfire_kirin.jpg
End

but, the targeting isn't right. When I cast Waking Nightmare (cmc = 3), it is looking for a target with CMC = 4. (no coincidence that Skyfire Kirin has CMC = 4, undoubtedly).

Am I doing it wrong, or is something unsupported here?

-slapshot5

[friarsol]

Hellfish, do we have a way to tell if an Ability on the stack is a triggered ability? I was trying to fix the ordering of Triggers, but I can't really do it without knowing if the Ability came from a Trigger. I didn't notice it when I was poking around, but maybe its hidden somewhere that I didn't see.

[Hellfish]

Slapshot:
Hoo boy, it's gonna be a long night. I have a sneaking suspicion this is related to the bugs I'm seeing with Myrsmith and the SOM spellbombs(EDIT2: No they're not.. crap. :/)... I'll just say I'm looking into it, for now.

Sol:
Right now, the abilities from triggers are just plain old abilities.Simply adding a boolean with accessor functions to SpellAbility should be enough?

EDIT:The triggers are already in APNAP order, if that's what you meant.

[Hellfish]

Just collecting my thoughts a bit here temporarily, as I might have to go soon.
Skyfire Kirin: I'm pretty sure the problem is that the ValidTgts parameter isn't interpreted by AbilityFactory.calculateAmount (which has access to the spellability and therefore the triggering card) but by CardFactoryUtil.xCount (which only has access to the card ability's sourcecard) because it's interpreted only when you're in the abilities input state and try to target a creature?

Myrsmith,Panic Spellbomb,Origin Spellbomb,Flight Spellbomb,Horizon Spellbomb: It almost seems like all mana abilities are ignored when I enter the mana paying input state.

[friarsol]

Sol:
Right now, the abilities from triggers are just plain old abilities.Simply adding a boolean with accessor functions to SpellAbility should be enough?

EDIT:The triggers are already in APNAP order, if that's what you meant.

Nope. I meant the trigger should hit the stack after the SA that caused it to trigger. I know exactly where this needs to occur, i just need to know if the ability is from a Trigger. I can use a boolean for now. Eventually I would like to takeover the Ability_Triggered class for the new Triggers. We would have to convert all of those before the takeover can happen.

At some point we should probably give the Human a choice about what order simultaneous triggers go on the stack. (Since that's important for things like Smokestack and Tangle Wire)

[Hellfish]

Quick thought for the Spellbomb deal: All of them have in common that the board state changes. In the spellbomb's case they change zone (and are therefore copied?) which could invalidate them and break GameAction.playSpellAbility() ?

This isn't true for Myrsmith though... Which is apparently a non-issue since it does work now, wow.

Sol:
Okay, I see what you mean now (I thought that was already the case though. Eh.). I chose to work with regular abilities because that's what AF's were doing already and it seemed fine.I agree about simultaneous trigger ordering but, IMHO, "some point" = "a bit later point".

I'm pondering moving triggeringCard info over to the source card of the trigger to make the info more widely accessible. Are there many more cases like Skyfire Kirin?

[Sloth]

Another issue that has to be fixed is that some targeted triggered abilities can be canceled even though they are not optional.
Example: If you cast Vithian Renegades and the AI has no artifacts, you would have to chose one of yours, but currently you can just cancel the ability.

The enters-the-battlefield ability is mandatory. If you're the only player who controls any artifacts, you must target one of them.

[friarsol]

Quick thought for the Spellbomb deal: All of them have in common that the board state changes. In the spellbomb's case they change zone (and are therefore copied?) which could invalidate them and break GameAction.playSpellAbility() ?

Sol:
Okay, I see what you mean now (I thought that was already the case though. Eh.). I chose to work with regular abilities because that's what AF's were doing already and it seemed fine.I agree about simultaneous trigger ordering but, IMHO, "some point" = "a bit later point".

Yea the Spellbombs are getting copied as they change zone. I've been trying to write some LastKnownInfo code before things change zones. Then we could use the last known info for triggers. That just means we need to copy over anything that is important.

Since you know what of the triggered stuff is important you can copy any of that over into the function I started writing in AllZone.GameAction.getLastKnownInformation() which you can turn on by calling it in the above moveTo function by changing:

Code: Select all

Card lastKnownInfo = c;


Code: Select all

Card lastKnownInfo = getLastKnownInformation(c);


I'm not worried about choosing the ordering of triggers now. I just wanted to make sure you knew it still missed a key piece.