[PIP] Implement Codsworth, Handy Helper, improved attachment to permanent logic #12098

PurpleCrowbar · 2024-04-09T19:58:42Z

No description provided.

PurpleCrowbar · 2024-04-09T19:58:50Z

[[Codsworth, Handy Helper]]

github-actions · 2024-04-09T19:59:03Z

Codsworth, Handy Helper - (Gatherer) (Scryfall) (EDHREC)

{2}{W}
Legendary Artifact Creature — Robot
2/3
Commanders you control have ward {2}.
{T}: Add {W}{W}. Spend this mana only to cast Aura and/or Equipment spells.
{T}: Attach target Aura or Equipment you control to target creature you control. Activate only as a sorcery.

PurpleCrowbar · 2024-04-10T20:00:20Z

Closes #12108

Susucre · 2024-04-10T22:07:24Z

I feel like the attachment actions are not setup the best way.
There is currently Permanent::attachTo, Card::addAttachment and Player::addAttachment.
Those methods should probably handle the invalid attachment (and not attach if invalid), and unattach if attachment to another object is done. I think part of that is already there, but apparently they don't do all of that since you needed to take care of it for those two cards.

That being said, both cards here should be fine, so I'm not against merging as is, but could you please add a couple tests to make a future rework a little safer? Unit tests are really a great way to ensure current effort will not be undone by mistake in the future.

PurpleCrowbar · 2024-04-11T01:36:30Z

I wrote some tests and hit a roadblock with one, in which moving an "Enchant land" aura to a creature would cause it to be moved to the graveyard, as opposed to not moving at all. Looking into this, it seems to be because Permanent::cantBeAttachedBy doesn't check for whether or not the aura is actually meant to be able to enchant that object. This is presumably because the Enchant Ability is actually cosmetic (#9583) and can't be used to check whether or not the move is legal. Accordingly, the aura is moved to the creature, and then moves to the graveyard (due to state-based actions). Not sure how to resolve yet as I suppose the answer is buried in the state-based action code, the relevant section of which (I believe) is below:

mage/Mage/src/main/java/mage/game/GameImpl.java

Lines 2493 to 2559 in fd0da67

    
           Target target = spellAbility.getTargets().get(0); 
        
           if (target instanceof TargetPermanent) { 
        
               Permanent attachedTo = getPermanent(perm.getAttachedTo()); 
        
               if (attachedTo == null || !attachedTo.getAttachments().contains(perm.getId())) { 
        
                   // handle bestow unattachment 
        
                   Card card = this.getCard(perm.getId()); 
        
                   if (card != null && card.isCreature(this)) { 
        
                       UUID wasAttachedTo = perm.getAttachedTo(); 
        
                       perm.attachTo(null, null, this); 
        
                       fireEvent(new UnattachedEvent(wasAttachedTo, perm.getId(), perm, null)); 
        
                   } else if (movePermanentToGraveyardWithInfo(perm)) { 
        
                       somethingHappened = true; 
        
                   } 
        
               } else { 
        
                   Filter auraFilter = spellAbility.getTargets().get(0).getFilter(); 
        
                   if (auraFilter instanceof FilterPermanent) { 
        
                       if (!((FilterPermanent) auraFilter).match(attachedTo, perm.getControllerId(), perm.getSpellAbility(), this) 
        
                               || attachedTo.cantBeAttachedBy(perm, null, this, true)) { 
        
                           Card card = this.getCard(perm.getId()); 
        
                           if (card != null && card.isCreature(this)) { 
        
                               UUID wasAttachedTo = perm.getAttachedTo(); 
        
                               perm.attachTo(null, null, this); 
        
                               BestowAbility.becomeCreature(perm, this); 
        
                               fireEvent(new UnattachedEvent(wasAttachedTo, perm.getId(), perm, null)); 
        
                           } else if (movePermanentToGraveyardWithInfo(perm)) { 
        
                               somethingHappened = true; 
        
                           } 
        
                       } 
        
                   } else if (!auraFilter.match(attachedTo, this) || attachedTo.cantBeAttachedBy(perm, null, this, true)) { 
        
                       // handle bestow unattachment 
        
                       Card card = this.getCard(perm.getId()); 
        
                       if (card != null && card.isCreature(this)) { 
        
                           UUID wasAttachedTo = perm.getAttachedTo(); 
        
                           perm.attachTo(null, null, this); 
        
                           BestowAbility.becomeCreature(perm, this); 
        
                           fireEvent(new UnattachedEvent(wasAttachedTo, perm.getId(), perm, null)); 
        
                       } else if (movePermanentToGraveyardWithInfo(perm)) { 
        
                           somethingHappened = true; 
        
                       } 
        
                   } 
        
               } 
        
           } else if (target instanceof TargetPlayer) { 
        
               Player attachedToPlayer = getPlayer(perm.getAttachedTo()); 
        
               if (attachedToPlayer == null || attachedToPlayer.hasLost()) { 
        
                   if (movePermanentToGraveyardWithInfo(perm)) { 
        
                       somethingHappened = true; 
        
                   } 
        
               } else { 
        
                   Filter auraFilter = spellAbility.getTargets().get(0).getFilter(); 
        
                   if (!auraFilter.match(attachedToPlayer, this) || attachedToPlayer.hasProtectionFrom(perm, this)) { 
        
                       if (movePermanentToGraveyardWithInfo(perm)) { 
        
                           somethingHappened = true; 
        
                       } 
        
                   } 
        
               } 
        
           } else if (target instanceof TargetCard) { 
        
               Card attachedTo = getCard(perm.getAttachedTo()); 
        
               if (attachedTo == null 
        
                       || !(spellAbility.getTargets().get(0)).canTarget(perm.getControllerId(), perm.getAttachedTo(), spellAbility, this)) { 
        
                   if (movePermanentToGraveyardWithInfo(perm)) { 
        
                       if (attachedTo != null) { 
        
                           attachedTo.removeAttachment(perm.getId(), null, this); 
        
                       } 
        
                       somethingHappened = true; 
        
                   } 
        
               } 
        
           }

Pushing tests now, including failing test (testIllegalAttachmentDenied)

PurpleCrowbar · 2024-04-11T18:14:00Z

Fixed the problem of Permanent::cantBeAttachedBy not checking for an aura's "Enchant" target, causing enchantments to illegally attach and then immediately go to the graveyard (now they simply don't move in the first place, as intended). Also added a test for ensuring equipment doesn't move when an attempt to attach it to a noncreature permanent is made.

Susucre · 2024-04-11T18:19:03Z

Mage/src/main/java/mage/game/permanent/PermanentImpl.java

+                && attachmentPermanent != null
+                && attachmentPermanent.getSpellAbility() != null
+                && !attachmentPermanent.getSpellAbility().getTargets().isEmpty()) {
+            canAttach = game.getPermanent(attachment.getId()).getSpellAbility().getTargets().get(0).getFilter().match(this, game);


Should probably use the 4 argument match(..)
Or else stuff like "Enchant creature you control" won't work properly.
Could you add a test on Codsworth trying to attach [[Captured by the Consulate]] to one of your creature (it shouldn't move)

Edit: Uhm you might not have easy access right now to the 4 argument match as it is only defined for FilterPermanent, FilterCard, FilterPlayer, etc...

Should Filter be changed to have 4 argument match defined? not sure.

Could you add a test on Codsworth trying to attach [[Captured by the Consulate]] to one of your creature (it shouldn't move)

Having written this test now, I can confirm it is currently failing. I'm not sure what you mean about how to account for this case?

I added the 4 argument match change you suggested to Permanent::cantBeAttachedBy and can confirm that the previously failing test now passes. However, adding the checks for Player / Card to Permanent would be redundant as if we are calling the Permanent's cantBeAttachedBy method it's because we're trying to attach to a permanent specifically. The issue being that Players and Cards (and I suppose any other object, auras don't discriminate) don't have a cantBeAttachedBy equivalent. I've written two tests for the cases of trying to use Codsworth to move auras that enchant players / cards to permanents which of course currently has the auras move illegally, and then sends them to the graveyard during state based actions. Am I missing something or does this need to be fixed via adding cantBeAttachedBy equivalents to these objects (Player and Card)?

I suppose there could be a check in Permanent::cantBeAttachedBy for if the aura’s filter is an instance of FilterPlayer and immediately return true if so. I imagine the same applies to a FilterCard, unless a permanent is somehow included as a “card” in such a case

It would make sense to have cantBeAttached for Cards and Players, that for equipment/fortification always return true (as you can only attach an equipement to creatures and a fortification to a land), and for auras, checks that the target is of the proper kind, and checks its prerequisite.

Side note: are Curses properly falling off right now when they are named by Runed Halo?

Side note: are Curses properly falling off right now when they are named by Runed Halo?

Yes they are, but will add a test for that to this PR for future-proofing.

Alright, thanks for testing.
Those Attachment related methods sure do a lot in a various different places of the codebase.

Susucre · 2024-04-13T16:48:37Z

Mage/src/main/java/mage/game/permanent/PermanentImpl.java

+            Filter filter = attachmentPermanent.getSpellAbility().getTargets().get(0).getFilter();
+            if (filter instanceof FilterPermanent) {
+                canAttach = ((FilterPermanent) filter).match(this, attachmentPermanent.getControllerId(), source, game);
+            }


Would there be a downside to add

else { canAttach = false; }

here?

No; in fact, doing so causes the tests with Spellweaver Volute (enchant card in graveyard) and Psychic Possession (enchant opponent) to now pass.

Yeah I was expecting that, since they respectively have a FilterCard and a FilterPlayer for target.

The attach to permanent action should now work properly.
I guess if we ever have an attach/reattach not specific to permanents, there would need to be cantBeAttachedBy in Card/Player that would look for their own Filter.

If there is ever an Aura able to enchant both Permanent and Player, I guess the engine will burst into flames.

Susucre

I think this looks good!

Susucre · 2024-04-14T21:31:34Z

Mage.Sets/src/mage/cards/c/CodsworthHandyHelper.java

+        }
+
+        if (creature.cantBeAttachedBy(attachment, source, game, true)) {
+            game.informPlayers(attachment.getLogName() + " was not attached to " + creature.getLogName()


You can add CardUtil.getSourceLogName(game, source) at the end of most log.
Adds info for the source (Codsworth), and when you hover over it, it show the card image, which is nice.

Added in 30d14e0

JayDi85 · 2024-04-14T21:42:46Z

Mage/src/main/java/mage/game/permanent/PermanentImpl.java

-        return game.getContinuousEffects().preventedByRuleModification(new StayAttachedEvent(this.getId(), attachment.getId(), source), null, game, silentMode);
+
+        boolean canAttach = true;
+        Permanent attachmentPermanent = game.getPermanent(attachment.getId());


Why you load attachment to attachmentPermanent? It’s same object already.

Code brevity; it's more readable in the code to use a variable called attachmentPermanent and use that than it is to write game.getPermanent(attachment.getId()) each time

Nope, I’m talking about “MageObject attachment” from method’s params. It’s same object, so why not to use it? Perm = ((Permanent) attachment).

Sure, I'll do that

Also be careful — that’s method used for aura cards too, not for permanents only. So it must be compatible for both use cases.

Card usage examples:

I think this is OK - I tested with casting an aura, making the aura's target illegal, and then resolving the aura. The outcome was, as expected, the aura going to the graveyard. Will add a test for futureproofing

Current code with game.getPermanent checks validation for permanents only (e.g. aura on battlefield try to reattach). But it will ignore card (e.g. aura moves from hand to battlefield without cast/stack).

"Sneaking" an aura onto the battlefield with Eureka gives no opportunity for a permanent to become an illegal target before the user selects the target for the aura to attach to. In the case that there are no legal targets, the aura goes to the graveyard as it is unattached. Is that what you mean or did you have another interaction in mind?

See code screens above. Example with Unfinished Business:

Return target creature card from your graveyard to the battlefield, then return up to two target Aura and/or Equipment cards from your graveyard to the battlefield attached to that creature. (If the Auras can’t enchant that creature, they remain in your graveyard.)

Ah yes, of course. I have tested and can confirm that the interaction works as expected (the aura does not leave the graveyard). Will add a test for that case to this PR
Edit: Actually, there's a broken interaction with Unfinished Business. Looking into

Mage/src/main/java/mage/game/permanent/PermanentImpl.java

JayDi85 · 2024-04-14T22:18:07Z

Looks like there are many bug reports with attach problems, maybe something can be closed by that PR

PurpleCrowbar · 2024-04-14T22:40:16Z

Looks like there are many bug reports with attach problems, maybe something can be closed by that PR

I believe that #11328 is fixed by this PR Halvar has issues fixed here

PurpleCrowbar · 2024-04-15T01:00:13Z

Seems like Travis is ignoring this PR?
Edit: nope, looks like Travis is ignoring all PRs

PurpleCrowbar · 2024-04-15T02:53:21Z

Not ready to merge - despite not shown on GitHub there is a failing test and a couple more tests I plan to write

[PIP] Implement Codsworth, Handy Helper

69f0647

github-actions bot added the cards label Apr 9, 2024

PurpleCrowbar changed the title ~~[PIP] Implement Codsworth, Handy Helper~~ [DO NOT MERGE][PIP] Implement Codsworth, Handy Helper Apr 10, 2024

PurpleCrowbar mentioned this pull request Apr 10, 2024

Halvar, God of Battle wrongly attaches to protection permanents #12108

Open

Fix Codsworth and Halvar

f233f60

PurpleCrowbar changed the title ~~[DO NOT MERGE][PIP] Implement Codsworth, Handy Helper~~ [PIP] Implement Codsworth, Handy Helper Apr 10, 2024

magefree deleted a comment from github-actions bot Apr 10, 2024

Write tests for attachments

e85793d

github-actions bot added the tests label Apr 11, 2024

Fix auras going to graveyard when attaching to illegal targets

b96109e

github-actions bot added the engine label Apr 11, 2024

Susucre reviewed Apr 11, 2024

View reviewed changes

Fix Captured by the Consulate interaction

303a046

Susucre reviewed Apr 13, 2024

View reviewed changes

Fix failing tests, add additional test

a4d6092

Susucre approved these changes Apr 14, 2024

View reviewed changes

JayDi85 changed the title ~~[PIP] Implement Codsworth, Handy Helper~~ [PIP] Implement Codsworth, Handy Helper, improved attachment to permanent logic Apr 14, 2024

Add source name to log message

30d14e0

JayDi85 reviewed Apr 14, 2024

View reviewed changes

Mage/src/main/java/mage/game/permanent/PermanentImpl.java Outdated Show resolved Hide resolved

PurpleCrowbar added 2 commits April 14, 2024 23:13

Implement requested changes

4a67c66

Revert removed null check

81bf0a0

Remove filter check, clean up code

641d0b6

PurpleCrowbar added 2 commits April 15, 2024 02:27

Add additional test

a4a318b

Fix failing roles test

332940f

xenohedron marked this pull request as draft April 17, 2024 04:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PIP] Implement Codsworth, Handy Helper, improved attachment to permanent logic #12098

[PIP] Implement Codsworth, Handy Helper, improved attachment to permanent logic #12098

PurpleCrowbar commented Apr 9, 2024

PurpleCrowbar commented Apr 9, 2024

github-actions bot commented Apr 9, 2024

PurpleCrowbar commented Apr 10, 2024

Susucre commented Apr 10, 2024 •

edited

PurpleCrowbar commented Apr 11, 2024 •

edited

PurpleCrowbar commented Apr 11, 2024

Susucre Apr 11, 2024

Susucre Apr 11, 2024 •

edited

PurpleCrowbar Apr 11, 2024

PurpleCrowbar Apr 12, 2024 •

edited

Susucre Apr 13, 2024 •

edited

PurpleCrowbar Apr 14, 2024

Susucre Apr 14, 2024

Susucre Apr 13, 2024

PurpleCrowbar Apr 14, 2024

Susucre Apr 14, 2024

Susucre left a comment

Susucre Apr 14, 2024

PurpleCrowbar Apr 14, 2024

JayDi85 Apr 14, 2024

PurpleCrowbar Apr 14, 2024

JayDi85 Apr 14, 2024

PurpleCrowbar Apr 14, 2024

JayDi85 Apr 14, 2024

PurpleCrowbar Apr 14, 2024

JayDi85 Apr 14, 2024 •

edited

PurpleCrowbar Apr 14, 2024

JayDi85 Apr 14, 2024

PurpleCrowbar Apr 15, 2024 •

edited

JayDi85 commented Apr 14, 2024

PurpleCrowbar commented Apr 14, 2024

PurpleCrowbar commented Apr 15, 2024 •

edited

PurpleCrowbar commented Apr 15, 2024

[PIP] Implement Codsworth, Handy Helper, improved attachment to permanent logic #12098

Are you sure you want to change the base?

[PIP] Implement Codsworth, Handy Helper, improved attachment to permanent logic #12098

Conversation

PurpleCrowbar commented Apr 9, 2024

PurpleCrowbar commented Apr 9, 2024

github-actions bot commented Apr 9, 2024

PurpleCrowbar commented Apr 10, 2024

Susucre commented Apr 10, 2024 • edited

PurpleCrowbar commented Apr 11, 2024 • edited

PurpleCrowbar commented Apr 11, 2024

Choose a reason for hiding this comment

Susucre Apr 11, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PurpleCrowbar Apr 12, 2024 • edited

Choose a reason for hiding this comment

Susucre Apr 13, 2024 • edited

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

Susucre left a 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

JayDi85 Apr 14, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PurpleCrowbar Apr 15, 2024 • edited

Choose a reason for hiding this comment

JayDi85 commented Apr 14, 2024

PurpleCrowbar commented Apr 14, 2024

PurpleCrowbar commented Apr 15, 2024 • edited

PurpleCrowbar commented Apr 15, 2024

Susucre commented Apr 10, 2024 •

edited

PurpleCrowbar commented Apr 11, 2024 •

edited

Susucre Apr 11, 2024 •

edited

PurpleCrowbar Apr 12, 2024 •

edited

Susucre Apr 13, 2024 •

edited

JayDi85 Apr 14, 2024 •

edited

PurpleCrowbar Apr 15, 2024 •

edited

PurpleCrowbar commented Apr 15, 2024 •

edited