AbilityFactory refactoring

[Max mtg]

Hey,

It looks like AbiltyFactory might use some refactoring. See r17739 for my thoughts on how this can be made... we got separate files to describe the spell effect behaviour and the ai's attitude towards this spell.

I might be able to perform the same operation for the rest of classes.
What do you think? Is this viable or does it need an urgent rollback?

UPD: Refactoring done... Some stats to compare in that abilityfactory package

r17736: 38 files, 1 679 770 bytes
r17907: 189 files, 1 020 011 bytes.

As a result of this refactoring some 650 kilobytes of duplicate code were eliminated.

[Sloth]

I think this is a good idea in general. Sol is the creator of AbilityFactory and may have more to say about the details, but i don't see anything here that i don't like.

I'm already thinking about the possibility to move some shared AI checks to SpellAiLogic.

[friarsol]

Yea I feel relatively neutral about this. If you think it'll help AI Organization and sharing checks, then that's fine. Although I do like the fact that I can currently find all the information about a single AF in the same file, instead of needing to hunt down some other file because I'm adding a new feature on.

[Max mtg]

Actually, yes.

I think this refactoring will:
1. get rid of those classes declared inside create[Ability|Spell|Drawback]XXX which refer to pretty the same methods and reduce number of lines this way.
2. decouple AI from abilities. Now there is only one AI and it's tied to SpellAbility, while the Forge game server would better have separated and replaceable AI. I hope sometime it will become possible to move the canPlayAi() method away from SpellAbility class.
3. Make code easier to understand. There's an api: here is a class with how it works, and there is a default AI behaviour class. We may even may have two maps: string->SpellEffect and string->AiLogics to get an appropriate instance of ability executor and AI in the factory method and then create ability, spell or drawback with a single unified constructor.

Sol, I hope, you'll help me sometimes about this task. I'll refactor it by myself (thanks to G510 keyboard which helps a lot with macros), but some logical errors are still expected. So may I ask you to fix them or to point at what needs to be done when you find them.

[friarsol]

I don't know how much time I have these days for active development, but I'll watch the conversions and try to make some time for fixing/pointing out conversion issues. If we're going to be doing these refactoring anyway, it might be worth separating files with multiple AFs into their own files too. Although it may be easier to just do that as two different steps.

Example AlterLife File has GainLife, LoseLife, SetLife, etc. So instead of just two AlterLife files, we've had two files for each of those different effects.

[Max mtg]

what is the difference between af.getHostCard() and sa.getSourceCard()?

[Sloth]

what is the difference between af.getHostCard() and sa.getSourceCard()?

The difference is that af.getHostCard() shouldn't be used if possible, because it doesn't work with ability copying (Necrotic Ooze and friends).

[Max mtg]

what is the difference between af.getHostCard() and sa.getSourceCard()?

The difference is that af.getHostCard() shouldn't be used if possible, because it doesn't work with ability copying (Necrotic Ooze and friends).

I've seen it all around resolve() methods.
Once I replaced it with sa.getSourceCard() but not sure it that would be correct.

Now SpellAbility anyway holds a reference to abilityfactory (so I'm able to get sa.getAbilityFactory().getHostCard()) but I don't understand why it holds that ref. Because as I understand the pattern, factory is not supposed to be referenced from the classes it originated.


Can we just replace hostCard from af with SourceCard from sa globally?

[Max mtg]

I am almost done.

Got stuck currently with Attach, Charm and ChangeZone families.

1. Attach uses wome weird logics to put auras on the battlefield
2. Charm is for unknown reason called from forge.card.abilityfactory.AbilityFactoryCharm
3. ChangeZone has some special abilities creation code that does not fit into my model at the moment.

Might have a change to return for them later. Today I am quite tired from mana abilities refactoring

[friarsol]

Alright, I'll take a look at Attach to start see if I can use it as a springboard for the conversion process. Now that I've seen a bunch of them converted and played around with the code, I do like the further split of the AFs.

I never looked at Charm, so I'm not sure what the deal with it is. It seems like it's own entity, and not really the same type of Effect that everything else is.

I know ChangeZone is way complex. But it's mostly just a single fork in the beginning for most of the functions, if Attach conversion goes well, I'll take a stab at that afterwards.

[Max mtg]

A next step on this task would be convert API into an enumertion and make ability creation code use reflection, as it was done for triggers.

[Max mtg]

Attach done, commit pending

[friarsol]

Attach done, commit pending

I guess I shouldn't have wasted my time last night converting it then?

[Max mtg]

Why wasted? You had a great chance to practice with the refactored AFs.
I'll switch to a different area soon, and all those AIs will remain yours.

API to enum conversion done, Charm also done (I know it is incorrect now)- hope AI will be able to use it now
ChangeZone remaining.

[friarsol]

Because I only get so much time coding and I would have spent it doing something else if I knew you were still planning on doing it.