AbilityFactory refactoring

[friarsol]

Not sure what's going on with Charm, but it seems to be incorrect, since you need to choose the Mode before doing any other prespell stuff (choosing targets, paying costs) currently, you choose the mode on resolution, which doesn't make sense since no targets are chosen or anything.

[Max mtg]

Not sure what's going on with Charm, but it seems to be incorrect, since you need to choose the Mode before doing any other prespell stuff (choosing targets, paying costs) currently, you choose the mode on resolution, which doesn't make sense since no targets are chosen or anything.

I know - have made choices on resolve, but according to rules one should choose when spell is cast.
Will take care right now... done. r17870

Some problems with stack descriptions remain.
We have StackDescription, SpellDescription and the own description and effect may provide. What will a common rule be? The rule to choose which text to display.

[friarsol]

We have StackDescription, SpellDescription and the own description and effect may provide. What will a common rule be? The rule to choose which text to display.

SpellDescription is what is used when the spell is in your hand. StackDescription tries to append useful Stack relevant information (Targets, etc) and is displayed in the stack panel. Plain old description I believe is the same as SpellDescription, just with an overwritable getter. And I'd imagine the effect description should be trying to "overwrite" the StackDescription, since that's when it's relevant.

[Sloth]

Just fyi:
I've moved all the drawback checks of the ai functions to SpellAiLogic. I will see what else can be generalized to reduce duplicate code.

[Max mtg]

Just fyi:
I've moved all the drawback checks of the ai functions to SpellAiLogic. I will see what else can be generalized to reduce duplicate code.

That's great, yet there are a few things to add:
- the wrapped canPlay no longer needs to be public.
- it also has to be abstract to prevent empty descendants.

I've already commited these changes.

[Max mtg]

Just fyi:
I've moved all the drawback checks of the ai functions to SpellAiLogic. I will see what else can be generalized to reduce duplicate code.

you may also remove

Code: Select all

    public boolean chkAIDrawback(final Map<String, String> params, final SpellAbility sa, final Player aiPlayer) {
        return true;
    }

from inheritors - whenever overriding method is just the same as parent's one.

[Max mtg]

ChangeZone converted.

The process is finished.

[Max mtg]

There is a harcoded card of Wing Puncture - if anyone converts it into script, there will remain no other descendants of AbilitySub than the CommonDrawback class!

Could anybody do it?

[Sloth]

There is a harcoded card of Wing Puncture - if anyone converts it into script, there will remain no other descendants of AbilitySub than the CommonDrawback class!

Could anybody do it?

I will do it.

EDIT: Done.

[Max mtg]

There is a harcoded card of Wing Puncture - if anyone converts it into script, there will remain no other descendants of AbilitySub than the CommonDrawback class!

Could anybody do it?

I will do it.

EDIT: Done.

Great! Thank you.

Now it's possible to get rid of AbilitySub class and replace it with CommonDrawback - yet this needs some evaluation.

Actually AbilitySubs form a double-linked list of abilities. At least that double-linked list functionality might be performed by some standard container... or not?

getCopy might be replaced with clone() for sure, making Spellability implement Cloneable


What I also would love to reach here is to remove a reference to factory from ability classes - so that factory products become independent.

[Sloth]

What I also would love to reach here is to remove a reference to factory from ability classes - so that factory products become independent.

The only use of backtracking from the ability to the factory is that AI functions can easily evaluate what a spell on a stack is doing. Of course only the api and the parameters are necessary, so maybe this could be handled in another way.

[Max mtg]

What I also would love to reach here is to remove a reference to factory from ability classes - so that factory products become independent.

The only use of backtracking from the ability to the factory is that AI functions can easily evaluate what a spell on a stack is doing. Of course only the api and the parameters are necessary, so maybe this could be handled in another way.

That's awesome.
Let then SA hold only api and paramters, and both are readonly fields I guess. So that the main AF class may become singleton one day.

I guess the reference to AF in SA can be replaced with these 3 methods.

Code: Select all

    public String getParam(String key) {
        return params == null ? null : params.get(key);
    }
    public boolean hasParam(String key) {
        return params == null ? false: params.containsKey(key);
    }

    // If this is not null, then ability was made in a factory
    public final ApiType getApi() {
        return api;
    }

Similiarly, all the methods of AF-Effects and AF-AIs no longer need a Map<String,String> params as an argument, because SA has all that data incapsulated

[Max mtg]

Well... noone stopped me, so it's already there, in 17906.

[Sloth]

Let then SA hold only api and paramters, and both are readonly fields I guess. So that the main AF class may become singleton one day.

I guess the reference to AF in SA can be replaced with these 3 methods.

Code: Select all

    public String getParam(String key) {
        return params == null ? null : params.get(key);
    }
    public boolean hasParam(String key) {
        return params == null ? false: params.containsKey(key);
    }

    // If this is not null, then ability was made in a factory
    public final ApiType getApi() {
        return api;
    }

Similiarly, all the methods of AF-Effects and AF-AIs no longer need a Map<String,String> params as an argument, because SA has all that data incapsulated

I think this way a very good change. Well done Max.

Now that the spellability itself holds the parameters, we are one step closer to implementing cards like Magical Hack and Sleight of Mind (at least for spell targets).