[YMID] Implement Arms Scavenger and 3 other cards, rework Seek effect #12802

Sidorovich77 · 2024-09-04T15:55:54Z

https://scryfall.com/card/ymid/55/settle-the-wilds
https://scryfall.com/card/ymid/48/geistpack-alpha
https://scryfall.com/card/ymid/43/tibalt-wicked-tormentor
https://scryfall.com/card/ymid/35/arms-scavenger

JayDi85 · 2024-09-04T16:14:04Z

Mage.Sets/src/mage/cards/g/GeistpackAlpha.java

+        this.addAbility(TrampleAbility.getInstance());
+
+
+        // When Geistpack Alpha dies, seek a permanent card with mana value equal to the number of lands you control.


miss card hint with lands amount info

JayDi85 · 2024-09-04T16:16:21Z

Mage.Sets/src/mage/cards/s/SettleTheWilds.java

+        super(ownerId, setInfo, new CardType[]{CardType.SORCERY}, "{2}{G}");
+
+        // Seek a basic land card and put it onto the battlefield tapped.
+        // Then seek a permanent card with mana value equal to the number of lands you control.


Must use one line per ability for rules text as comment for better text and regexp searching by IDE

JayDi85 · 2024-09-04T16:19:59Z

Mage.Sets/src/mage/cards/s/SettleTheWilds.java

+
+    SettleTheWildsSeekEffect() {
+        super(Outcome.Benefit);
+        staticText = "Then seek a permanent card with mana value equal to the number of lands you control.";


Why it's a custom effect instead of standard SeekCardEffect?

Agreed, the custom filter isn't actually necessary. You can make a predicate like ManaValueLessThanControlledLandCountPredicate but with equal to instead

JayDi85 · 2024-09-04T16:25:31Z

Mage.Sets/src/mage/cards/t/TibaltWickedTormentor.java

+        this.addAbility(ability);
+
+        // +1: Tibalt, Wicked Tormentor deals 4 damage to target creature or planeswalker unless its controller has Tibalt deal 4 damage to them.
+        // If they do, you may discard a card. If you do, draw a card.


must use one line comment in card

JayDi85 · 2024-09-04T16:26:34Z

Mage.Sets/src/mage/cards/t/TibaltWickedTormentor.java

+    public boolean apply(Game game, Ability source) {
+        Permanent permanent = game.getPermanent(source.getFirstTarget());
+        Player player = game.getPlayer(source.getControllerId());
+        Player controller = game.getPlayer(permanent.getControllerId());


Miss permanent null check

Looks like you wrongly use player and controller (must invert). Also check who does damage and make an answer.

JayDi85 · 2024-09-04T16:38:02Z

...c/main/java/mage/abilities/effects/common/DraftFromSpellbookThenExileCastThisTurnEffect.java

+/**
+ * @author Sidorovich77
+ */
+public class DraftFromSpellbookThenExileCastThisTurnEffect extends OneShotEffect {


Instead copy paste full code from existing effect -- extends DraftFromSpellbookEffect and override apply method like: "if super.apply then exile/cast"

Maybe should be a method in Player class Set<Card> draftFromSpellbook(List<String> spellbook, PutCards putCards) to handle the functionality

JayDi85 · 2024-09-04T16:39:32Z

...c/main/java/mage/abilities/effects/common/DraftFromSpellbookThenExilePlayThisTurnEffect.java

+/**
+ * @author Sidorovich77
+ */
+public class DraftFromSpellbookThenExilePlayThisTurnEffect extends OneShotEffect {


@xenohedron
As I remember play and cast now are same due oracle changes few releases ago? If true then no needs in duplicated class

I'm pretty sure there's still a difference - but shouldn't need a duplicated class anyway.

JayDi85 · 2024-09-04T16:47:55Z

Mage/src/main/java/mage/abilities/effects/common/SeekCardEffect.java

+                return false;
+            }
+            game.informPlayers(controller.getLogName() + " seeks a card from their library");
+            if (inZone == Zone.BATTLEFIELD) {


New zone code must be part of Player.seekCard method (additional params), not here.

I've decided to modify SeekCardEffect directly instead of using Player's seekCard() because of next problem.
There are several cards that manipulate those cards that were found through the Seek. For example, Signature Spells or Back-Alley Gardener or such a complex example like Plunderer's Prize.
So it seems that seekCard() method in PlayerImpl should return cards that it has found, for example as Cards interface. Would it be more appropriate way?

Yes, changing the return type of that method to Card or Cards is the way to go (then when you need the boolean result, you check if null/empty)

Sidorovich77 · 2024-09-15T11:54:54Z

I've added more complex Seek method that returns set of cards to allow further manipulations with sought cards. Rule generation is simple enough, because of variety of rule text in cards with seek. I hope that setText() would be suitable for any case.

xenohedron · 2024-09-15T20:10:02Z

Mage/src/main/java/mage/players/PlayerImpl.java

@@ -2972,6 +2972,40 @@ public boolean seekCard(FilterCard filter, Ability source, Game game) {
        return true;
    }

+    @Override
+    public Set<Card> seekCard(FilterCard filter, Zone inZone, boolean tapped, int amount, Ability source, Game game) {
+        Player controller = game.getPlayer(source.getControllerId());


I'm confused, why do you need controller? this is a method in PlayerImpl, so it seems like a redundant param. (If it's not dependent on the player, then it should instead be directly in the apply method of the Effect.)

Consider using PutCards rather than Zone + tapped.

Also, you should be able to simplify the existing seekCard method to call this more general one - don't want to have duplicate code.

xenohedron · 2024-09-15T20:26:46Z

...c/main/java/mage/abilities/effects/common/DraftFromSpellbookThenExilePlayThisTurnEffect.java

+/**
+ * @author Sidorovich77
+ */
+public class DraftFromSpellbookThenExilePlayThisTurnEffect extends OneShotEffect {


I'm pretty sure there's still a difference - but shouldn't need a duplicated class anyway.

xenohedron · 2024-09-15T20:29:23Z

...c/main/java/mage/abilities/effects/common/DraftFromSpellbookThenExileCastThisTurnEffect.java

+/**
+ * @author Sidorovich77
+ */
+public class DraftFromSpellbookThenExileCastThisTurnEffect extends OneShotEffect {


Maybe should be a method in Player class Set<Card> draftFromSpellbook(List<String> spellbook, PutCards putCards) to handle the functionality

xenohedron · 2024-09-15T20:32:04Z

Mage.Sets/src/mage/cards/t/TibaltWickedTormentor.java

+        if (permanent == null) {
+            return false;
+        }
+        Player you = game.getPlayer(source.getControllerId());


no need for "you" right? it's not used anywhere

xenohedron · 2024-09-15T20:34:39Z

Mage.Sets/src/mage/cards/s/SettleTheWilds.java

+
+    SettleTheWildsSeekEffect() {
+        super(Outcome.Benefit);
+        staticText = "Then seek a permanent card with mana value equal to the number of lands you control.";


Agreed, the custom filter isn't actually necessary. You can make a predicate like ManaValueLessThanControlledLandCountPredicate but with equal to instead

Sidorovich77 added 3 commits September 4, 2024 18:24

Implement Arms Scavenger and 3 other cards, rework Seek effect

57c73a2

Merge branch 'refs/heads/YMID' into Innistrad

12937fb

Implement Arms Scavenger and 3 other cards, rework Seek effect

623a20e

github-actions bot added cards engine labels Sep 4, 2024

Sidorovich77 added 2 commits September 4, 2024 19:12

Remove unfinished work

d9992b4

Implement Arms Scavenger and 3 other cards, rework Seek effect

5304dcc

JayDi85 reviewed Sep 4, 2024

View reviewed changes

Sidorovich77 added 2 commits September 8, 2024 16:33

Fixed Tibalt for test.

254095f

Fix for review.

8bfd51f

github-actions bot added the tests label Sep 15, 2024

Sidorovich77 requested a review from xenohedron September 15, 2024 11:55

Fix for review.

14948bf

xenohedron requested changes Sep 15, 2024

View reviewed changes

Sidorovich77 marked this pull request as draft October 17, 2024 18:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[YMID] Implement Arms Scavenger and 3 other cards, rework Seek effect #12802

[YMID] Implement Arms Scavenger and 3 other cards, rework Seek effect #12802

Sidorovich77 commented Sep 4, 2024

JayDi85 Sep 4, 2024

JayDi85 Sep 4, 2024

JayDi85 Sep 4, 2024

xenohedron Sep 15, 2024

JayDi85 Sep 4, 2024

JayDi85 Sep 4, 2024

JayDi85 Sep 4, 2024

xenohedron Sep 15, 2024

JayDi85 Sep 4, 2024

xenohedron Sep 15, 2024

JayDi85 Sep 4, 2024

Sidorovich77 Sep 9, 2024 •

edited

Loading

xenohedron Sep 14, 2024

Sidorovich77 commented Sep 15, 2024

xenohedron Sep 15, 2024

xenohedron Sep 15, 2024

xenohedron Sep 15, 2024

xenohedron Sep 15, 2024

xenohedron Sep 15, 2024

		this.addAbility(TrampleAbility.getInstance());


		// When Geistpack Alpha dies, seek a permanent card with mana value equal to the number of lands you control.

[YMID] Implement Arms Scavenger and 3 other cards, rework Seek effect #12802

Are you sure you want to change the base?

[YMID] Implement Arms Scavenger and 3 other cards, rework Seek effect #12802

Conversation

Sidorovich77 commented Sep 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sidorovich77 Sep 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sidorovich77 commented Sep 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sidorovich77 Sep 9, 2024 •

edited

Loading