Collectible Card Game Headquarters

ArsenalNut wrote:I think it implies that the triggered ability should be using the LKI of the clone which would be the copied creature. This means I need to fix what Forge currently does but I was looking a definitive answer before I went down that path.

The Copying rules are giving me a headache

Yep that's right, the creature that left the battlefield is whatever had been Cloned. "Cemetery Puca copies the printed values of the creature, plus any copy effects that have been applied to it." But not changes by other effects/tokens/etc.

And yes, copying rules and LKI are a pain.

friarsol wrote:

ArsenalNut wrote:I think it implies that the triggered ability should be using the LKI of the clone which would be the copied creature. This means I need to fix what Forge currently does but I was looking a definitive answer before I went down that path.

The Copying rules are giving me a headache

Yep that's right, the creature that left the battlefield is whatever had been Cloned. "Cemetery Puca copies the printed values of the creature, plus any copy effects that have been applied to it." But not changes by other effects/tokens/etc.

And yes, copying rules and LKI are a pain.

Game state...boy, would that fix a lot of things.

friarsol wrote:

ArsenalNut wrote:I think it implies that the triggered ability should be using the LKI of the clone which would be the copied creature. This means I need to fix what Forge currently does but I was looking a definitive answer before I went down that path.

The Copying rules are giving me a headache

Yep that's right, the creature that left the battlefield is whatever had been Cloned. "Cemetery Puca copies the printed values of the creature, plus any copy effects that have been applied to it." But not changes by other effects/tokens/etc.

And yes, copying rules and LKI are a pain.

I was tracing through the issue I am having with Cemetery Puca and found the culprit. The LKI get stored perfectly in TriggeringObject. However, getDefinedCards calls AllZoneUtil.getCardState to find the current state which is now a Clone in the graveyard. Anybody now why the LKI version of the card isn't just returned?

Edit: If I return the LKI version, Cemetery Puca works fine, but is that the correct fix? This not a part of the code I am familiar with.

ArsenalNut wrote:I was tracing through the issue I am having with Cemetery Puca and found the culprit. The LKI get stored perfectly in TriggeringObject. However, getDefinedCards calls AllZoneUtil.getCardState to find the current state which is now a Clone in the graveyard. Anybody now why the LKI version of the card isn't just returned?

Edit: If I return the LKI version, Cemetery Puca works fine, but is that the correct fix? This not a part of the code I am familiar with.

I think cards like Angelic Renewal need to find the card in the graveyard, so it would be better to add and use "TriggeredCardLKICopy" as an option for getDefinedCards.

I've got the all the hard code clones converted to scripts. The conversion has fixed issues with things like Undying and flicker-effects. Also Vesuvan Doppelganger adheres to the ruling text after the conversion.

With the addition of the Clone AF, I have been able to add the following card that can become copies
Cemetery Puca
Dimir Doppelganger
Sakashima's Student
Shapeshifter's Marrow
Unstable Shapeshifter
Vesuva

Here are the existing issues with copied cards
1) Copies of tokens cause a crash when they leave play. The source of issue is how the LKI copy gets created.
2) Leave play triggers don't work correct for clones e.g. when a clone of Doomed Traveler leaves play, no Spirit token is produced. I think this is really with a problem with leave play triggers in general (see my post in the Bug Report thread about Turn to Frog and Doomed Traveler).
3) Copies of equipment made via Copy Artifact or Phyrexian Metamorph cannot be equipped. This has to do with the way eqPump is implemented. This is the source of the issue with copies of Living Weapons. The 0/0 Germ cannot be equipped so it dies when state effects are checked.
4) Copies of cards that setup Zone Change triggers via addComesIntoPlayCommand and addLeavesPlayCommand will not function correctly. This applies to most of the hard coded cards and any cards that use keywords processed by forge.card.cardfactory.CardFactoryUtil.parseKeywords.

I've looked into all the above issue and there is no quick and easy fix for any of them. The existing hard coded implementation has all of these same issues so should I go ahead and merge my branch back into the Trunk or wait till I address the above issues? I am inclined to merge since we'll be in a better state but I want to give the other developers a chance to give their opinion before I merge.

This sounds like a huge improvement. So i vote for a merge.

ArsenalNut wrote:3) Copies of equipment made via Copy Artifact or Phyrexian Metamorph cannot be equipped. This has to do with the way eqPump is implemented. This is the source of the issue with copies of Living Weapons. The 0/0 Germ cannot be equipped so it dies when state effects are checked.

Another reason to convert eqPump cards to AF Attach. It's only scripting work.

ArsenalNut wrote:1) Copies of tokens cause a crash when they leave play. The source of issue is how the LKI copy gets created.

This shouldn't be a problem that is really hard to fix or?

ArsenalNut wrote:2) Leave play triggers don't work correct for clones e.g. when a clone of Doomed Traveler leaves play, no Spirit token is produced. I think this is really with a problem with leave play triggers in general (see my post in the Bug Report thread about Turn to Frog and Doomed Traveler).

This is quiite a deep problem. I guess one day we have to split the changesZone triggers into entersZone (looks at the new object) and leavesZone (looks at the LKI copy).

ArsenalNut wrote:I've looked into all the above issue and there is no quick and easy fix for any of them. The existing hard coded implementation has all of these same issues so should I go ahead and merge my branch back into the Trunk or wait till I address the above issues? I am inclined to merge since we'll be in a better state but I want to give the other developers a chance to give their opinion before I merge.

 
I vote for merging the clone fix branch into the trunk.

I'm fine with merging too. As Sloth says, 3 should be fixed just by migrating eqPump (as we planned on doing anyway) and we can at least prevent the crash in 1 until we get that specific kink worked out.

friarsol wrote:I'm fine with merging too. As Sloth says, 3 should be fixed just by migrating eqPump (as we planned on doing anyway) and we can at least prevent the crash in 1 until we get that specific kink worked out.

I had similar thoughts about the eqPump problem. Is thought to change eqPump into a sorcery speed attach ability and use a continuous static effect to provide the equipped pump? I was going to test this to make it would solve the Living weapon problem.

The changes are merged back into the trunk. I used the Animate AF as a starting template so the AI methods are copies of whatever is in Animate. I noticed that the AI didn't seem to play Phantasmal Image. I suspect the AI methods are the culprit. The logic in the Copy AF is probably a good starting point. One of the these days I need to figure out how those AI methods work

Sloth wrote:

ArsenalNut wrote:1) Copies of tokens cause a crash when they leave play. The source of issue is how the LKI copy gets created.

This shouldn't be a problem that is really hard to fix or?

I want to try to get this fixed over the weekend. The crash is caused because the getLKICopy method tries to get a copy of the card from the the card database by name. Since the plain tokens aren't in the database, Forge throws an exception when it can't find it.

It seems like getting an LKI copy and making a clone should be very similar (obviously the LKI version needs to copy the current status as well). The big difference between what getLKIcopy does versus making clones is that getLKIcopy runs a template card through all the methods that parse keywords and handle hard coded cards. I assume it was done this way in order to make a deep copy of a card. Between what I did for clones and what Hellfish did for ability stealers, I can make a copy of everything but the zone change triggers. Does an LKI copy really need all of its abilities instantiated, in particular the zone change triggers?

ArsenalNut wrote:Does an LKI copy really need all of its abilities instantiated, in particular the zone change triggers?

Definitely not.

I rewrote the getLKICopy method and that fixed the issue with clones of tokens leaving the battlefield. Before I check in the change, I'd like to test that I haven't somehow broken the use of LKI. Any suggestions of scenarios to test?

ArsenalNut wrote:I rewrote the getLKICopy method and that fixed the issue with clones of tokens leaving the battlefield. Before I check in the change, I'd like to test that I haven't somehow broken the use of LKI. Any suggestions of scenarios to test?

Altar of Dementia with something like Spike Feeder.

Sloth wrote:

ArsenalNut wrote:3) Copies of equipment made via Copy Artifact or Phyrexian Metamorph cannot be equipped. This has to do with the way eqPump is implemented. This is the source of the issue with copies of Living Weapons. The 0/0 Germ cannot be equipped so it dies when state effects are checked.

Another reason to convert eqPump cards to AF Attach. It's only scripting work.

As a test, I converted Batterskull to the following script

Batterskull using attach | Open

Name:Batterskull
ManaCost:5
Types:Artifact Equipment
Text:no text
K:Living Weapon
S:Mode$ Continuous | Affected$ Creature.AttachedBy | AddPower$ 4 | AddToughness$ 4 | AddKeyword$ Vigilance & Lifelink | Description$ Equipped creature gets +4/+4 and has vigilance and lifelink.
A:AB$ ChangeZone | Cost$ 3 | Origin$ Battlefield | Destination$ Hand | SpellDescription$ Return CARDNAME to its owner's hand.
A:AB$ Attach | Cost$ 5 | ValidTgts$ Creature.YouCtrl | TgtPrompt$ Select target creature you control | SorcerySpeed$ True | AILogic$ Pump | SpellDescription$ Equip
SVar:Rarity:Mythic
SVar:Picture:http://www.wizards.com/global/images/magic/general/batterskull.jpg
SetInfo:NPH|Mythic|http://magiccards.info/scans/en/nph/130.jpg
Oracle:Living weapon (When this Equipment enters the battlefield, put a 0/0 black Germ creature token onto the battlefield, then attach this to it.)\nEquipped creature gets +4/+4 and has vigilance and lifelink.\n{3}: Return Batterskull to its owner's hand.\nEquip
End

Cloning this version works perfectly. I am willing to taken on the task of converting all the equipment but I can see three ways to approach it.
1) Just add a raw script like I did for test to every card
2) Create a keyword script macro for Equip similar to how things like Living Weapon are handled. The equipment bonus would still have to be added via a Continuous static effect.
3) Convert existing eqPump keyword to a script macro that creates both the attach ability and the Continuous static effect. I would probably rename eqPump to Equip.