Hanmac's Refactoring Thread

[Hanmac]

in this thread i want to post and collect the things i am doing refactoring within forge.
i want to make an extra thread for that to keep the bug threads free from it and to see your opinions about my changes before i do them.

First Topic:
"Splice (onto) <Arcane>"
Now:

• currently it does show a massive list there you try to select which of the many combinations you want.
• also there is currently no way to select the order, but the rules explicit says that you choose an order (even if it might not have a big effect from the spells)

Refactored:

• i did remove it from GameActionUtil and make a new function in AbilityUtils#addSpliceEffects(SpellAbility) which does return the cloned and modified SpellAbility object. (cloned to prevent it from changing the main object)
• inside there is a call getController().chooseCardsForSplice where the player can select the cards to splice similar to the Effects of a Charm Effect. (Gui().many(0,cards.size()))

Missing:

• The AI currently does not know what do to with that. should that be done in PlayerControllerAi or should that go to AiController?
• my Plan is that the AI does try to find the best combination (irrelevant order) of Splice Cards it has the mana to pay for.

[Agetian]

Sounds like a good change. I found Splice onto X to always be very confusing inside Forge.

- Agetian

[Hanmac]

My current sollution for the AI choices for the splice cards:
First order them descending by cmc (thinking that better cards cost more)
Then try to add cards with checking again if AI does still want to play that combined spell and can pay the cost.

Not tested yet but it should work.

The only problem with that is that AI will probably prefer to pay the more costing card instead of splicing into a cheaper spell.

[Hanmac]

because the Splice thing was finished faster than i wanted, we might discuss the next topic. (even if the AI part might not be perfect yet)

next thing is something for AI.
while testing how CounterMoveAi works, i tested Evolve Trigger.
Specially Simic Fluxmage and Cytoplast Root-Kin because of their different MoveCounter Effects.

while testing i noticed that the AI currently has no logic how to stack Abilities that are put on the stack the same time.

in this example the AI should prefer to resolve Evolve Trigger first before PutCounter(All) Effects, so the creature can benefit from that.

another example would that the AI should prefer Draw Effects before Discard Effects (even if the Discard are Reverse-Loot effects)

[Agetian]

I agree that since the AI can't really make strategic considerations and operates on a purely tactical (decision-by-decision) basis, then common sense logic should be applied to ordering abilities entering the stack at the same time when possible (Evolve before PutCounter, Draw before Discard, etc.). I think the AI would benefit from that. Even though it won't be universal, at least it'll fit most typical scenarios where the AI would commonly misplay.

- Agetian

[Hanmac]

Because a simple sort might to complex, I will split the list by APIType first and put them together later. For that I will add a SpellAbilityPredicates helper like the others.

For Draw vs Discard:
If mandatory Discard do it first if hand is empty.
Then do Draw (first the ones without discard subAbility)
Then do Loot (Draw -> Discard)
Then do optional Discard (because it's most likely ReverseLoot)
Then do mandatory Discard if not done before.

[Agetian]

Because a simple sort might to complex, I will split the list by APIType first and put them together later. For that I will add a SpellAbilityPredicates helper like the others.

For Draw vs Discard:
If mandatory Discard do it first if hand is empty.
Then do Draw (first the ones without discard subAbility)
Then do Loot (Draw -> Discard)
Then do optional Discard (because it's most likely ReverseLoot)
Then do mandatory Discard if not done before.

Sounds reasonable, but maybe we should also think about what to do on a random discard (in that case it's questionable whether it's better to draw first and then random-discard or the other way around; not as straightforward of a choice, anyway; probably in most cases you'd want to random-discard first and then draw something new, hoping that it'll make a difference, but the AI doesn't evaluate its hand vs. a potential possible draw in any way, so this line of thinking does not apply to it. :/ Also, if you have zero cards in hand, then a discard effect should be of higher priority to draw since you won't have to discard a card you've just drawn - this actually applies to both standard discard and random discard).

- Agetian

[Hanmac]

i did commit the first thing with Evolve/PutCounter and Draw/Discard Effects.
for that i added some SpellAbilityPredicates as helper. (that can be useful somewhere too)

there is also a filterList helper function. it does remove the filtered elements from the input list. I use it to have the filtered elements first, and the ones i do not care later.
But i don't know if i should move that to a more common place.

i don't know if there should be more API checks, or more special cases like if the hand is empty or it has DiscardMe cards.

New Project:
Refactoring ReplacementEffects, specially if they are doubling a value like damage or counters or tokens, it might be easier for the Replacement effects to just change the values and have the original function handle the rest.

that could fix the problem with lifelink and (battle)damage manipulation

for that i might add an extra Effect named ReplaceVar.
as Example for Gisela, Blade of Goldnight it would look like that:

Code: Select all

R:Event$ DamageDone | ActiveZones$ Battlefield | ValidSource$ Card | ValidTarget$ Opponent,Permanent.OppCtrl | ReplaceWith$ DmgTwice | Description$ If a source would deal damage to an opponent or a permanent an opponent controls, that source deals double that damage to that player or permanent instead.
SVar:DmgTwice:DB$ ReplaceEffect | VarName$ DamageAmount | VarValue$ X | References$ X
SVar:X:ReplaceCount$DamageAmount/Twice

it also would shorten the code because it now doesn't need to differ between combat and non-combat anymore

what do you guys think about the plan?
it would require some more rewrites because the effects which does call the ReplacementHandler does need to be updated too, to understand what is going on, but i think it would help the code.

[Agetian]

Personally I really appreciate the effort here and I think this is a very good idea. It will both streamline the code (and also remove the need to distinguish some special cases in code) and also allow the game to work properly in certain cases where currently the game does not follow the rules and behaves strangely. Best of luck!

- Agetian

[Hanmac]

i did the ReplaceEffect. tested with addCounter and Hardened Scales

Code: Select all

R:Event$ AddCounter | ActiveZones$ Battlefield | ValidCard$ Creature.YouCtrl | ValidCounterType$ P1P1 | ReplaceWith$ AddOneMoreCounters | Description$ If one or more +1/+1 counters would be placed on a creature you control, that many plus one +1/+1 counters are placed on it instead.
SVar:AddOneMoreCounters:DB$ ReplaceEffect | VarName$ CounterNum | VarValue$ X | References$ X
SVar:X:ReplaceCount$CounterNum/Plus.1


the testing works so far, two of them work correct together, but i am unsure yet how they would work with normal replacement effects. (but should work)

it also uses calculateAmount, for calcing the new value, that might not work yet for other changes. (like damage redirection)

[Agetian]

Ah, this is very nice, Hanmac! Great to hear that this plan is coming to fruition. Is there a possibility to adapt this for use in the lifelink scenario vs. damage replacement?

- Agetian

[Hanmac]

i updated TokenDoubler now as keyword using Replacement Effect.
with that i can finally get Varchild's War-Riders as real Cumulative upkeep working.

for that i planed a cost like that:

Code: Select all

CreateToken<1/Player.Opponent/1/1/Red/Creature,Survior>


it currently does not support keywords and other stuff, but i think we will not need it.

for the lifelink scenario i will look, there are many more cards than just with the Token Doublers

[Agetian]

Sounds very good!

- Agetian

[Agetian]

Hmm, for some reason Android compilation was broken and it's complaining about a method in the newly introduced ReplaceEffect

Code: Select all

[INFO] Warning: forge.game.ability.effects.ReplaceEffect: can't find referenced method 'java.lang.Object replace(java.lang.Object,java.lang.Object)' in class java.util.Map


What would be a good way to modify the implementation to satisfy this? :/
(I may be wrong but I think the Android SDK we're targeting is using Java 7 era libraries).
Would this, for example, suffice as an alternative?

Code: Select all

                if (originalParams.containsKey(e.getKey())) {
                    originalParams.put(e.getKey(), e.getValue());
                }


- Agetian

[Hanmac]

Hm normally a simple put should be enough because the map should not have parameters with where not there before.

(You can replace "replace" with just "put")

I wanted to do a map.replace (newMap) but there is no nice function for that.