Reworking goad effects (ready for review)

[theelk801]

Changing goad effects so that they add a designation to the creature that is goaded. I'll build goad support directly into the game.

• goad effects now set a permanent as having been "goaded"
• implement Vengeful Ancestor
• add goad support directly into combat engine (waiting on comprehensive rules update to be released)
• write some more tests

[theelk801]

still having problems with this, not sure if it's due to the changes not working or issues with the test framework or both

[theelk801]

ok now when I run the test I added it attacks a random player including the goading player and also sometimes no players at all

[JayDi85]

BTW you can use runCode command in tests -- it allows to make checks and asserts at any player's priority time. Not only at the end. Example: it can run attacks restriction or targets count (is it correct or not).

[theelk801]

oh wow, that's awesome

[theelk801]

@JayDi85 I think there's something wrong with TestPlayer's combat handling in multiplayer, I copied testSimpleAttackRequirementBerserkersofBloodRidge from AttackRequirementTest into GoadTest and it didn't work at all.

[theelk801]

ok so it turns out that testRegularCombatRequirement actually passes or fails randomly

[theelk801]

it seems like this is something to do with target selection

[JayDi85]

ok so it turns out that testRegularCombatRequirement actually passes or fails randomly

Testing on current build, not PR. There are bug in getRandomOpponent call -- it selects only first opponent, all other will be miss. It's never work from 2015 and only affected in multiplayer games (with multiple opponents).

I don't known -- is it a real bug or it was a feature. randomOpponentId field logic in targeting -- AI use it at the end if all other targets can't be selected. So in "bugged" version AI attacks only first player in random cases (30 of 100 times in 4x players game).

count++ can fix the problem (AI will choose random player all the time):

[JayDi85]

Maybe related issues to that PR (wrong goad/restriction):

• AI ignore playerMustBeAttackedIfAble restriction (Trove of Temptation) #6911
• Goad doesn't work correctly #6970

[theelk801]

ok I just pushed that one fix, I'll take another look at the other ones

[theelk801]

ok, it looks like the issue is that the randomly chosen opponent isn't necessarily valid for TargetDefender, and in general doesn't seem to care about the targeting restrictions

I'm testing a fix right now

[theelk801]

seems like it's choosing the opponent just fine but the attackers aren't getting declared anyway

[JayDi85]

Todos in targeting code -- about choosing good or bad target. Look at target permanent code -- it selects by toughness to kill first. So player target can be same -- selects first to kill.

Btw targeting code uses for non target calls, e.g. on resolve or choices. AI selects real spell targets by game simulations and battlefield score calc.

[theelk801]

I think the changes I made to targeting were fine since they only seem to matter when attacks are being forced. Anyway, all the tests pass now.

[JayDi85]

Is it fix something in goad or it just a refactor?

[theelk801]

what do you mean?

[JayDi85]

1. See old issues example above.
2. New code example -- removed planeswalker priority as a target:
   

[JayDi85]

Another breaking example:

[JayDi85]

So something changed in game logic, not only refactor.

[theelk801]

I took that to mean that it shouldn't have been choosing planeswalkers at a higher priority, plus it was using a randomly chosen opponent rather than one of the defenders that the target was supposed to choose from. I'm pretty sure the only situation where that code block is even used is when the player is forced to attack.

And I removed those setNotTarget calls because I added it directly into the TargetDefender constructor.

[theelk801]

oh I see what you're asking, yeah it's a fix and a refactor, but mostly because the rules for goad changed and I wanted to rework the implementation anyway because it wasn't working right

[theelk801]

just rebasing, not merging

[theelk801]

I'm gonna merge this tomorrow