[40K] Implement Seeker of Slaanesh, added new must attack option #12712
Conversation
JayDi85
changed the title
|
“Must attack” effects are old, many cards uses it. Are there was a miss implementation and current PR fixed it (for human only and AI players still buggy)? |
|
I was not able to find any cards that do exactly what this card does--requiring all opponents to attack /anyone/ with at least one creature, irrespective of who they attack. Yes, AI does not respect this requirement at all right now. I looked into adding it and fixing the linked issue, but it looks like there's a lot of history in the |
| @@ -1916,13 +1916,17 @@ private boolean checkIfAttackersValid(Game game) { | |||
| // check if enough attackers are declared | |||
| // check if players have to be attacked | |||
| Set<UUID> playersToAttackIfAble = new HashSet<>(); | |||
| boolean mustAttackAPlayer = false; | |||
There was a problem hiding this comment.
misleading name - effect says "must attack" so could be a planeswalker or battle
make sure to check your code for this possibility
There was a problem hiding this comment.
Good catch. My code isn't only checking for creatures attacking players (only that at least one creature is attacking at all), but I will modify the "can any creatures attack" check to check for any defenders at all, not just players
There was a problem hiding this comment.
I still don't like the variable name "mustAttackAPlayer"
There was a problem hiding this comment.
Ah i see, i misread.
|
|
||
| @Override | ||
| public boolean mustAttack(Game game) { | ||
| return true; |
There was a problem hiding this comment.
are you sure this works the way you want it to? seems like this would require all creatures to attack.
There was a problem hiding this comment.
It does, since playerRelated is set to true. all other calls to getApplicableRequirementEffects pass in false for this flag, so they will not see this requirement when checking creatures.
#8954
Added a player-based "must attack" check in
HumanPlayerto satisfy this effect. Unfortunately, this seems to have added to the backlog of issues related to must-attack requirements that the AI isn't checking for:#6911
I threw in an ignored test with a similar TODO to make note of this.