[WIP] Refactor AsThoughEffect to split .applies into two functions

[Alex-Vasile]

See related issue for explanation of why it's needed.

The idea is is that AsThoughEffect .applies() method (both version) is split into two functions .applies and .apply (both taking the same parameters. The .applies() checks if the AsThoughEffect applies to the situation.

The important thing is that .applies does not (for the most part, see TODOs at the bottom) make changes to the state or ask the user for anything. All of that must be handled by the .apply() method. the .apply() method is called if and only if .applies() returns true.

This does not address any of the problems that #9268 is trying to address. After this is merged, that PR will be rebased off of this one and re-written with the new functions in mind. Because of that asThough() and asThoughMana() look a little funny. They do not take advantage of the changes yet, that will be done in #9268 afterwards.

Changes:

• AsThoughEffectImpl now implements a default .apply(Game, Ability). All of the Overrides were simply returning true.
• Split the functionality found in .applies() over into 2 functions.
• Simplified several highly nested AsThoughEffects that I ran across.

TODOs:

• [ ] The rest of the TODOs
• [ ] Figure out how to deal with .discard() in .applies() and with IntetTheDreamerLookEffect.
  • I left these calls in there since they require the information available only inside .applies. There is no way to know if the effects left in there must be applied once the .applies() method returns. Importantly, they cannot be placed in .apply() since the changes in question would always be accompanied by a .applies() returning false (so .apply() would not be called)
• [ ] Should I merge this and #9268 and fix those issues in here as well?

TODO after this is merged:

• [ ] change parameter name and ordering of .applies to be consistent (no functional changes but will add a lot of noise to this PR)

Closes #9521.

[JayDi85]

• applies for checks and other fast and safe code (e.g. can be used multiple times per game cycle and can be calls from any place -- without any feedback/dialogs/choices, it's very important);
• apply for real action (e.g. can be used one time per game cycle, can contains dialogs -- the last one depends on effect type, see docs/warning in some effects);
• discard calls to remove unused effect from the game. I think it's an optimization/workaround cause some abilities bugged and can generate too much effects without finishing it (example: wrong/unknown end step). You must not call a discard command in normal situation. So it's can be good to refactor/remove that calls (if possible).

It's a standard logic all around xmage for apply/applies and same methods (example: checkTrigger, canActivate, etc).

Can you explain that you mean by "split .applies into two functions"?

1. If you try to remove state changeable code from applies to apply then it's ok;
2. If you try to make another things then explain.

[Alex-Vasile]

• applies for checks and other fast and safe code (e.g. can be used multiple times per game cycle and can be calls from any place -- without any feedback/dialogs/choices, it's very important);
• apply for real action (e.g. can be used one time per game cycle, can contains dialogs -- the last one depends on effect type, see docs/warning in some effects);
• discard calls to remove unused effect from the game. I think it's an optimization/workaround cause some abilities bugged and can generate too much effects without finishing it (example: wrong/unknown end step). You must not call a discard command in normal situation. So it's can be good to refactor/remove that calls (if possible).

It's a standard logic all around xmage for apply/applies and same methods (example: checkTrigger, canActivate, etc).

Can you explain that you mean by "split .applies into two functions"?

1. If you try to remove state changeable code from applies to apply then it's ok;
2. If you try to make another things then explain.

What you describe is what I am doing.

Before this PR the applies methods for AsThoughEffect changed state and asked users for input, meaning that it could not be called multiple times.

This PR takes the code that is inside applies and puts it across two functions applies and apply (as you describe in your bullet points). The code in applies checks if the AsThoughEffect can be used in a given situation (mostly) without changing state. And the code from applies which requires user input or a change to the state (e.g. adding a life cost when playing a cast with Bolas's Citadel) has been moved to apply.

A few of the effects still change state (mostly calls to discard()) and I still need to figure out how to get rid of that.

[JayDi85]

There are another potential code for refactor: XanatharGuildKingpin -- it's uses discard + applies combo to execute a code one time only for effect's restart with new settings. Or it's just an example of discard usage (I don't like such workarounds).

[alexander-novo]

Hey all, do we know what the current path to getting this resolved looks like? Let me know what kind of work still needs to be done and maybe I can get it past the finish line. Right now this is currently a blocker for [[Henzie "Toolbox" Torre]]

[github-actions[bot]]

Henzie "Toolbox" Torre - (Gatherer) (Scryfall) (EDHREC)

{B}{R}{G} Legendary Creature — Devil Rogue 3/3 Each creature spell you cast with mana value 4 or greater has blitz. The blitz cost is equal to its mana cost. (You may choose to cast that spell for its blitz cost. If you do, it gains haste and "When this creature dies, draw a card." Sacrifice it at the beginning of the next end step.) Blitz costs you pay cost {1} less for each time you've cast your commander from the command zone this game.

[Alex-Vasile]

Hey all, do we know what the current path to getting this resolved looks like? Let me know what kind of work still needs to be done and maybe I can get it past the finish line. Right now this is currently a blocker for Henzie "Toolbox" Torre

I haven't worked on this in a while so all I know is the TODOs I left here and in the code. In addition to those:

• bring this branch up to date with master and see if any new cards were added that use this effect.
• See if this PR is actually still needed given that #11114 was merged.

Feel free to take it over.

[alexander-novo]

Hey all, do we know what the current path to getting this resolved looks like? Let me know what kind of work still needs to be done and maybe I can get it past the finish line. Right now this is currently a blocker for Henzie "Toolbox" Torre

I haven't worked on this in a while so all I know is the TODOs I left here and in the code. In addition to those:

• bring this branch up to date with master and see if any new cards were added that use this effect.

• See if this PR is actually still needed given that Rework AsThough handling to allow choosing/affecting a specific alternate cast #11114 was merged.

Feel free to take it over.

As far as I know, that PR is orthogonal to this one. That PR was for if multiple AsThoughEffects applied to the same card, and lets you choose which one to use (i.e. if they give different cost reductions). Looking through the AsThoughEffects currently on master, it looks like this PR is still needed.

[Alex-Vasile]

The idea of the PR is to split the affect into two methods, one that does not affect state which checks if the effect can apply to a situation, and one which uses the effect and changes state. One or two cards are tricky, I mentioned them above/comments in the code, since they require passing info from one method to another. That portion still needs solving. I am also unsure if I have properly dealt with all of the instances.

Oh and this need tests.