magefree / mage

Magic Another Game Engine
http://xmage.today
MIT License
1.85k stars 761 forks source link

Sneak Attack doesn't give MDFC creatures haste #8474

Open alexander-novo opened 2 years ago

alexander-novo commented 2 years ago

[[Sneak Attack]] doesn't give MDFC creatures haste. Just observed with [[Tergrid, God of Fright]].

github-actions[bot] commented 2 years ago

Sneak Attack - (Gatherer) (Scryfall) (EDHREC)

{3}{R} Enchantment {R}: You may put a creature card from your hand onto the battlefield. That creature gains haste. Sacrifice the creature at the beginning of the next end step.

Tergrid, God of Fright // Tergrid's Lantern - (Gatherer) (Scryfall) (EDHREC)

{3}{B}{B} Legendary Creature — God 4/5 Menace Whenever an opponent sacrifices a nontoken permanent or discards a permanent card, you may put that card from a graveyard onto the battlefield under your control. :arrows_counterclockwise: {3}{B} Legendary Artifact {T}: Target player loses 3 life unless they sacrifice a nonland permanent or discard a card. {3}{B}: Untap Tergrid's Lantern.

weirddan455 commented 2 years ago

@jeffwadsworth I was also just taking a look at this. It looks like MDFCs always enter the battlefield as their left half, which has a different UUID from the main card Id. I have a fix but it's maybe not the best. Check if it's an instance of ModalDoubleFacesCard. If it is, get the permanent from the left side's Id. There may be a cleaner way to fix this in engine code but I'm not sure.

Goryo's Vengeance and probably other cards are broken in the same way.

JayDi85 commented 2 years ago

enter the battlefield as their left half, which has a different UUID from the main card Id

That's correct. MDF cards and sides are fully independent cards with different ids (as example: transformable cards are same single card with different characteristics).

There are dozen cards with same code style (move + gain haste). Search by:

So it must be improved for all cards.

Workaround 1 (different action order): GainAbilityTargetEffect support targeting to cards and it used for same code style in other places (gives a haste before put card to battlefield). But it must add additional effect before move call.

Workaround 2 (usage of CardUtil.getDefaultCardSideForBattlefield): Broken code:

controller.moveCards(card...
game.getPermanent(card...

Can be fixed by:

game.getPermanent(CardUtil.getDefaultCardSideForBattlefield(game, card).getId())

Maybe another fixes possible.

JayDi85 commented 2 years ago

I added ModalDoubleFacesCardsTest.test_FindMovedPermanentByCard and a fix from workaround 2 in a5eb02515571727b53e95604f1db4d11090e4d52. So you can test, search other potential broken cards or find another workaround.

JayDi85 commented 2 years ago

Too much cards with same code style (100+). Regexp searches:

There are comments in Game->getPermanent from commit 10cf9c4a4e03f709c9989d3299edb486ccb06ead:

Find permanent on the battlefield by id. If you works with cards and want to check it on battlefield then use getState().getZone() instead. Card's id and permanent's id can be different (example: mdf card puts half card to battlefield, not the main card).

There are other places with getPermanent methods: getBattlefield().getPermanent getBattlefield().containsPermanent

It can be modified to support mdfc search logic (e.g. find a half's permanent by main card id), but it's add a different logic to different getPermanent methods. So must be restricted.

Conclusion: moveCards + getPermanent combo must be improved to use CardUtil.getDefaultCardSideForBattlefield(game, card) instead. Or game.getPermanent call must be replaced by game.getPermanentAfterMove call in that use cases.

theelk801 commented 2 years ago

in general we want something better for tracking cards as they change zones, especially if we want to implement mutate properly