[M3C] Implement Desert Warfare #12749
Conversation
| /** | ||
| * @author Sidorovich77 | ||
| */ | ||
| public enum DesertsYouControlCount implements DynamicValue { |
There was a problem hiding this comment.
This should not be a common class. You can use existing PermanentsOnBattlefieldCount with a filter.
| /** | ||
| * @author JayDi85 | ||
| */ | ||
| public enum DesertsYouControlHint implements Hint { |
There was a problem hiding this comment.
This also should not be a common class. Just use ValueConditionHint.
| // graveyard from your hand or library, put that card onto the battlefield under your control at the beginning of your next end step. | ||
|
|
||
| //Based on Seraph, Yuma, Proud Protector | ||
| Effect effect = new CreateDelayedTriggeredAbilityEffect((DelayedTriggeredAbility) new AtTheBeginOfNextEndStepDelayedTriggeredAbility(new DesertWarfareReturnEffect(), TargetController.YOU).setTriggerPhrase("Put that card onto the battlefield under your control at the beginning of your next end step.").setRuleVisible(false)); |
There was a problem hiding this comment.
Hiding the rule is bad style. I kinda see what you're doing here, simulating the compound trigger condition by two abilities, but it is not a clean approach.
You're already writing a custom trigger, so just go all the way and check SACRIFICED_PERMANENT in it as well, and remove extraneous parameters. Setting the trigger phrase in that class and the staticText in the effect class should be sufficient, without all this other manual text setting.
| @Override | ||
| public boolean apply(Game game, Ability source) { | ||
| Player player = game.getPlayer(source.getControllerId()); | ||
| Permanent permanent = source.getSourcePermanentIfItStillExists(game); |
There was a problem hiding this comment.
why are you checking source permanent existing? it's not necessary for the effect.
| if (player == null || permanent == null) { | ||
| return false; | ||
| } | ||
| int deserts = game.getBattlefield().countAll(new FilterLandPermanent(SubType.DESERT, "Deserts"), game.getControllerId(source.getSourceId()), game); |
There was a problem hiding this comment.
can't you just use player.getId() here?
also, "Deserts" does not specify land
| @Override | ||
| public boolean checkTrigger(GameEvent event, Game game) { | ||
| Card card = game.getCard(event.getTargetId()); | ||
| if (((ZoneChangeEvent) event).getToZone() == Zone.GRAVEYARD |
There was a problem hiding this comment.
don't repeat checks in both branches of an if-else, perform the common checks first. return false at everything that doesn't trigger, and only have a single setTargetPointer at the end before returning true
also it's simpler to declare ZoneChangeEvent zEvent = (ZoneChangeEvent) event so you only have to cast once
|
|
||
| public PutCardIntoGraveFromHandLibraryAllTriggeredAbility(Zone zone, Effect effect, boolean optional, TargetController targetController, SetTargetPointer setTargetPointer) { | ||
| super(zone, effect, optional); | ||
| FilterCard filter = filterCard.copy(); |
There was a problem hiding this comment.
FilterOwnedCard exists.
This is a custom class, so it should include the card name, and params should just be hardcoded.
|
|
||
| @Override | ||
| public int calculate(Game game, Ability sourceAbility, Effect effect) { | ||
| return new PermanentsOnBattlefieldCount(new FilterControlledPermanent(SubType.DESERT)).calculate(game, sourceAbility, null); |
There was a problem hiding this comment.
Don't wrap this in a custom class. Declare it as private static final DynamicValue xValue and likewise for the hint
| @@ -0,0 +1 @@ | |||
|
|
|||
There was a problem hiding this comment.
Remove these empty files
|
|
||
| @Override | ||
| public boolean apply(Game game, Ability source) { | ||
| if (source.getControllerId() == null) { |
There was a problem hiding this comment.
what's the point of this null check?
| } | ||
| } | ||
|
|
||
| class DesertWarfareReturnEffect extends OneShotEffect { |
There was a problem hiding this comment.
why do you actually need a custom class, won't ReturnFromGraveyardToBattlefieldTargetEffect suffice?
| super(zone, effect, optional); | ||
| FilterOwnedCard filter = filterCard.copy(); | ||
| this.setTargetPointer = setTargetPointer; | ||
| filter.add(targetController.getOwnerPredicate()); |
There was a problem hiding this comment.
there is no point in copying the filter and adding a redundant predicate
|
|
||
| private final SetTargetPointer setTargetPointer; | ||
|
|
||
| public DesertWarfareTriggeredAbility(Zone zone, Effect effect, boolean optional, TargetController targetController, SetTargetPointer setTargetPointer) { |
There was a problem hiding this comment.
this is a custom class so hardcode all the params and remove the ones you don't need
|
|
||
| protected DesertWarfareTriggeredAbility(final DesertWarfareTriggeredAbility ability) { | ||
| super(ability); | ||
| filterCard = filterCard.copy(); |
There was a problem hiding this comment.
don't copy static member (it should be final anyway)
| switch (event.getType()) { | ||
| case SACRIFICED_PERMANENT: | ||
| Permanent permanent = game.getPermanentOrLKIBattlefield(event.getTargetId()); | ||
| if (permanent == null || !permanent.hasSubtype(SubType.DESERT, game)) { |
There was a problem hiding this comment.
you're missing playerid check
https://scryfall.com/card/m3c/64/desert-warfare