[DSK] Implement Acrobatic Cheerleader #13232
Conversation
|
You need to change the name of the PR [DSK] not [DKS]. |
|
I mean, it's nice to spell correctly, but nothing actually relies on the names of pull requests. |
Grath
changed the title
| } | ||
| String keyAlreadyTriggered = getKeyAlreadyTriggered(game); | ||
| Boolean alreadyTriggered = (Boolean) game.getState().getValue(keyAlreadyTriggered); | ||
| if (alreadyTriggered != null && alreadyTriggered.equals(Boolean.TRUE)) { |
There was a problem hiding this comment.
No need in boolean param at all (save and load). Just use count param > 0.
There was a problem hiding this comment.
You're right, I'll optimize.
| @@ -71,6 +71,7 @@ public void trigger(Game game, UUID controllerId, GameEvent triggeringEvent) { | |||
| //20091005 - 603.4 | |||
| if (checkInterveningIfClause(game)) { | |||
| setLastTrigger(game); | |||
| updateTriggerCount(game); | |||
There was a problem hiding this comment.
Better to use same naming for both stats methods like:
- setLastTrigger: updateTurnCounts
- updateTriggerCount: updateGameCounts
|
|
||
| // Survival -- At the beginning of your second main phase, if Acrobatic Cheerleader is tapped, put a flying counter on it. This ability triggers only once. | ||
| TriggeredAbility ability = new SurvivalAbility(new AddCountersSourceEffect(CounterType.FLYING.createInstance())) | ||
| .setTriggersLimitEachGame(1) |
There was a problem hiding this comment.
BTW you can improve UX by auto-adding default card hints to the ability (one for turn limit, one for game limit). It can help to show limit info and close #12680. See lands card hint example (it allow to show hints for limited abilities only, not for all): a5c354f
Maybe improved here or by another PR.
There was a problem hiding this comment.
There are possible multiple/diff triggers per card (see #12300), so card hint must contain simple info about related ability (cause hints goes after all abilities and must be unique by design). Example:
- [good/bad icon] Can trigger more times that turn (1 of 2, [some part of the ability text like CardUtil.substring(this, 50, …) + this.getoriginalid().tostring().substring(1,3)])
There was a problem hiding this comment.
BTW you can improve UX by auto-adding default card hints to the ability (one for turn limit, one for game limit). It can help to show limit info and close #12680. See lands card hint example (it allow to show hints for limited abilities only, not for all): a5c354f
Maybe improved here or by another PR.
Maybe I can work on it by another PR linked to that issue. What do you think?
|
I'm writing some unit tests. |
|
The Travis CI build fails because of this error: "Error: (rarity) mismatched. MtgJson has Common while set file has Uncommon for DSC - Falkenrath Noble - 140". It has not been introduced by this PR. |
Part of #12534
I have tested the functionality of Acrobatic Cheerleader and other per-turn limited triggered abilities by directly running the Game server and client.
Since this update involves modifications to the TriggeredAbility feature, please let me know if you think additional or more in-depth testing is necessary.