-
Notifications
You must be signed in to change notification settings - Fork 822
Implement [ICE],[ME2] Goblin Ski Patrol #13285
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
About the two areas that could be improved.
[[Goblin Ski Patrol]] |
Goblin Ski Patrol - (Gatherer) (Scryfall) (EDHREC)
|
Nope, new controller must sacrifice it. There are oracle text:
|
Effect effect = new BoostSourceEffect(2, 0, Duration.EndOfTurn); | ||
effect.setText("{this} gets +2/+0"); | ||
// This should be ActivateOncePerGameActivatedAbility but I couldn't work out how to apply a condition, so used LimitedTimesPerTurnActivatedAbility instead. This will only be an issue if Goblin Ski Patrol somehow avoids being sacrificed. | ||
Ability ability = new LimitedTimesPerTurnActivatedAbility(Zone.BATTLEFIELD, effect, new ManaCostsImpl<>("{1}{R}"), 1, condition); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Must modify ActivateOncePerGameActivatedAbility to support required conditional
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Found ThoughtShucker uses mage.abilities.decorator.ConditionalActivatedAbility and applies maxActivationsPerGame = 1 so used that method. Also discovered by reading ArmorOfThorns that a parameter can be passed to SacrificeSourceEffect to require its controller to sacrifice it, so did that also. fingers crossed This should be right, once I solve the compilation error. |
I wasn't able to get the mage.abilities.decorator.ConditionalActivatedAbility version from ThoughtShucker working, so reverted to original implementation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All must be fine except controlled filter.
import mage.abilities.condition.common.PermanentsOnTheBattlefieldCondition; | ||
import mage.abilities.hint.ConditionHint; | ||
import mage.abilities.hint.Hint; | ||
import mage.filter.FilterPermanent; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Must use FilterControlledPermanent as a filter
Sorry I hadn't seen the commit by JayDi when I was trying to solve the build error.
part of #5382
Original comment - no longer applies
The one (edit - now two) things that aren't quite right is:I used code from Pyric Salamander which states
So I believe that if Goblin Ski Patrol somehow changes controller before the next end step, the new controller wont have to sacrifice it - which they still should.
Edit - now two things. Because ActivateOncePerGameActivatedAbility didn't have the option to apply a condition, I needed to use LimitedTimesPerTurnActivatedAbility instead. This means if Goblin Ski Patrol somehow avoids getting sacrificed at the end of turn, then its ability can be reused in a later turn. (This shouldn't be the case.)