REFACTOR: AsThoughEffect need a refactor
- Dominant language
- Java
- Stars
- 2.4k
- Forks
- 940
- Avg merge
- 2d 12h
- Merged PRs (30d)
- 160
Description
Currently AsThoughEffect use their `applies` method to *both* identify if the asThoughEffect can be used AND to make changes to the state of the game.
E.g. [[Bolas's Citadel]] has the following code at the end of its `.applies`:
https://github.com/magefree/mage/blob/572104b8fcdb50b509c34b10e113cf39602374bb/Mage.Sets/src/mage/cards/b/BolassCitadel.java#L116-L122
If this method is called, it will cause the controller to pay life instead of mana to cast the spell *even if they did not want to use Bolas's citadel*.
This in turns requires each of these effects to be checked while it `game.inCheckPlayableState() == true`, and list of them to be stored in the state so that one the second pass (when `game.inCheckPlayableState() == false`) that list can be read back and one of the entries in it be picked.
This needs to change. I propose that the functionality be split across two methods:
1. The first one which checks if the asThoughEffect applies in this instance
2. The second actually makes and changes required to the state
This is the same idea as how TriggeredAbility has `checkEventType`, `checkTrigger` and only after `resolve`.
_Out of the discussion in https://github.com/magefree/mage/issues/9268#issuecomment-1243287853_
Contributor guide
No contributing guide indexed for this repository
Research direction
Start with the AsThoughEffect `applies` method and the BolassCitadel.java example linked in the issue, then trace `game.inCheckPlayableState()` and the stored effect list. Compare this flow with TriggeredAbility’s `checkEventType`, `checkTrigger`, and `resolve` methods. Done means applicability checks no longer change game state, and the affected effects and state handling follow the proposed separation.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- java
- Domain
- game-dev
- Issue type
- Refactor
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Stale
- Clarity
- Mostly clear
- Newbie friendliness
- 25/100