Color Identity by Hellfish

[Max mtg]

There is a commit from Hellfish (r20671) that introduces color identities of cards.

I heartly welcome your next step towards EDH, but have to argue about it's implementation.

Code: Select all

                else if ("ColorIdentity".equals(key)) {
                    // This is forge.card.CardColor not forge.CardColor.
                    // Why do we have two classes with the same name?
                    ColorSet newCol = ColorSet.fromNames(value.split(","));
                    this.faces[this.curFace].setColorIdentity(newCol);
                }

This code is intended to read identity directly from cards, but I can't figure out why cannot this be calculated?

Since "color identity of a card is specified by all mana symbols that appear on the card anywhere, including within its rules text" it's easy to determine it.
We already have card's casting cost, that's one set of mana symbols. The remaining mana symbols can be parsed from oracle text, where all mana symbols are enclosed in curly braces. The only thing to do is write that parser, take StringTokenizer get that enclosed fragments and for each when it's not number feed it to ManaCostShard.parseNonGeneric() method.

Why not precalculate and store color identity in card scripts?
- Harder to mantain scripts (will have to add identity to newly created cards)
- Have to develop a tool to calculate anyway, that will mean parsing some external data sources, which can be late from the date the set is avaliable and Forge ready to play with it's cards.
- Excessive or duplicate data was never good.

Will you, Hellfish, please write a routine to calculate card's color identity?

PS: Answering the question in comments - forge.CardColor uses strings array to identify colors and has to be replaced with that newer other class, however that replace still was not made because the former class contains a timestamp. I don't think it's useful to create separate classes for color+timestamp, type+timestamp, but instead create a class TimeStamped<T> to store a number of values coupled with their timestamp.

[Max mtg]

And speaking of data incapsulation, colorIdentity is sooner a property of CardRules, than of CardFace.
That is to say the identity has to be calculated in CardPrinted ctor or on first access, and be removed from CardFace and ICardCharacteristics.

[Hellfish]

I had a reply here about how the color identity field was only required for a select few cards before I realised other cards would need a corrected identity as well... D'oh! :S I'll take a look at doing it better soon.

[Max mtg]

r20706 looks amazing.
May I commit a different revision of method calculateColorIdentity?

Code: Select all

    private byte calculateColorIdentity(ICardFace face)
    {
        byte res = face.getManaCost() == null ? 0 : face.getManaCost().getColorProfile();
        boolean isReminder = false;
        boolean isSymbol = false;
        String oracleText = face.getOracleText();
        int len = oracleText.length();
        for(int i = 0; i < len; i++) {
            char c = oracleText.charAt(i); // This is to avoid needless allocations performed by toCharArray()
            switch(c) {
                case('('): isReminder = true; break;
                case(')'): isReminder = false; break;
                case('{'): isSymbol = true; break;
                case('}'): isSymbol = false; break;
                default:
                    if(isSymbol && !isReminder) {
                        switch(c) {
                            case('W'): res |= MagicColor.WHITE; break;
                            case('U'): res |= MagicColor.BLUE; break;
                            case('B'): res |= MagicColor.BLACK; break;
                            case('R'): res |= MagicColor.RED; break;
                            case('G'): res |= MagicColor.GREEN; break;
                        }
                    }
                    break;
            }
        }
        return res;
    }


[Hellfish]

Sure. Improve away.

[Max mtg]

Sure. Improve away.

You haven't left much to improve.

That is the only thing I wanted to change.

[Hellfish]

Found a little bug, actually, but that should be it. Thanks.

[Max mtg]

Found a little bug, actually, but that should be it. Thanks.

With that fulfilled, decks can be checked to match commander's profile.

Remaining features:
* add a special attribute on card which acts as a general.
* accumulation of damage dealt to players by generals, check it as a lose condition.
* add a replacement trigger on commander when game starts
* cast commander from command zone and count number of times it was cast to adjust the price by

Am I right about that?

[friarsol]

I believe also:

"A deck may not generate mana outside its colours. If an effect would generate mana of an illegal colour, it generates colourless mana instead."

Does the implementation of color identity conform to this rule?
"A card's colour identity is its colour plus the colour of any mana symbols in the card's rules text. A card's colour identity is established before the game begins, and cannot be changed by game effects."

Do we already have a banned list setup for Commander?

[Hellfish]

Color Identity is calculated when loading the scripts so the bold bit is fine. The other bits are not implemented yet though.

[Max mtg]

Sol, identity won't change as long as CardRules remains immutable.
Hellfish, I could not help implementing that small feature about 21 general damage as GL reason - because it's easy and gives lots of fun - sorry!

[friarsol]

Hellfish, I could not help implementing that small feature about 21 general damage as GL reason - because it's easy and gives lots of fun - sorry!

The EDH Damage implementation is incorrect in two ways:
- Needs to be combat damage
- It's 21 damage by a particular Commander, not total from all Commanders.

"If a player has been dealt 21 points of combat damage by a particular Commander during the game, that player loses a game"

[Max mtg]

Uh.. right. I have reverted the variable added to player class, there has to be a map of player(cmdr ctrl'er)->int(damage dealt) instead and only combat damage taken into consideration.
The rest like GL reason and player outcome still can be used.

[Max mtg]

I saw you've copied the InputMulligan class.
I wrote this not to say that copy-paste is a bad practice (you know that it is), but at the moment InputMulligan contains some logic that is to be execute in pooled thread, not EDT, and should be moved outside of mulligan class - that is everything related to leylines and chancellors

[Max mtg]

Oh, I almost forgot: Thanks for your color identity code!

The colored deck generation looks much better when backed by the color identity you've implemented!