Skip to content
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

Alternative solution to problem of unplayable cards from target adjustment #12842

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions Mage.Sets/src/mage/cards/b/BloodchiefsThirst.java
Original file line number Diff line number Diff line change
Expand Up @@ -40,9 +40,9 @@ public BloodchiefsThirst(UUID ownerId, CardSetInfo setInfo) {
"Destroy target creature or planeswalker with mana value 2 or less. " +
"If this spell was kicked, instead destroy target creature or planeswalker."
));
this.getSpellAbility().addTarget(new TargetPermanent(filter));
this.getSpellAbility().addTarget(new TargetCreatureOrPlaneswalker());
this.getSpellAbility().setTargetAdjuster(new ConditionalTargetAdjuster(KickedCondition.ONCE,
new TargetCreatureOrPlaneswalker()));
new TargetPermanent(filter), new TargetCreatureOrPlaneswalker()));
}

private BloodchiefsThirst(final BloodchiefsThirst card) {
Expand Down
4 changes: 2 additions & 2 deletions Mage.Sets/src/mage/cards/e/ExpelTheUnworthy.java
Original file line number Diff line number Diff line change
Expand Up @@ -43,9 +43,9 @@ public ExpelTheUnworthy(UUID ownerId, CardSetInfo setInfo) {
this.getSpellAbility().addEffect(new InfoEffect("Choose target creature with mana value 3 or less. If this spell was kicked, instead choose target creature."));
this.getSpellAbility().addEffect(new ExileTargetEffect().setText("Exile the chosen creature"));
this.getSpellAbility().addEffect(new ExpelTheUnworthyEffect());
this.getSpellAbility().addTarget(new TargetCreaturePermanent(filter));
this.getSpellAbility().addTarget(new TargetCreaturePermanent());
this.getSpellAbility().setTargetAdjuster(new ConditionalTargetAdjuster(KickedCondition.ONCE,
new TargetCreaturePermanent()));
new TargetCreaturePermanent(filter), new TargetCreaturePermanent()));
}

private ExpelTheUnworthy(final ExpelTheUnworthy card) {
Expand Down
5 changes: 3 additions & 2 deletions Mage.Sets/src/mage/cards/f/FightWithFire.java
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
import mage.cards.CardImpl;
import mage.cards.CardSetInfo;
import mage.constants.CardType;
import mage.target.common.TargetAnyTarget;
import mage.target.common.TargetAnyTargetAmount;
import mage.target.common.TargetCreaturePermanent;
import mage.target.targetadjustment.ConditionalTargetAdjuster;
Expand Down Expand Up @@ -35,9 +36,9 @@ public FightWithFire(UUID ownerId, CardSetInfo setInfo) {
+ "it deals 10 damage divided as you choose among any number of targets instead."
+ "<i> (Those targets can include players and planeswalkers.)</i>"
));
this.getSpellAbility().addTarget(new TargetCreaturePermanent());
this.getSpellAbility().addTarget(new TargetAnyTarget());
this.getSpellAbility().setTargetAdjuster(new ConditionalTargetAdjuster(KickedCondition.ONCE,
new TargetAnyTargetAmount(10)));
new TargetCreaturePermanent(), new TargetAnyTargetAmount(10)));
}

private FightWithFire(final FightWithFire card) {
Expand Down
5 changes: 3 additions & 2 deletions Mage.Sets/src/mage/cards/g/GaladrielsDismissal.java
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
import mage.game.permanent.Permanent;
import mage.players.Player;
import mage.target.TargetPlayer;
import mage.target.common.TargetCreatureOrPlayer;
import mage.target.common.TargetCreaturePermanent;
import mage.target.targetadjustment.ConditionalTargetAdjuster;

Expand All @@ -42,9 +43,9 @@ public GaladrielsDismissal(UUID ownerId, CardSetInfo setInfo) {
KickedCondition.ONCE, "Target creature phases out. If this spell was kicked, each creature target player controls phases out instead. " +
"<i>(Treat phased-out creatures and anything attached to them as though they don't exist until their controller's next turn.)</i>"
));
this.getSpellAbility().addTarget(new TargetCreaturePermanent());
this.getSpellAbility().addTarget(new TargetCreatureOrPlayer());
this.getSpellAbility().setTargetAdjuster(new ConditionalTargetAdjuster(KickedCondition.ONCE,
new TargetPlayer()));
new TargetCreaturePermanent(), new TargetPlayer()));
}

private GaladrielsDismissal(final GaladrielsDismissal card) {
Expand Down
16 changes: 14 additions & 2 deletions Mage.Sets/src/mage/cards/i/IntoTheFloodMaw.java
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,10 @@
import mage.cards.CardSetInfo;
import mage.constants.CardType;
import mage.constants.GiftType;
import mage.constants.TargetController;
import mage.filter.FilterPermanent;
import mage.filter.StaticFilters;
import mage.filter.predicate.Predicates;
import mage.target.TargetPermanent;
import mage.target.common.TargetOpponentsCreaturePermanent;
import mage.target.targetadjustment.ConditionalTargetAdjuster;
Expand All @@ -18,6 +21,15 @@
* @author TheElk801
*/
public final class IntoTheFloodMaw extends CardImpl {
private static final FilterPermanent playableFilter = new FilterPermanent("creature or nonland permanent");

static {
playableFilter.add(TargetController.OPPONENT.getControllerPredicate());
playableFilter.add(Predicates.or(
xenohedron marked this conversation as resolved.
Show resolved Hide resolved
CardType.CREATURE.getPredicate(),
Predicates.not(CardType.LAND.getPredicate())
));
}

public IntoTheFloodMaw(UUID ownerId, CardSetInfo setInfo) {
super(ownerId, setInfo, new CardType[]{CardType.INSTANT}, "{U}");
Expand All @@ -29,9 +41,9 @@ public IntoTheFloodMaw(UUID ownerId, CardSetInfo setInfo) {
this.getSpellAbility().addEffect(new ReturnToHandTargetEffect()
.setText("return target creature an opponent controls to its owner's hand. If the gift was promise, " +
"instead return target nonland permanent an opponent controls to its owner's hand"));
this.getSpellAbility().addTarget(new TargetOpponentsCreaturePermanent());
this.getSpellAbility().addTarget(new TargetPermanent(playableFilter));
this.getSpellAbility().setTargetAdjuster(new ConditionalTargetAdjuster(GiftWasPromisedCondition.TRUE,
new TargetPermanent(StaticFilters.FILTER_OPPONENTS_PERMANENT_NON_LAND)));
new TargetOpponentsCreaturePermanent(), new TargetPermanent(StaticFilters.FILTER_OPPONENTS_PERMANENT_NON_LAND)));
}

private IntoTheFloodMaw(final IntoTheFloodMaw card) {
Expand Down
4 changes: 2 additions & 2 deletions Mage.Sets/src/mage/cards/l/LongRiversPull.java
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,9 @@ public LongRiversPull(UUID ownerId, CardSetInfo setInfo) {
// Counter target creature spell. If the gift was promised, instead counter target spell.
this.getSpellAbility().addEffect(new CounterTargetEffect()
.setText("counter target creature spell. If the gift was promised, instead counter target spell"));
this.getSpellAbility().addTarget(new TargetSpell(StaticFilters.FILTER_SPELL_CREATURE));
this.getSpellAbility().addTarget(new TargetSpell());
this.getSpellAbility().setTargetAdjuster(new ConditionalTargetAdjuster(GiftWasPromisedCondition.TRUE,
new TargetSpell()));
new TargetSpell(StaticFilters.FILTER_SPELL_CREATURE), new TargetSpell()));
}

private LongRiversPull(final LongRiversPull card) {
Expand Down
2 changes: 0 additions & 2 deletions Mage.Sets/src/mage/cards/r/RushingRiver.java
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@
import mage.filter.StaticFilters;
import mage.target.common.TargetNonlandPermanent;
import mage.target.targetadjustment.ConditionalTargetAdjuster;
import mage.target.targetpointer.EachTargetPointer;

import java.util.UUID;

Expand All @@ -27,7 +26,6 @@ public RushingRiver(UUID ownerId, CardSetInfo setInfo) {

// Return target nonland permanent to its owner's hand. If Rushing River was kicked, return another target nonland permanent to its owner's hand.
this.getSpellAbility().addEffect(new ReturnToHandTargetEffect()
.setTargetPointer(new EachTargetPointer())
.setText("Return target nonland permanent to its owner's hand. " +
"If this spell was kicked, return another target nonland permanent to its owner's hand"));
this.getSpellAbility().addTarget(new TargetNonlandPermanent());
Expand Down
15 changes: 13 additions & 2 deletions Mage.Sets/src/mage/cards/t/TearAsunder.java
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,9 @@
import mage.cards.CardImpl;
import mage.cards.CardSetInfo;
import mage.constants.CardType;
import mage.filter.FilterPermanent;
import mage.filter.StaticFilters;
import mage.filter.predicate.Predicates;
import mage.target.TargetPermanent;
import mage.target.common.TargetNonlandPermanent;
import mage.target.targetadjustment.ConditionalTargetAdjuster;
Expand All @@ -17,6 +19,15 @@
* @author TheElk801
*/
public final class TearAsunder extends CardImpl {
private static final FilterPermanent playableFilter = new FilterPermanent("artifact, enchantment, or nonland permanent");

static {
playableFilter.add(Predicates.or(
CardType.ARTIFACT.getPredicate(),
CardType.ENCHANTMENT.getPredicate(),
Predicates.not(CardType.LAND.getPredicate())
));
}

public TearAsunder(UUID ownerId, CardSetInfo setInfo) {
super(ownerId, setInfo, new CardType[]{CardType.INSTANT}, "{1}{G}");
Expand All @@ -26,9 +37,9 @@ public TearAsunder(UUID ownerId, CardSetInfo setInfo) {

// Exile target artifact or enchantment. If this spell was kicked, exile target nonland permanent instead.
this.getSpellAbility().addEffect(new ExileTargetEffect().setText("Exile target artifact or enchantment. If this spell was kicked, exile target nonland permanent instead."));
this.getSpellAbility().addTarget(new TargetPermanent(StaticFilters.FILTER_PERMANENT_ARTIFACT_OR_ENCHANTMENT));
this.getSpellAbility().addTarget(new TargetPermanent(playableFilter));
this.getSpellAbility().setTargetAdjuster(new ConditionalTargetAdjuster(KickedCondition.ONCE,
new TargetNonlandPermanent()));
new TargetPermanent(StaticFilters.FILTER_PERMANENT_ARTIFACT_OR_ENCHANTMENT), new TargetNonlandPermanent()));
}

private TearAsunder(final TearAsunder card) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -215,4 +215,26 @@ public void testGerbilGift() {
assertHandCount(playerA, 1);
assertHandCount(playerB, 1);
}

//Test Conditional Target Adjuster allowing more generic casts
@Test
public void testLongRiversPull() {
addCard(Zone.BATTLEFIELD, playerA, "Island", 3);
addCard(Zone.HAND, playerA, "Ponder");
addCard(Zone.HAND, playerA, "Long River's Pull"); // UU, counter noncreature only if gift

setChoice(playerA, true);
setChoice(playerA, playerB.getName());
castSpell(1, PhaseStep.PRECOMBAT_MAIN, playerA, "Ponder");
castSpell(1, PhaseStep.PRECOMBAT_MAIN, playerA, "Long River's Pull");
addTarget(playerA, "Ponder");

setStrictChooseMode(true);
setStopAt(1, PhaseStep.POSTCOMBAT_MAIN);
execute();

assertHandCount(playerA, 0);
assertGraveyardCount(playerA, 2);
assertHandCount(playerB, 1);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -4,48 +4,85 @@
import mage.abilities.condition.Condition;
import mage.game.Game;
import mage.target.Target;
import mage.target.Targets;

/**
* @author notgreat
*/
public class ConditionalTargetAdjuster implements TargetAdjuster {
private final Condition condition;
private final boolean keepExistingTargets;
private final Targets replacementTargets;
private final boolean keepBlueprintTarget;
private final Target replacementTarget;
private Target blueprintTarget;
xenohedron marked this conversation as resolved.
Show resolved Hide resolved

/**
* If the condition is true, replace the target
* If the condition is true, replace the ability's target.
* <p>
* Note that if the conditional target can be found when the original can not,
* you must use the two-target constructor and give the ability a separate general target
* that can encompass both targets
*
* @param condition The condition to be checked
* @param replacementTarget The target to use if the condition is true.
*/
public ConditionalTargetAdjuster(Condition condition, Target replacementTarget) {
this(condition, false, replacementTarget);
this(condition, null, false, replacementTarget);
}

/**
* If the condition is true, change the target list with multiple targets at once
* If the condition is true, add another target to the ability
*
* @param condition The condition to be checked
* @param keepExistingTargets if true, don't clear existing targets when adding the new ones
* @param replacementTargets Targets to be added if the condition is true
* @param condition The condition to be checked
* @param keepBlueprintTarget if true, don't remove the original target when adding the new one
* @param replacementTarget The target to use if the condition is true.
*/
public ConditionalTargetAdjuster(Condition condition, boolean keepExistingTargets, Target... replacementTargets) {
public ConditionalTargetAdjuster(Condition condition, boolean keepBlueprintTarget, Target replacementTarget) {
this(condition, null, keepBlueprintTarget, replacementTarget);
}

/**
* If the condition is false, use the blueprint. If the condition is true, use the replacement target.
*
* @param condition The condition to be checked
* @param blueprintTarget The blueprint/original target to use (set to null to use the ability's first target)
* @param replacementTarget The target to use if the condition is true.
*/
public ConditionalTargetAdjuster(Condition condition, Target blueprintTarget, Target replacementTarget) {
this(condition, blueprintTarget, false, replacementTarget);
}

/**
* If the condition is false, use the blueprint. If the condition is true, add or use the replacement target.
*
* @param condition The condition to be checked
* @param blueprintTarget The blueprint/original target to use (set to null to use the ability's first target)
* @param keepBlueprintTarget if true, don't remove the original target when adding the new one
* @param replacementTarget Target to be added if the condition is true
*/
public ConditionalTargetAdjuster(Condition condition, Target blueprintTarget, boolean keepBlueprintTarget, Target replacementTarget) {
this.condition = condition;
this.keepExistingTargets = keepExistingTargets;
this.replacementTargets = new Targets(replacementTargets);
this.keepBlueprintTarget = keepBlueprintTarget;
this.blueprintTarget = blueprintTarget;
this.replacementTarget = replacementTarget;
}

@Override
public void addDefaultTargets(Ability ability) {
if (blueprintTarget == null && !ability.getTargets().isEmpty()) {
blueprintTarget = ability.getTargets().get(0).copy();
}
}

@Override
public void adjustTargets(Ability ability, Game game) {
if (condition.apply(game, ability)) {
if (!keepExistingTargets) {
ability.getTargets().clear();
}
for (Target target : replacementTargets) {
ability.addTarget(target.copy());
ability.getTargets().clear();
boolean result = condition.apply(game, ability);
if (keepBlueprintTarget || !result) {
if (blueprintTarget != null) {
ability.addTarget(blueprintTarget.copy());
}
}
if (result) {
ability.addTarget(replacementTarget.copy());
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
public interface TargetAdjuster extends Serializable {

// Warning: This is not Copyable, do not use changeable data inside (only use static objects like Filter)
// Note: in playability check for cards, targets are not adjusted.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Original commit 9c52dfa reverted from master due wrong usage. Current PR don’t use that code, but it have docs about skip adjusting.

  1. Is it fixed a bugs in current master code or just a refactored for better class usage?
  2. Is it require additional fixes due not working adjusters in playable check?

Copy link
Contributor Author

@ssk97 ssk97 Sep 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. This fixes the same major bug as 9c52dfa, where cards that should be playable are not, such as trying to use [[Long River's Pull]] with a Gift, where the base spell's target is more restrictive than the adjusted version.
  2. There are some cases where a spell that isn't currently playable is highlighted as if it were, such as [[Bloodchief's Thirst]] if you don't have enough mana to kick it and the only creatures on the battlefield have a higher MV, but I don't think there's currently any reasonable way to do that.

void adjustTargets(Ability ability, Game game);

/**
Expand Down
Loading