[WIP] Allows player to choose which asThough ability to use

[Alex-Vasile]

Previously the player would be prompted for which permitting object to use only if all of the approving objects were consumable. Now, they are prompted whenever there is more than one object (consumable or not). Also fixed Risen Executioner to only add its cost increase when its cast using its ability.

Closes #2087. Closes #8584.

TODO

• [ ] See which asThough's should give the player the option to choose, and which have rules for automatically picking them

[awjackson]

We already store the ApprovingObject that was used to cast a spell inside the CAST_SPELL and SPELL_CAST events, and it can be retrieved from those via a rather convoluted process (search the codebase for getAdditionalReference). If you're going to introduce a new way to retrieve this information, I propose finding some way to store the ApprovingObject directly inside the Spell object rather than resorting to setValue/getValue. It would also be ideal if the method works with all the cards that are currently using getAdditionalReference so that that convoluted subsystem can be replaced completely.

[Alex-Vasile]

We already store the ApprovingObject that was used to cast a spell inside the CAST_SPELL and SPELL_CAST events, and it can be retrieved from those via a rather convoluted process (search the codebase for getAdditionalReference).

My current saving into the state is not for the chosen ApprovingObject, but the set of all approving object for an ability. The set of all possible ApprovingObjects is to find all of them on the first pass when game.inCheckPlayableState() == true, and then load that set back up when the game is running for real and have the player chose.

If you're going to introduce a new way to retrieve this information, I propose finding some way to store the ApprovingObject directly inside the Spell object rather than resorting to setValue/getValue.

The setValue/getValue usage that I added is meant to be used only inside asThough() (and whoever else I am able to clean it up from.

It would also be ideal if the method works with all the cards that are currently using getAdditionalReference so that that convoluted subsystem can be replaced completely.

Yeah I haven't heard of that before

[awjackson]

My current saving into the state is not for the chosen ApprovingObject, but the set of all approving object for an ability.

The set of all possible ApprovingObjects is to find all of them on the first pass when game.inCheckPlayableState() == true, and then load that set back up when the game is running for real and have the player chose.

Is there any particular pressing reason to do this rather than just invoking all the potential approvers again? It seems unsafe to me. Some classes that implement AsThoughEffect may apply different logic depending on whether you are checking playables or actually using the effect, so caching may produce incorrect results.

Yeah I haven't heard of that before

All right, please disregard it as it seems to be completely unrelated to what you are doing. My mistake.

[Alex-Vasile]

My current saving into the state is not for the chosen ApprovingObject, but the set of all approving object for an ability. The set of all possible ApprovingObjects is to find all of them on the first pass when game.inCheckPlayableState() == true, and then load that set back up when the game is running for real and have the player chose.

Is there any particular pressing reason to do this rather than just invoking all the potential approvers again? It seems unsafe to me. Some classes that implement AsThoughEffect may apply different logic depending on whether you are checking playables or actually using the effect, so caching may produce incorrect results.

Honestly, I am not entirely sure. It's been two months since I last looked at this and I apparently did not leave good enough documentation.

IIRC, I believe the issue I was that if I try to find the AsThroughEffects outside of game.inCheckPlayableState() == true, many of them will change the game state when their .applies() is called.

E.g. If the two options are [[Bolas's Citadel]] and [[Vivien, Monsters' Advocate]], Bolas's .applies() will execute the following code: https://github.com/magefree/mage/blob/572104b8fcdb50b509c34b10e113cf39602374bb/Mage.Sets/src/mage/cards/b/BolassCitadel.java#L116-L122

This will cause the player to still lose life even if Vivien was chosen as the Approving object. Since the .applies() has to be called to even generate the list.

Yeah I haven't heard of that before

All right, please disregard it as it seems to be completely unrelated to what you are doing. My mistake.

My bad, that was actually not true, I looked into this some more, I have actually implemented a card that needed this (Share the Spoils). But I am still not sure if the improvements to that are relevant to this PR, here I want the players to have the option to choose which from several approving objects.

[github-actions[bot]]

Bolas's Citadel - (Gatherer) (Scryfall) (EDHREC)

{3}{B}{B}{B} Legendary Artifact You may look at the top card of your library any time. You may play lands and cast spells from the top of your library. If you cast a spell this way, pay life equal to its mana value rather than pay its mana cost. {T}, Sacrifice ten nonland permanents: Each opponent loses 10 life.

Vivien, Monsters' Advocate - (Gatherer) (Scryfall) (EDHREC)

{3}{G}{G} Legendary Planeswalker — Vivien 3 You may look at the top card of your library any time. You may cast creature spells from the top of your library. +1: Create a 3/3 green Beast creature token. Put your choice of a vigilance counter, a reach counter, or a trample counter on it. −2: When you cast your next creature spell this turn, search your library for a creature card with lesser mana value, put it onto the battlefield, then shuffle.

[awjackson]

IIRC, I believe the issue I was that if I try to find the AsThroughEffects outside of game.inCheckPlayableState() == true, many of them will change the game state when their .applies() is called.

E.g. If the two options are [[Bolas's Citadel]] and [[Vivien, Monsters' Advocate]], Bolas's .applies() will execute the following code:

https://github.com/magefree/mage/blob/572104b8fcdb50b509c34b10e113cf39602374bb/Mage.Sets/src/mage/cards/b/BolassCitadel.java#L116-L122

This will cause the player to still lose life even if Vivien was chosen as the Approving object. Since the .applies() has to be called to even generate the list.

Yeah, yeesh, I should have known because when I was investigating CAST_AS_INSTANT I came across OfferingAbility which has exactly the same issue. I think AsThoughEffect might have to be redesigned so that checking whether the effect could be used and actually using the effect are two different methods.

[Alex-Vasile]

Yeah, yeesh, I should have known because when I was investigating CAST_AS_INSTANT I came across OfferingAbility which has exactly the same issue. I think AsThoughEffect might have to be redesigned so that checking whether the effect could be used and actually using the effect are two different methods.

Yes, even the method name, applies makes it sound like it should only decided whether or not the effect applies to the given instance.

The question is: Should I do that refactoring first and then implement this now that I have a method which does not change state?

I'm tempted to say yes. It will mean I don't have to use setValue/getValue (which give me pause because of the debugging nightmare implied by having functions with so many side effects and global state)

[awjackson]

Yes, I think you should do the refactoring first. The current implementation of OfferingAbility made me recoil in horror at the hoops it jumps through to work around the fact that CAST_AS_INSTANT AsThoughs are (or were) invoked multiple times during the process of casting a spell.

[Alex-Vasile]

Yes, I think you should do the refactoring first. The current implementation of OfferingAbility made me recoil in horror at the hoops it jumps through to work around the fact that CAST_AS_INSTANT AsThoughs are (or were) invoked multiple times during the process of casting a spell.

Low and behold. It's an AsThoughEffect, and the one currently failing tests after my changes...

[awjackson]

The thing that's unusual about Offering is that you can use it when you don't need to: Offering lets you cast the spell as if it had flash, but you can also choose to pay the Offering cost when you could cast it normally.

[xenohedron]

superseded by #11114