Aura Spells have internal Attach Spell for multiple Enchant Keywords

[Hanmac]

Closes #169

I can't add the AI flags into the Keyword because of Tallowisp,
so i added Svars instead.

• Updated Cards
• Fixed GUI, i think?
• AI still need to use filter the possible list via canBeAttached

[Hanmac]

@tool4ever i don't know why " Incremental Growth " fails ... that isn't even an Aura?

Incremental Growth (8) - [Couldn't add to stack, failed to target] - Put a +1/+1 counter on target creature, two +1/+1 counters on another target creature, and three +1/+1 counters on a third target creature.[[Flying Men (1)], [Forest Bear (7)], []]

[Hanmac]

@Agetian can you look at the failing test case why the AI has problem with Incremental Growth?
i somehow can't see how this is related to this MR

[Agetian]

Hmm, not sure about Incremental Growth (and why it isn't targeting correctly in one of the tests) yet, but the immediate failing test for me is testPlayingSorceryPumpSpellsBeforeBlocks, which fails at line 609 of SpellAbilityPickerSimulationTest. What's happening there is the AI is playing a Dwarven Trader and then tries to make the AI decide on playing Furor of the Bitten on it, which is what line 609 is asserting. Furor of the Bitten is indeed affected by this MR, since it uses the new AI SVar for the aura attach. Possibly the SVar isn't accounted for somewhere in the AI code yet? Please let me know if you're able to figure this one out, we can take a look at Incremental Growth next if that causes issues once this immediate test case is fixed.

[Hanmac]

Hmm, not sure about Incremental Growth (and why it isn't targeting correctly in one of the tests) yet, but the immediate failing test for me is testPlayingSorceryPumpSpellsBeforeBlocks, which fails at line 609 of SpellAbilityPickerSimulationTest. What's happening there is the AI is playing a Dwarven Trader and then tries to make the AI decide on playing Furor of the Bitten on it, which is what line 609 is asserting. Furor of the Bitten is indeed affected by this MR, since it uses the new AI SVar for the aura attach. Possibly the SVar isn't accounted for somewhere in the AI code yet? Please let me know if you're able to figure this one out, we can take a look at Incremental Growth next if that causes issues once this immediate test case is fixed.

AttachAILogic is just turned into AILogic by this, so that shouldn't be the problem.

                if (c.hasSVar("AttachAILogic")) {
                    extra += " | AILogic$ " + c.getSVar("AttachAILogic");
                }

I think the AI needs better logic what it can target with AttachAi.attachPreference
So if sa is a Spell of an Aura Card, it should check canAttach too instead of just tgt

[Agetian]

Yeah, agree about the AI logic improvements, sounds like a good thing to implement 👍

[Hanmac]

tests are green, and i did some hard wire that canTarget does call canAttach for Aura Spells

[tool4ever]

yea that's what I meant
but wouldn't adding CanBeEnchantedBy property to the ValidTgts$ be much cleaner? 🤔

[Hanmac]

yea that's what I meant but wouldn't adding CanBeEnchantedBy property to the ValidTgts$ be much cleaner? 🤔

I don't know? Like this? ValidTgts$ Card.CanBeEnchantedBy,Player.CanBeEnchantedBy

You can try it if you want?

[Hanmac]

@tool4ever can you check why the last change does break the tests again?

getAllCandidates there calls sa.canTarget which then should call tr.getValidTgts() ?

Found it!

[Hanmac]

@tool4ever the MR should be clean now?
@Agetian now that CanBeEnchantedBy is part of the ValidTgts, the AI should respect that?

[Agetian]

@Agetian now that CanBeEnchantedBy is part of the ValidTgts, the AI should respect that?

Hopefully yes, but if you can tell me some test cases for that, I can give it a shot and test/trace it in action :)
Otherwise, looks good, yeah 👍

[Hanmac]

@Agetian it is mostly the problem that tgt.canTgtPlayer() doesn't work anymore
but i think the one place in AttachAi where that is used got updated

[tool4ever]

was this branch tested?
I have doubts it still works now...

[Hanmac]

It got tested by GameSimulationTest.testEpochrasite

Or should we do another Test case for this?

[tool4ever]

that's not how the tests work, they are only to check engine and won't use AI api for the most parts
this code wouldn't be reached and such changes need to be confirmed manually ingame

[tool4ever]

why resolve when it's been broken? :/

[Hanmac]

@tool4ever you or @Agetian could try to add an extra Animate Dead Test that uses AI parts

[tool4ever]

I don't think we have a precedent for that...? So we'd need to add some framework parts there first.
Anyway to fix the logic here I suggest you try using AnimateAi.becomeAnimated so the new Aura is able to pass the check

[Hanmac]

@tool4ever i will need to look at Weekend how to change the new Aura

[Agetian]

I agree that it's important to come up with some in-game board states that would be good to check how this works in practice, it's a bit difficult for me to figure out what would be good test cases tbh, but yeah, the automated tests won't help since they use the simulation AI which is a completely different thing in itself :)

[Hanmac]

@Agetian @tool4ever, something unrelated, that ticked me off before:

forge/forge-gui-desktop/src/test/java/forge/ai/simulation/SpellAbilityPickerSimulationTest.java

Line 827 in 53fca12

public void threeDistinctTargetSpellCantBeCast() {

still shows this in the Log:

Incremental Growth (8) - [Couldn't add to stack, failed to target] - Put a +1/+1 counter on target creature, two +1/+1 counters on another target creature, and three +1/+1 counters on a third target creature.[[Flying Men (1)], [Forest Bear (7)], []]

i know the spell should not be able to cast, but the Stack Message should still not appear there?

[Agetian]

Hmm yeah, not sure what's happening there, probably needs looking into, yes. I'll see if I can figure it out on the upcoming weekend.