-
-
Notifications
You must be signed in to change notification settings - Fork 135
Fix move tracking #816
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
base: master
Are you sure you want to change the base?
Fix move tracking #816
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #816 +/- ##
==========================================
- Coverage 83.38% 82.88% -0.50%
==========================================
Files 39 47 +8
Lines 3918 5691 +1773
==========================================
+ Hits 3267 4717 +1450
- Misses 651 974 +323 🚀 New features to boost your workflow:
|
b3af5f3 to
994fdab
Compare
c3963ee to
cd0d901
Compare
hsahovic
left a comment
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.
Thanks for this PR! I left a couple of questions / comment - I'll merge when they are resolved.
| "[from] lockedmove", | ||
| "[from] Pursuit", | ||
| "[from]lockedmove", | ||
| "[from] Sky Attack", |
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.
is sky attack still handled?
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.
See here for what I yielded from my investigation: ada8c73. Basically, that message only seems to appear in random battles in early gens, so I added those to the strict integration tests and passed through them a few times to weed out existing early-gen bugs.
src/poke_env/battle/battle.py
Outdated
| if self.active_pokemon is not None: | ||
| if ( | ||
| strict_battle_tracking | ||
| and self.gen not in [7, 8] |
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.
out of curiosity - why only these gens?
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 made the give-up for move consistency checking more specific here: 7fd69fa. The reason why we need to do this is because the mapping from moves to Z-moves or Dynamax moves is not invertible (2 distinct moves may have the same name for their z-move or dynamax move). Thus, it is impossible to track pp changes in those circumstances, so I have the code just not check that in gen 7 and 8, but only for pp consistency checking now.
src/poke_env/battle/battle.py
Outdated
| self._available_switches.append(pokemon) | ||
|
|
||
| def _pressure_on(self, pokemon: str, move: str, target_str: Optional[str]) -> bool: | ||
| if self.gen == 7: |
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.
why?
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 made the if-statement more specific and added an explaining comment: 4318f77
src/poke_env/battle/move.py
Outdated
| if pressure: | ||
| self.current_pp -= 2 | ||
| else: | ||
| self.current_pp -= 1 |
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.
should we bound this at 0?
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.
| ] | ||
| if not matches: | ||
| continue | ||
| move = matches[0] |
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.
should we assert only one match?
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.
790d722 to
e3afdd5
Compare
Improved move tracking guided by strict integration tests, which now also validate the tracked battle state against the
"active"section of the request message we receive. I also remove the dead code ofEmptyMove, which is completely unused throughout poke-env.