Fixing the Gitrog Monster

[jje4th]

Thanks for the feedback! I've been rethinking and testing the fix as well. Turns out it still may be more tricky.

Sinister concoction apparently should trigger The Gitrog Monster twice:
http://magicjudge.tumblr.com/post/14263 ... the-gitrog

That means we need to track the instance of each cost effect inside an SpellAbility rather than just the ability itself. Just creating the StackInstance earlier likely isn't enough. (Note that my fix also doesn't handle this situation correctly either).

I'll think about this more.

It'd be nice to get a definitive answer, since I got the opposite response from a different judge that also runs a "Ask judges questions" place. There's about 3 different scenarios I can think of that need clarification.

Sinister Concoction - Do two lands from different costs provide 2 triggers
Keldon Arsonist - Do two lands from the same/combined cost provide 2 triggers
Brutal Suppression + Lin Sivvi - Do two lands from two separate additional costs provide 2 triggers


As far as our implementation goes, if we assign the StackInstance, whether it's a Cost or Effect, and what type of cost or effect, we should be able to uniquely identify using those three aspects. (Resolved mana abilities should be the effect of their own SI, not whatever SA is being paid for)

[[I'm going to spawn this discussion off into a new thread, but... uhh.. Game of Thrones is on, and I have my priorities]]

Forking this off to it's own discussion as suggested.

The Gitrog Moster has the following in the SOI rules article:
"If multiple land cards are put into your graveyard at once, The Gitrog Monster’s last ability triggers only once. This could happen because an effect (such as that of Crawling Sensation’s first ability) put them there from your library at once, or because they were destroyed at the same time (such as two land creatures that were dealt lethal combat damage)."

So the primary question is whether the multiple land cards go to the graveyeard simultaneously.

Sinister Concoction - from the SOI rules article: "You may pay Sinister Concoction’s costs in any order. However, once you’ve decided to activate the ability and you’ve seen the top card of your library, you can’t change your mind." (a restatement of 601.2g). The fact you can chose which order to pay indicates that there are multiple individual events, one for each cost, on the card (as separated by commas). This looks pretty clear that costs aren't paid simultaneously and thus get a trigger for milling a land and and one for discarding a land.

For Keldon Arsonist - it's only one trigger, regardless of how many lands are discard. Sacrificing two lands is a single action and thus happens simultaneously.

Brutal Suppression x2 + Lin Sivvi - this should be two triggers because each sacrifice is an independent action. (e.g. the card would read "Sacrifice a land, sacrifice a land, 3: Put target Rebel card from your graveyard on the bottom of the library)

I like your suggestion of instantiating the StackInstance object before running the trigger (this is actually more in line with the rules too. "601.2 - to cast a spell is to take it from where it is (usually the hand), put it on the stack, and pay its costs, so that it will eventually resolve and have its effect"). Much simpler than my current implementation. Storing the cost/effect and type would need to be done either way. I'll try to figure out where to put this.

If nothing else - I've learned a lot about how Forge works

[Marek14]

BTW, as a bit similar card -- would Rakshasa Vizier be possible or easier with this?

[friarsol]

BTW, as a bit similar card -- would Rakshasa Vizier be possible or easier with this?

Probably easier since there aren't as many things that go from Grave -> Exile. And certainly there aren't as many costs that can fire it. Night Soil or Delve.

I like your suggestion of instantiating the StackInstance object before running the trigger (this is actually more in line with the rules too. "601.2 - to cast a spell is to take it from where it is (usually the hand), put it on the stack, and pay its costs, so that it will eventually resolve and have its effect"). Much simpler than my current implementation. Storing the cost/effect and type would need to be done either way. I'll try to figure out where to put this.

Well, we already put the spell in the stack zone when a spell is being cast. We just don't have a fully instantiated version of the spell ability there until it's completely paid for. Considering SIs can be spells or abilities, I don't think that's necessarily incorrect.

[jje4th]

Well, we already put the spell in the stack zone when a spell is being cast. We just don't have a fully instantiated version of the spell ability there until it's completely paid for. Considering SIs can be spells or abilities, I don't think that's necessarily incorrect.

The spell card is put into the stack zone, but it doesn't appear in the MagicStack until the spell's costs are paid. The issue here is that the GameAction.changeZone function (where the change zone tirgger is fired) doesn't have access to the triggering SpellAbility or SpellAbilityStackIntance directly. It attempts to get the top most SpellAbilityStackInstance from the game's MagicStack object (game.getStack().peek()). Since the costs have not been paid, the ability is not put on the stack.

There are two paths to addressing this part of the issue -
1) put payment of costs onto the stack
2) plumb the SpellAbility all the way through to the change zone triggers

In either case we need to track the cost payments as individual effects along with the spell ability instance. The major benefit of #1 is that SpellAbilityStackInstance provides a very good place to put that information. It also can work for basically any trigger without much additional work. The major downside is that it fundamentally changes the way the game stack works and there is a significant destabilization risk. It also requires special handling for costs on the stack because opponents should not be able to respond to cost payments. (I'm not as intimately familiar as the other devs, but it looks like there is a decent amount of interplay between CostPayment, SpellAbility, MagicStack, and SpellAbilityStackInstance such that a decent amount of refactoring would be needed).

I wonder if a better solution would be to track the current cost payment as game state. It would be available from the triggers and it wouldn't require messing with the stack. The cost payment would just need to be updated with an instance id (since it looks to always be instantiated at time of payment as opposed to the SpellAbility's Cost object) and expose the currently executing cost part as a pubic accessor.

[jje4th]

I decided to implement a separate stack for tracking active costs as it provides similar benefits to putting costs on the main stack, but with a MUCH smaller surface area (i.e. re-writing the payment system and core stack handling logic).

This is much better than my initial proposal and actually works for the different cases (though can't test Brutal Suppression as it isn't implemented in forge).

I'd still really appreciate a code review before I commit the changes given the scope.

[friarsol]

Yea I was a bit worried when you were saying using the main stack, using it's own stack should be fine though.

Can you give an example of - "each cost in a payment can trigger the effect; however, due to forge implementation multiple triggers may be created for a single cost (e.g. discarding two cards to pay Keldon Arsonist's ability cost)" ?

Seems pretty straightforward though, I'd just recommend waiting till Krazy rolls out 1.5.52 so we can iron things out before the following release

[jje4th]

Thanks for the feedback. I cleaned up the comments quite a bit and added examples. I'll keep an eye out for 1.5.52, then check in.

Regarding Rakshasa Vizier, that one is particularly hard to do right. This fix effectively "covers up" the fact that behind the scenes there are multiple triggers running for a single action and suppresses extraneous triggers. Rakshasa Vizier requires an accumulator to count the number of cards moved across zone, suppress all but the last trigger, and then put one effect on the stack to add counters for the number of cards moved. In the MTG rules, all of the cards moved from the graveyard to exile technically happen simultaneously then a single trigger fires, but this is not what Forge does. Supporting this correctly may require a rewrite of TriggerZoneChange and the associated ZoneChange game actions. (Note that doing so would also have fixed the issue with Gitrog Monster; however, it felt to risky to make large changes in crucial parts of Forge for just a few cards).

[friarsol]

Thanks for the feedback. I cleaned up the comments quite a bit and added examples. I'll keep an eye out for 1.5.52, then check in.

Regarding Rakshasa Vizier, that one is particularly hard to do right. This fix effectively "covers up" the fact that behind the scenes there are multiple triggers running for a single action and suppresses extraneous triggers. Rakshasa Vizier requires an accumulator to count the number of cards moved across zone, suppress all but the last trigger, and then put one effect on the stack to add counters for the number of cards moved. In the MTG rules, all of the cards moved from the graveyard to exile technically happen simultaneously then a single trigger fires, but this is not what Forge does. Supporting this correctly may require a rewrite of TriggerZoneChange and the associated ZoneChange game actions. (Note that doing so would also have fixed the issue with Gitrog Monster; however, it felt to risky to make large changes in crucial parts of Forge for just a few cards).

The main issue here is the difference between "OncePerEffect" triggers, and standard triggers.

I feel like it's easier with the patch you have. When triggers fire they are put onto a waitingTriggers list. Basically, whenever a trigger is about to be put in this list, we just check if it has a "OncePerEffect" param already exists with the same uniqueness. If it doesn't then we add it. But if it does we tabulate into that existing one (accumulating the cards involved in the effect at the same time the lone trigger is removed from ).

Speaking of which, we may need to have a CostPart for Delve so it can be added onto your new stack and be appropriately accounted for (since it lives in the pay mana cost area of the code). Reading the current wording of Delve, it definitely occurs separate from mana generation, so we only have to worry about the total delving.