Why does copyCard copy the UniqueNumber?

[Chris H.]

In message:

viewtopic.php?f=26&t=787&start=5235#p66537

I used Master Transmuter to bounce an artifact creature targeted by Excommunicate to my hand and returned it to play; however, Excommunicate still put it on top of my library (technically it was a different creature than the one the Excommunicate had targeted).

I can confirm this. Using Momentary Blink to save a creature from Vindicate doesn't work at the moment.

EDIT: Why does copyCard copy the UniqueNumber? Wouldn't this be fixed without copying it?

[Chris H.]

In message:

viewtopic.php?f=26&t=787&start=5235#p66554

. . . Why does copyCard copy the UniqueNumber? Wouldn't this be fixed without copying it?

i am not particularly happy with the way Forge uses the term Card. also, there are too many ways to "copy" a Card. if someone could get a good grasp of these things, i think it would help the other developers.

[Chris H.]

In message:

viewtopic.php?f=26&t=787&start=5250#p66566

. . . Why does copyCard copy the UniqueNumber? Wouldn't this be fixed without copying it?

i am not particularly happy with the way Forge uses the term Card. also, there are too many ways to "copy" a Card. if someone could get a good grasp of these things, i think it would help the other developers.

I agree. We should at least split:
1. The card fully defined by name.
2. The card in a collection (with set and image info).
3. The card in a game.
I think using three classes that each "extend" the former would be helpful. (Maybe these posts should be moved to the developer corner).

[Sloth]

Thanks for moving this Chris.

Maybe we can start with an answer for the headline.

[Chris H.]

You are welcome Sloth. For this topic I took the easy way out and did a quick copy and paste.

It looks like I can move topics far easier than individual messages. I might explore this in more detail at some point.

[friarsol]

I don't think we necessarily need to copy over the UniqueNumber from copyCard. The moveTo() functions are returning the new card, so as long as whatever calls moveTo() uses the returned Card for any further calculation/manipulation we should be fine.

I just cleaned up some unnecessary member variables in Card. I've also made a list of how we could split up the class, along with some commentary:

CardData | Open

private Map<String, Object> triggeringObjects = new TreeMap<String, Object>();
private ArrayList<Trigger> triggers = new ArrayList<Trigger>();

private ArrayList<String> intrinsicAbility = new ArrayList<String>();
// (SOL: I don't think we need Static Ability Strings AND StaticAbilities, just the Abilities should be fine)
private ArrayList<String> staticAbilityStrings = new ArrayList<String>();
private ArrayList<String> intrinsicKeyword = new ArrayList<String>();

private ArrayList<String> type = new ArrayList<String>();

// (SOL: I don't think ManaAbilities necessarily need their own ArrayList)
private ArrayList<SpellAbility> spellAbility = new ArrayList<SpellAbility>();
private ArrayList<Ability_Mana> manaAbility = new ArrayList<Ability_Mana>();

private ArrayList<StaticAbility> staticAbilities = new ArrayList<StaticAbility>();

// (SOL: CanPlay() Should handle along with StaticAbilities that effect players
private boolean unCastable;

private boolean levelUp = false;

// (SOL: Flashback and Unearth can be converted to have an ActivatingZone probably)
private boolean flashback = false;
private boolean unearth = false;

private boolean madness = false;
private boolean suspend = false;

// (SOL: Why do we have both baseAttack and baseAttack String? Can everything just use Card_PT now?)
private int baseAttack = 0;
private int baseDefense = 0;
private ArrayList<Card_PT> newPT = new ArrayList<Card_PT>(); // stack of set power/toughness
private int baseLoyalty = 0;
private String baseAttackString = null;
private String baseDefenseString = null;

// (SOL: owner/controller are game specific, but might cause issues if not available here)
private Player owner = null;
private Player controller = null;

private String name = "";
private String imageName = "";
private String rarity = "";
private String text = "";
private String manaCost = "";

// (SOL: More things I'd love to see moved somewhere else)
private String echoCost = "";
private String madnessCost = "";

private Map<String, String> SVars = new TreeMap<String, String>();

private String ImageFilename = "";

private ArrayList<Ability_Triggered> zcTriggers = new ArrayList<Ability_Triggered>();

CardCollectionData | Open

private long value;

private ArrayList<SetInfo> Sets = new ArrayList<SetInfo>();
private String curSetCode = "";

CardGameData | Open

private long timestamp = -1; // permanents on the battlefield
private static int nextUniqueNumber;
private int uniqueNumber = nextUniqueNumber++;

private ArrayList<String> extrinsicKeyword = new ArrayList<String>();
private ArrayList<String> HiddenExtrinsicKeyword = new ArrayList<String>(); //Hidden keywords won't be displayed on the card
private ArrayList<String> prevIntrinsicKeyword = new ArrayList<String>();

// (Note: equipping and enchanting should just be Card not ArrayList<Card>
private ArrayList<Card> attached = new ArrayList<Card>();
private ArrayList<Card> equippedBy = new ArrayList<Card>(); //which equipment cards are equipping this card?
//equipping size will always be 0 or 1
private ArrayList<Card> equipping = new ArrayList<Card>(); //if this card is of the type equipment, what card is it currently equipping?
private ArrayList<Card> enchantedBy = new ArrayList<Card>(); //which auras enchanted this card?
//enchanting size will always be 0 or 1
private ArrayList<Card> enchanting = new ArrayList<Card>(); //if this card is an Aura, what card is it enchanting?

private ArrayList<String> prevType = new ArrayList<String>();
private ArrayList<String> ChoicesMade = new ArrayList<String>();

// (SOL: This seems to be used with Modal spells, but I'm not sure why it's a List of strings)
private ArrayList<String> Targets_for_Choices = new ArrayList<String>();

// (SOL: I wonder if we should combine these "tracking" variables into a Map of Lists)
private ArrayList<Object> rememberedObjects = new ArrayList<Object>();
private ArrayList<Card> imprintedCards = new ArrayList<Card>();
private Card championedCard = null;
private CardList devouredCards = new CardList();

private Map<Card, Integer> receivedDamageFromThisTurn = new TreeMap<Card, Integer>();
private Map<Card, Integer> dealtDamageToThisTurn = new TreeMap<Card, Integer>();
private Map<Card, Integer> assignedDamageMap = new TreeMap<Card, Integer>();

private boolean drawnThisTurn = false;
private boolean tapped;
private boolean sickness = true; //summoning sickness
private boolean token = false;
private boolean copiedToken = false;
private boolean copiedSpell = false;

private boolean creatureAttackedThisTurn = false;
private boolean creatureAttackedThisCombat = false;
private boolean creatureBlockedThisCombat = false;
private boolean creatureGotBlockedThisCombat = false;
private boolean dealtDmgToHumanThisTurn = false;
private boolean dealtDmgToComputerThisTurn = false;

//(SOL: Siren's Call should be able to convert to an Effect)
private boolean sirenAttackOrDestroy = false;

private boolean faceDown = false;
private boolean kicked = false;
private boolean evoked = false;

private boolean bounceAtUntap = false;
private boolean finishedEnteringBF = false;

private boolean unearthed;
private boolean suspendCast = false;

private boolean isImmutable = false;

private int damage;

//(SOL: This really should be regenerationShield)
private int nShield; // regeneration
private int preventNextDamage = 0;

private int turnInZone;

private int tempAttackBoost = 0;
private int tempDefenseBoost = 0;

private int semiPermanentAttackBoost = 0;
private int semiPermanentDefenseBoost = 0;

private int randomPicture = 0;

private int xManaCostPaid = 0;

private int xLifePaid = 0;

private int multiKickerMagnitude = 0;
private int replicateMagnitude = 0;

// (SOL: There's a better/more robust way to handle this situation)
private int sunburstValue = 0;
private String colorsPaid = "";

private String chosenType = "";
private String chosenColor = "";
private String namedCard = "";

private Card cloneOrigin = null;
private ArrayList<Card> clones = new ArrayList<Card>();
private Card currentlyCloningCard = null;
private Command cloneLeavesPlayCommand = null;
private ArrayList<Card> gainControlTargets = new ArrayList<Card>();
private ArrayList<Command> gainControlReleaseCommands = new ArrayList<Command>();

// (SOL: Not sure where these Commands are created.)
private ArrayList<Command> turnFaceUpCommandList = new ArrayList<Command>();
private ArrayList<Command> equipCommandList = new ArrayList<Command>();
private ArrayList<Command> unEquipCommandList = new ArrayList<Command>();
private ArrayList<Command> enchantCommandList = new ArrayList<Command>();
private ArrayList<Command> unEnchantCommandList = new ArrayList<Command>();
private ArrayList<Command> untapCommandList = new ArrayList<Command>();
private ArrayList<Command> changeControllerCommandList = new ArrayList<Command>();
private ArrayList<Command> replaceMoveToGraveyardCommandList = new ArrayList<Command>();
private ArrayList<Command> cycleCommandList = new ArrayList<Command>();

private Map<Counters, Integer> counters = new TreeMap<Counters, Integer>();

// (SOL: Outdated I'm going to remove this in a little bit)
private int abilityUsed;

[Sloth]

I don't think we necessarily need to copy over the UniqueNumber from copyCard. The moveTo() functions are returning the new card, so as long as whatever calls moveTo() uses the returned Card for any further calculation/manipulation we should be fine.

My concern is that some triggers might be unable to find the triggered card. But I'm not an expert of trigger logic.

I just cleaned up some unnecessary member variables in Card.

I've also removed quite a few unnecessary variables and functions from the card class (the most funniest one was storing the name of the top card of the library ). A lot of chaff was added to make a specific card work and without having a general concept.
Good work removing some more.

I've also made a list of how we could split up the class, along with some commentary:

CardData | Open

private Map<String, Object> triggeringObjects = new TreeMap<String, Object>();
private ArrayList<Trigger> triggers = new ArrayList<Trigger>();

private ArrayList<String> intrinsicAbility = new ArrayList<String>();
// (SOL: I don't think we need Static Ability Strings AND StaticAbilities, just the Abilities should be fine)
private ArrayList<String> staticAbilityStrings = new ArrayList<String>();
private ArrayList<String> intrinsicKeyword = new ArrayList<String>();

private ArrayList<String> type = new ArrayList<String>();

// (SOL: I don't think ManaAbilities necessarily need their own ArrayList)
private ArrayList<SpellAbility> spellAbility = new ArrayList<SpellAbility>();
private ArrayList<Ability_Mana> manaAbility = new ArrayList<Ability_Mana>();

private ArrayList<StaticAbility> staticAbilities = new ArrayList<StaticAbility>();

// (SOL: CanPlay() Should handle along with StaticAbilities that effect players
private boolean unCastable;

private boolean levelUp = false;

// (SOL: Flashback and Unearth can be converted to have an ActivatingZone probably)
private boolean flashback = false;
private boolean unearth = false;

private boolean madness = false;
private boolean suspend = false;

// (SOL: Why do we have both baseAttack and baseAttack String? Can everything just use Card_PT now?)
private int baseAttack = 0;
private int baseDefense = 0;
private ArrayList<Card_PT> newPT = new ArrayList<Card_PT>(); // stack of set power/toughness
private int baseLoyalty = 0;
private String baseAttackString = null;
private String baseDefenseString = null;

// (SOL: owner/controller are game specific, but might cause issues if not available here)
private Player owner = null;
private Player controller = null;

private String name = "";
private String imageName = "";
private String rarity = "";
private String text = "";
private String manaCost = "";

// (SOL: More things I'd love to see moved somewhere else)
private String echoCost = "";
private String madnessCost = "";

private Map<String, String> SVars = new TreeMap<String, String>();

private String ImageFilename = "";

private ArrayList<Ability_Triggered> zcTriggers = new ArrayList<Ability_Triggered>();

CardCollectionData | Open

private long value;

private ArrayList<SetInfo> Sets = new ArrayList<SetInfo>();
private String curSetCode = "";

CardGameData | Open

private long timestamp = -1; // permanents on the battlefield
private static int nextUniqueNumber;
private int uniqueNumber = nextUniqueNumber++;

private ArrayList<String> extrinsicKeyword = new ArrayList<String>();
private ArrayList<String> HiddenExtrinsicKeyword = new ArrayList<String>(); //Hidden keywords won't be displayed on the card
private ArrayList<String> prevIntrinsicKeyword = new ArrayList<String>();

// (Note: equipping and enchanting should just be Card not ArrayList<Card>
private ArrayList<Card> attached = new ArrayList<Card>();
private ArrayList<Card> equippedBy = new ArrayList<Card>(); //which equipment cards are equipping this card?
//equipping size will always be 0 or 1
private ArrayList<Card> equipping = new ArrayList<Card>(); //if this card is of the type equipment, what card is it currently equipping?
private ArrayList<Card> enchantedBy = new ArrayList<Card>(); //which auras enchanted this card?
//enchanting size will always be 0 or 1
private ArrayList<Card> enchanting = new ArrayList<Card>(); //if this card is an Aura, what card is it enchanting?

private ArrayList<String> prevType = new ArrayList<String>();
private ArrayList<String> ChoicesMade = new ArrayList<String>();

// (SOL: This seems to be used with Modal spells, but I'm not sure why it's a List of strings)
private ArrayList<String> Targets_for_Choices = new ArrayList<String>();

// (SOL: I wonder if we should combine these "tracking" variables into a Map of Lists)
private ArrayList<Object> rememberedObjects = new ArrayList<Object>();
private ArrayList<Card> imprintedCards = new ArrayList<Card>();
private Card championedCard = null;
private CardList devouredCards = new CardList();

private Map<Card, Integer> receivedDamageFromThisTurn = new TreeMap<Card, Integer>();
private Map<Card, Integer> dealtDamageToThisTurn = new TreeMap<Card, Integer>();
private Map<Card, Integer> assignedDamageMap = new TreeMap<Card, Integer>();

private boolean drawnThisTurn = false;
private boolean tapped;
private boolean sickness = true; //summoning sickness
private boolean token = false;
private boolean copiedToken = false;
private boolean copiedSpell = false;

private boolean creatureAttackedThisTurn = false;
private boolean creatureAttackedThisCombat = false;
private boolean creatureBlockedThisCombat = false;
private boolean creatureGotBlockedThisCombat = false;
private boolean dealtDmgToHumanThisTurn = false;
private boolean dealtDmgToComputerThisTurn = false;

//(SOL: Siren's Call should be able to convert to an Effect)
private boolean sirenAttackOrDestroy = false;

private boolean faceDown = false;
private boolean kicked = false;
private boolean evoked = false;

private boolean bounceAtUntap = false;
private boolean finishedEnteringBF = false;

private boolean unearthed;
private boolean suspendCast = false;

private boolean isImmutable = false;

private int damage;

//(SOL: This really should be regenerationShield)
private int nShield; // regeneration
private int preventNextDamage = 0;

private int turnInZone;

private int tempAttackBoost = 0;
private int tempDefenseBoost = 0;

private int semiPermanentAttackBoost = 0;
private int semiPermanentDefenseBoost = 0;

private int randomPicture = 0;

private int xManaCostPaid = 0;

private int xLifePaid = 0;

private int multiKickerMagnitude = 0;
private int replicateMagnitude = 0;

// (SOL: There's a better/more robust way to handle this situation)
private int sunburstValue = 0;
private String colorsPaid = "";

private String chosenType = "";
private String chosenColor = "";
private String namedCard = "";

private Card cloneOrigin = null;
private ArrayList<Card> clones = new ArrayList<Card>();
private Card currentlyCloningCard = null;
private Command cloneLeavesPlayCommand = null;
private ArrayList<Card> gainControlTargets = new ArrayList<Card>();
private ArrayList<Command> gainControlReleaseCommands = new ArrayList<Command>();

// (SOL: Not sure where these Commands are created.)
private ArrayList<Command> turnFaceUpCommandList = new ArrayList<Command>();
private ArrayList<Command> equipCommandList = new ArrayList<Command>();
private ArrayList<Command> unEquipCommandList = new ArrayList<Command>();
private ArrayList<Command> enchantCommandList = new ArrayList<Command>();
private ArrayList<Command> unEnchantCommandList = new ArrayList<Command>();
private ArrayList<Command> untapCommandList = new ArrayList<Command>();
private ArrayList<Command> changeControllerCommandList = new ArrayList<Command>();
private ArrayList<Command> replaceMoveToGraveyardCommandList = new ArrayList<Command>();
private ArrayList<Command> cycleCommandList = new ArrayList<Command>();

private Map<Counters, Integer> counters = new TreeMap<Counters, Integer>();

// (SOL: Outdated I'm going to remove this in a little bit)
private int abilityUsed;

I think imageName can be moved to CardCollectionData.

[Hellfish]

My concern is that some triggers might be unable to find the triggered card. But I'm not an expert of trigger logic.

Trigger logic should be pretty much unaffected as long as Card.IsValid(...) is. That's the only place I can think of in trigger code that uses card IDs.

[Sloth]

As I expected removing the copying of the UniqueNumber will cause a ton of problems. Moving cards to the stack in addAndUnfreeze does throw a NPE for targeted spells and Remembered doesn't work any more.

I guess the whole structure is not ready for this.

[friarsol]

As I expected removing the copying of the UniqueNumber will cause a ton of problems. Moving cards to the stack in addAndUnfreeze does throw a NPE for targeted spells and Remembered doesn't work any more.

I guess the whole structure is not ready for this.

I should have some time today to tweak at this. I have some ideas about how it might work without overhauling the whole structure.

[friarsol]

I should have some time today to tweak at this. I have some ideas about how it might work without overhauling the whole structure.

My simple solution for this didn't work the way I was hoping.

[mtgrares]

Conceptually I thought of a card's unique number equals one cardboard physical card but with Momentary Blink + Vindicate this clearly isn't true. A physical card "changes" each time it is played. One possible suggestion is that when a card is put onto the battlefield is gets a new unique card number, that way the card's unique number stays the same when the card is in hand and on the stack. This still might cause a some errors.

Just my 2 cents.

[Braids]

Conceptually I thought of a card's unique number equals one cardboard physical card but with Momentary Blink + Vindicate this clearly isn't true. A physical card "changes" each time it is played. One possible suggestion is that when a card is put onto the battlefield is gets a new unique card number, that way the card's unique number stays the same when the card is in hand and on the stack. This still might cause a some errors.

Just my 2 cents.

is this because Vindicate targets a permanent, and Momentary Blink causes the card to leave play and come back as a separate permanent, causing Vindicate to fizzle?

[Sloth]

is this because Vindicate targets a permanent, and Momentary Blink causes the card to leave play and come back as a separate permanent, causing Vindicate to fizzle?

This is what should be happening, but does not.

Conceptually I thought of a card's unique number equals one cardboard physical card but with Momentary Blink + Vindicate this clearly isn't true. A physical card "changes" each time it is played. One possible suggestion is that when a card is put onto the battlefield is gets a new unique card number, that way the card's unique number stays the same when the card is in hand and on the stack. This still might cause a some errors.

Just my 2 cents.

This sounds like it will work as a temporary fix.

[Braids]

is this because Vindicate targets a permanent, and Momentary Blink causes the card to leave play and come back as a separate permanent, causing Vindicate to fizzle?

This is what should be happening, but does not.

Conceptually I thought of a card's unique number equals one cardboard physical card but with Momentary Blink + Vindicate this clearly isn't true. A physical card "changes" each time it is played. One possible suggestion is that when a card is put onto the battlefield is gets a new unique card number, that way the card's unique number stays the same when the card is in hand and on the stack. This still might cause a some errors.

Just my 2 cents.

This sounds like it will work as a temporary fix.

i would tend to think that a card on the battlefield should be some sort of Permanent instance, with a reference to the Card that it embodies. tokens would have no such reference.