Converting cards from Keyword to AbilityFactory

[Chris H.]

I have converted all of the spBounceTgt spell over to AF_Bounce except for one, Capsize. Capsize has a Buyback SVar. The buyback code is fairly short and simple but I am unsure of where to add it to the existing AF_Bounce code. It would be great if someone who is more familiar with the new AF code could add in the buyback code.

For testing purposes, this is what Capsize would look like:

Code: Select all

Name:Capsize
ManaCost:1 U U
Types:Instant
Text:no text
A:SP$Bounce|Cost$1 U U|ValidTgts$Permanent|TgtPrompt$Select target permanent|Destination$Hand|SpellDescription$Return target permanent to its owner's hand.
SVar:Buyback:3
SVar:RemAIDeck:True
SVar:PlayMain1:TRUE
SVar:Rarity:Common
SVar:Picture:http://www.wizards.com/global/images/magic/general/capsize.jpg
End

[Chris H.]

With the cards that I converted yesterday it came to me that the string of data that is parsed with the new AbilityFactory is very hard to read.

Would it make sense to add a few spaces here and there to make them more readable? The .trim() command can be used to remove the leading and trailing spaces.

I think that it may be helpful to replace the separator "|" with " | " and to separate the paramName$paramValue by replacing the "$" with a "$ ".

With these adjustments, the AF_Bounce Ability for Capsize would look like this:

Code: Select all

A:SP$Bounce | Cost$ 1 U U | ValidTgts$ Permanent | TgtPrompt$ Select target permanent | Destination$ Hand | SpellDescription$ Return target permanent to its owner's hand.

[Hellfish]

Having spaces there looks nice, but I'm not sure it should be enforced. So I'm in favor of the trim() approach. Can't help you with the buyback, sorry.

[Sloth]

I haven't tested it, but here is Mind Games (added by Sol I think):

Code: Select all

Name:Mind Games
ManaCost:U
Types:Instant
Text:no text
A:SP$Tap|Cost$U|TgtPrompt$Choose target artifact, creature or land|ValidTgts$Artifact,Creature,Land|SpellDescription$Tap target artifact, creature or land.
SVar:Buyback:2 U
SVar:RemAIDeck:True
SVar:Rarity:Common
SVar:Picture:http://www.wizards.com/global/images/magic/general/mind_games.jpg
End


[friarsol]

Yep I'm not sure if I did Mind Games or not, but Buyback was already implemented in the necessary spot for this. I just took your line exactly as you had it and it worked. Just like Magic!

[Chris H.]

I converted Capsize to AF_Bounce and ran a test. I get to choose between the two abilities, but the cost for both is the buyback cost.

The non-buyback ability resolves with the Capsize card going to the graveyard. The buyback ability resolves with the Capsize card going back to my hand.

So it looks like both abilities are given the same cost.

[friarsol]

Ahh.. that looks like a Pointer problem in sa.copy(); Since the Ability_Cost is the same for each SA when one is overridden the other is as well. Nice catch.

I can't test this today, but basically what needs to happen is a new Ability_Cost needs to be created in the buyback area.

in CardFactory.java around line 6871 this section needs this small change

Code: Select all

String bbCost = card.getSVar("Buyback");
if (!bbCost.equals(""))
{
   SpellAbility bbSA = sa.copy();
   String newCost = CardUtil.addManaCosts(card.getManaCost(), bbCost);
   if (bbSA.payCosts != null)
      bbSA.payCosts = new Ability_Cost(newCost, sa.getSourceCard().getName(), false); // create new abCost
   StringBuilder sb = new StringBuilder();
   sb.append("Buyback ").append(bbCost).append(" (You may pay an additional ").append(bbCost);
   sb.append(" as you cast this spell. If you do, put this card into your hand as it resolves.)");
   bbSA.setDescription(sb.toString());
   bbSA.setIsBuyBackAbility(true);
                  
   card.addSpellAbility(bbSA);
}


.

Currently Buyback assumes the cost is mana only. I can look into a way to add two Ability_Costs together to create a new Ability_Cost soon. And then the few Buyback cards that have non-mana payments (like Forbid) can be coded up as well.

[Chris H.]

Thank you, Sol. I tested the code and it works. I merged this into the SVN and converted Capsize. I will now go back and comment out spBounceTgt.

[slapshot5]

Having spaces there looks nice, but I'm not sure it should be enforced. So I'm in favor of the trim() approach.

agreed. Spaces could be allowed anywhere, so:

|Destination$Hand|
| Destination$Hand|
| Destination $Hand|
| Destination $ Hand|
| Destination $ Hand |

would all be equally valid.

-slapshot5

[silly freak]

Code: Select all

replaceAll(" ", "")


could be used to get rig of the whitespace right in the beginning, but that would handle "Des tination" just the same. another way would be to replace

Code: Select all

split("$")
by
split(" *$ *")


which treats the whitespace as part of the separator

[Chris H.]

OK, I have spent some time peeking at the code. I found one place that looked promising, and that is ReadCard.run() which has:

Code: Select all

else if (s.startsWith("A:"))
{
    String t = s.substring(2);
    c.addIntrinsicAbility(t);
}

`
So I added some code and made it look like this:

Code: Select all

else if (s.startsWith("A:"))
{
    String t = s.substring(2);
   
    String check[] = { " |", "| " };
    for (int j = 0; j < check.length; j ++) {
        while (t.contains(check[j])) {
            t = t.replaceAll(check[j], "|");
        }
    }
    c.addIntrinsicAbility(t);
}

`
I built and ran the project without changing any of the cards and forge threw up an error exception as it was starting. We have 20 cards currently that can not handle the removing of the leading/trailing spaces at the separator. These cards are:

active_volcano.txt
chainflinger.txt
chaos_charm.txt
consuming_bonfire.txt
Crosiss_charm.txt
darigaazs_charm.txt
evolution_charm.txt
feast_or_famine.txt
flame_jab.txt
flash_flood.txt
infernal_tribute.txt
keldon_megaliths.txt
krosan_restorer.txt
kuldotha_phoenix.txt
monstrify.txt
natural_order.txt
nightscape_master.txt
shinen_of_fears_chill.txt
sunscape_master.txt
syphon_life.txt

`
So, there must be some spots in the new AF code with checks that need the leading/trailing spaces at the separator. I guess that these tests could be changed to deal with this but I have not had a chance to examine this situation in more detail.

[Sloth]

Actually these cards are the only ones that have " |" or "| " in their abilities, but replacing them with "|" should do no harm. I guess there is another regex problem causing replaceAll replacing unappropriate stuff.

[Hellfish]

Thinking deeper about it, replacing spaces like this will mess up PreCostDesc$ parameters such as "Metalcraft - ","Channel - " and the like.It shouldn't throw exception hissy fits but it will look weird.

[Chris H.]

16 cards have PrecostDesc$

2 cards have CostDesc$

Which leaves Nightscape Master and Sunscape Master. These two cards have a space following the tap "T" as part of their Cost$.

[Chris H.]

Actually these cards are the only ones that have " |" or "| " in their abilities, but replacing them with "|" should do no harm. I guess there is another regex problem causing replaceAll replacing unappropriate stuff.

`
Sloth was right. It turns out that it would be best to do the character replacement at AbilityFactory.getAbility(). A simple for loop can then use the simpler trim command.

Code: Select all

   public SpellAbility getAbility(String abString, Card hostCard){
      
      SpellAbility SA = null;
      
      hostC = hostCard;
      
      if (!(abString.length() > 0))
         throw new RuntimeException("AbilityFactory : getAbility -- abString too short in " + hostCard.getName());
      
      String a[] = abString.split("\\|");
      
      for (int aCnt = 0; aCnt < a.length; aCnt ++)
         a[aCnt] = a[aCnt].trim();
      
      if (!(a.length > 1))
         throw new RuntimeException("AbilityFactory : getAbility -- a[] too short in " + hostCard.getName());
         
      for (int i=0; i<a.length; i++)
      {
         String aa[] = a[i].split("\\$");
         
         for (int aaCnt = 0; aaCnt < aa.length; aaCnt ++)
            aa[aaCnt] = aa[aaCnt].trim();
         
         if (!(aa.length == 2))
            throw new RuntimeException("AbilityFactory : getAbility -- aa.length not 2 in " + hostCard.getName());
         
         mapParams.put(aa[0], aa[1]);
      }

`
Tested and it works OK. We may need to append a space to the precost and cost descriptions, but that should not be too difficult.