Fix alternative costs from static abilities

[tchataway]

Currently, when a static ability grants an alternative cost to spells (for example, Hunting Velociraptor giving Dinosaur spells "prowl", and Henzie "Toolbox" Torre giving mana value 4 and greater creature spells "blitz"), the alternative costs only apply when casting from the Hand. In the case of Toolbox, the mana value is also ignored, allowing any creature spell to be cast from the hand with blitz.

This change allows alternative costs granted from static abilities to function properly. The main change in behaviour here is that the alternative cost can be paid whenever it is supposed to be able to be paid. E.g. if you can cast creature spells from the top of your library, you can now cast them using alternative costs granted by static abilities. Basically, if you could cast it normally, and there aren't other restrictions for the alternative cost (e.g. "flashback" and "escape" only casting from the graveyard), then you are able to cast it using that cost (relevant rules here being 601.2, 601.2b, and 601.2f-h).

Overview

• Move "alternative costs from static ability" logic from inside getAlternativeCosts to new function getAlternativeCostsGrantedByStaticAbilities function which is called before alternative costs are retrieved for card
• Update "alternative costs from static ability" logic to check all alternative cost keywords, not just blitz
• Move cmcGE4 restriction from Toolbox's static ability's AddKeyword parameter to its Affected parameter
• Change Hunting Velociraptor's static ability's AffectedZone parameter from Hand to Stack

Testing

• No unit tests (I can add some if necessary)
• Local testing of both Toolbox and Hunting Velociraptor, ensuring
  • Restrictions are applied
  • Cost reductions are applied

[tchataway]

This logic was moved, mostly unchanged, into getAlternativeCostsGrantedByStaticAbilities.

[tchataway]

The previous logic checked if the source is a creature, but I removed that here, because the restriction is already checked when applying the static ability in checkStaticAbilities, below.

This will allow static abilities that grant alternative costs to non-creature spells to function, also.

[tchataway]

I admit I don't really understand the concept of Last Known Information, so I've tried to leave it untouched, but I noticed that some of this logic was being run before the static ability checks were happening in the old location, so I included it here in case it's necessary.

There's also corresponding code that is executed afterward, which book-ends the logic.

[tchataway]

This line is new. Without it, the alternative cost isn't copied for additional CardPlayOptions, which means it can only be paid when casting from the Hand.

[tchataway]

Some places I've worked frown upon using exceptions this way.

If it violates convention here, I'll write a new function to verify the keyword represents an alternative cost rather than calling valueOf on the AlternativeCost enum.

[tchataway]

The previous implementation was so close to being correct!

[Hanmac]

This Check is for MDFC

is the affected checked again when casting the Backside?

[tchataway]

I'll look into this later today (it's currently 3am local time for me).

[Hanmac]

For Henzie, it also needs to check the Spell Mana Value because for X Creature Spells

so you can only Blitz them when X is high enough

[tchataway]

Yes, that wasn't happening at all before. The exclusive restriction wasn't defined in the right place. I moved it to the Affected param, and now it actually does check it (i.e. Affected Creature.YouCtrl+cmcGE4).

Also, the wording of Henzie is mana value, not mana cost, so it will always consider X to be zero when checking this restriction.

[tchataway]

Unfortunately you picked a pretty complex area as your first project :/

Just my luck! 😆

We haven't been happy with the MayPlay / alternative cost handling for quite some time, but it has also proven rather difficult to fix smaller aspects of it without breaking something else at the same time...(so it has been left alone mostly for a bigger refactor)

Right, fair enough. It is turning out to be much hairier than anticipated. Would it be better to just shelve this in lieu of a more comprehensive refactor later, then?

For your X interaction it's this:

202.3e. When calculating the mana value of an object with an {X} in its mana cost, X is treated as 0 while the object is not on the stack, and X is treated as the number chosen for it while the object is on the stack.

Thank you! I was very confused but this makes sense. I'm obviously very new to the codebase so I'm not clear on how Forge lines things up relative to the "proposal to cast a spell". When the user clicks a card in their hand, for example, Forge appears to get all costs and alternative costs at that point, and present them as a list for the user. For X cost creatures in Toolbox's case, should it present the Blitz option the way it normally would, but with X in the cost, then prompt for X, then check the total mana value of the spell on the stack and either cancel/prompt for mana?

[Hanmac]

for human it goes into this line:

forge/forge-gui/src/main/java/forge/player/HumanPlaySpellAbility.java

Lines 160 to 166 in 3fa89af

	// because of Selective Snare do announceType first
	final boolean prerequisitesMet = announceType()
	&& announceValuesLikeX()
	&& ability.checkRestrictions(human)
	&& (!mayChooseTargets || ability.setupTargets()) // if you can choose targets, then do choose them.
	&& ability.canCastTiming(human)
	&& ability.isLegalAfterStack()

where it checks this:

forge/forge-game/src/main/java/forge/game/spellability/SpellAbility.java

Lines 2574 to 2579 in 3fa89af

	public boolean isLegalAfterStack() {
	if (!matchesValidParam("ValidAfterStack", this)) {
	return false;
	}
	return true;
	}

which is set by Blitz:Cost:* for this:

forge/forge-game/src/main/java/forge/game/card/CardFactoryUtil.java

Lines 2712 to 2720 in 3fa89af

	} else if (keyword.startsWith("Blitz")) {
	final String[] k = keyword.split(":");
	final Cost blitzCost = new Cost(k[1], false);
	final SpellAbility newSA = card.getFirstSpellAbility().copyWithManaCostReplaced(host.getController(), blitzCost);
	if (k.length > 2) {
	newSA.getMapParams().put("ValidAfterStack", k[2]);
	}

[tool4ever]

Right, fair enough. It is turning out to be much hairier than anticipated. Would it be better to just shelve this in lieu of a more comprehensive refactor later, then?

It's your choice I guess. I'm not saying you can't try to tackle this and splitting it across consecutive PR is also sometimes easier. Just don't want you getting frustrated by this too quickly ;)

You noticed correctly that spell casting is one of the parts where we're not super accurate in the order according to the CR (sometimes it's also different for AI vs Human PlayerController). I suppose we could have less trouble in that regard if we would basically allow any card to be moved to the stack first as outlined by 601.2a. But then again this might end up too confusing GUI wise, which is one of the reasons we're doing some of these choices in sort of a "look-ahead" fashion. Another one would be this:

601.3a. If an effect prohibits a player from casting a spell with certain qualities, that player may consider any choices to be made during that spell's proposal that may cause those qualities to change. If any such choices could cause that effect to no longer prohibit that player from casting that spell, the player may begin to cast the spell, ignoring the effect.

So yea, that gets messy pretty quickly... 🤹‍♂️

[tchataway]

Ah, geez. It all makes sense now. That clears everything up, thanks @Hanmac.

I saw the comment in getAlternativeCosts that said // TODO with mana value 4 or greater has blitz. and that, coupled with the fact that the Blitz cost was appearing on the list of casting options when clicking a card, led me to believe that the behaviour hadn't been implemented yet. If I'd tried to actually click the Blitz option, I would have seen that it didn't work for mana values < 4.

It didn't help that I didn't know about rule 202.3e, either (cheers, @tool4ever!).

Thanks for your patience, @Hanmac. I've been very obtuse.

So, to summarise:

• On master, static abilities that grant alternative costs work for normal cards and both faces of MDFCs in the hand, but not when able to cast from the top of the library or the command zone, etc.
• On the fork, static abilities that grant alternative costs work for normal cards and the front face of MDFCs for all zones they could normally be cast from, but not the back face of MDFCs, and it also breaks Toolbox for X costs (sorry about that 😩 )

I thought that moving the static ability checking code back to where it came from and duplicating the loop over source.mayPlay(activator) would do the trick for the back face of MDFCs, but it doesn't seem to have worked. I'd prefer to find a way to enable the extra CardPlayOptions that minimises disruption, but I'll need to do some more investigation.

Thanks again, both of you, for taking the time to educate me on the codebase!

[tchataway]

It's your choice I guess. I'm not saying you can't try to tackle this and splitting it across consecutive PR is also sometimes easier. Just don't want you getting frustrated by this too quickly ;)

Oh I'm happy to keep thrashing away at this. I've been a software engineer for over a decade now so I'm no stranger to frustration 😆

Thanks for the warm welcome. I'm looking forward to learning more about how Forge does things. I might take a look at some of the beginner friendly issues to get a bit more orientation, as well.

So yea, that gets messy pretty quickly... 🤹‍♂️

Yes, absolutely. Those two rules, if implemented faithfully, would turn Forge into a Christopher Nolan film. 😬

[Hanmac]

with your change you can't Prowl/Blitz a Modular DFC (see below)

that's why called by GameActionUtil.getAlternativeCosts is better

[Hanmac]

@tool4ever remember me to recreate the Issue about May Play again

in short summary, we need to split the MayPlay logic between multiple effects that may or may not be combined:

• Effects that alter the Cost like Rooftop Storm, that should be use a Static instead of a Keyword like Alternative Cost:0
• Effects that allow you to cast/play stuff from zones but without setting a cost, like Augur of Autumn
• Effects that combine both, but for these, the other effects can't be used too like Bolas's Citadel
• The Goal also was to reduce the use of Continuous for LKI stuff

i think my first attempt to split was 3+ years ago:
https://github.com/Card-Forge/forge/compare/mayPlayAlternativeCost

[Hanmac]

i think in the current state,
the best thing would be if we are doing it in a three step process:

• get Base Abilities, (including Modal)
• get possible Stack Keyword from them (this needs to apply possible Modal States for LKI)
• and then apply MayPlay stuff (this should notice that it can't apply MayPlay WithoutCost stuff on Stack Keywords)

[Hanmac]

@tool4ever to fix the problems addressed there, we probably still need to refactor MayPlay stuff

[github-actions]

This PR has not been updated in a while nad has been marked on stale. Stale PRs will be auto closed

[tool4ever]

Gonna close this for now since the logic got shuffled around and improved a decent amount since this started
But feel free to revisit this if your time allows