-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Ai battle tests + ai logic fixes #3361
Conversation
Everything passes now. I won't add anything new to this PR, so it should be ready to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AI tests!! 🥳
I'm mostly nitpicking names and syntax here, the work itself looks really solid!
Would you feel up to rebasing and separating the non-test-related changes in, e.g. battle_ai_util.c
from the test-related changes. If not, I was thinking I'd squash this PR?
include/test/battle.h
Outdated
#define EXPECTED_MOVE(battler, ...) ExpectedMove(__LINE__, battler, (struct MoveContext) { APPEND_TRUE(__VA_ARGS__) }) | ||
#define NOT_EXPECTED_MOVE(battler, _move) ExpectedMove(__LINE__, battler, (struct MoveContext) { .move = _move, .explicitMove = TRUE, .notExpected = TRUE, .explicitNotExpected = TRUE, }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if this should use a different type rather than MoveContext
. Some of the things in MoveContext
don't seem to make sense in the AI, e.g. allowed
or rng
, and similarly notExpected
doesn't seem to make sense in a regular MOVE
.
It might be possible to combine that new type with FourMoves
, and thus combine EXPECTED_MOVE
and EXPECTED_MOVES
(and same for NOT
variants)? I think it would be nice to combine them from an API perspective, although idk if that would run into trouble with explicitly specifying the target:
in double battles?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with what you said, though I had my reasons for using MoveContext
. allowed
is of no use, but I wanted to have an option for potential AI tests to specify things like critical hit, secondary effect and mega evolution.
As for combining them, I couldn't really think of a way to do that without losing the ability to specify target(Since EXPECT_MOVES
can't specify the target). I'm kinda blocked with what I have atm.
Though if you have any ideas, maybe it could be changed 👀
Co-authored-by: Martin Griffin <[email protected]>
Co-authored-by: Martin Griffin <[email protected]>
Co-authored-by: Martin Griffin <[email protected]>
Co-authored-by: Martin Griffin <[email protected]>
Co-authored-by: Martin Griffin <[email protected]>
Co-authored-by: Martin Griffin <[email protected]>
Co-authored-by: Martin Griffin <[email protected]>
Co-authored-by: Martin Griffin <[email protected]>
Apart from the things in the previous comment, all of the other requested changes should be submitted. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll keep thinking about merging EXPECT_MOVE
and EXPECT_MOVES
, but there's no need to hold up the PR waiting for that :)
This reverts commit 6745f13.
This reverts commit cfa63ac.
The long awaited AI tests which should help get rid of all the popping issues related to AI, as well as improve it overall.
Replaces #2871
Fixes #3130 with tests
Fixes #3153 with tests
I implemented it differently than griffin(for example the log functionality or expected moves), but the general idea is the same. Having some way of making sure AI behaves the way we want it to behave.
The new
AI_BATTLE_TEST
andAI_DOUBLE_BATTLE_TEST
follow the same rules as regular battle tests. The only difference is inGIVEN
andTURN
.In
GIVEN
, usingAI_FLAGS
, we specify which flags AI has for the test.In
OPPONENT
we have to specify all moves the AI has.TURN
is different in that we don't control the opponent, but rather guess what it'll do. And if we guess correctly, that is AI does what we want, the test passes. Instead ofMOVE
,SEND_OUT
andSWITCH
there areEXPECTED_MOVE
,EXPECTED_SEND_OUT
andEXPECTED_SWITCH
. These command work the same as their regular counterparts.Note, for
EXPECTED_MOVE
there are 3 possible variants:EXPECTED_MOVES
- same asEXPECTED_MOVE
but allows more than one move. Useful when there are several valid moves for the AI to choose from.NOT_EXPECTED_MOVE
- reverse ofEXPECTED_MOVE
. Test fails if AI decides to use specified move. Useful when we want to make sure the objectively bad move in a certain situation is never gonna be used.NOT_EXPECTED_MOVES
- same as above, but specifies that neither of the given moves are going to be used.New
SCORE
commands. Apart from the above, there is another way of checking the correctness of AI's logic by comparing the AI scores. We can compare one move's score to another's or one move's score to a specific value.We compare the values using
EQ
,LT
,NE
andGT
which correspond to equal, less than, not equal and greater than.The 8 possible combinations are then:
I wrote a couple of tests already for this PR, so it'll hopefully be more clear with the examples.
I also fixed some fixes(xd) from #3199 with tests to confirm.
Note: Nothing in this PR is set for stone, so any feedback is greatly appreciated :)