-
-
Notifications
You must be signed in to change notification settings - Fork 107
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
Putting "pass" in message when battle order is None in DoubleBattleOrder #659
base: master
Are you sure you want to change the base?
Putting "pass" in message when battle order is None in DoubleBattleOrder #659
Conversation
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.
This is a good change.
Can you add some unit tests?
…-battleorder-is-pass
…-battleorder-is-pass
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #659 +/- ##
==========================================
+ Coverage 83.38% 85.26% +1.88%
==========================================
Files 39 42 +3
Lines 3918 4392 +474
==========================================
+ Hits 3267 3745 +478
+ Misses 651 647 -4 |
…-battleorder-is-pass
1ab04a3
to
c5612fb
Compare
b4121f4
to
abbd584
Compare
abbd584
to
0df3a58
Compare
@hsahovic this is being blocked by bugs in poke-env not being able to handle Zoroark properly😞 all other tests pass. I guess the good news is that this PR exposes a bug, but the bad news is that Zoroark is a nightmare. @caymansimpson if you're interested in taking down one of the most infamous problems of all time, you're invited hahaha |
haha I don't think it's worth our time to tackle Illusion; it involves keeping multiple possible battle states and integrating speed/move inferences into the core of poke-env, which if comprehensively done, will require 10K+ lines of code (my inferences work is 95% accurate at 6K lines; there are so many niche interactions). |
@@ -130,6 +130,13 @@ def parse_request(self, request: Dict[str, Any]) -> None: | |||
if side["pokemon"]: | |||
self._player_role = side["pokemon"][0]["ident"][:2] | |||
self._update_team_from_request(side) | |||
if self.player_role is not None: |
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 is this change in this PR?
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.
So, this was a little while ago, but if I remember right, this was to try and simplify the code and fix Zoroark bugs on our side of the battle specifically (since that's all the request tells us about, is our own side).
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.
to be clear, it's a replacement for the code being removed a few lines further below.
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 definitely could test taking it out real quick and see what happens, whether that breaks stuff.
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.
Alright, I remember it all now. So, yes, this is a necessary fix, and it is for Zoroark. This is also just a much easier code chunk to look at than what it's replacing. Basically, the request will always put the active pokemon in the first and second positions in the pokemon list, and although you could be worried that we aren't correctly filtering out fainted pokemon, if you look at the _get_active_pokemon method in double_battle.py (which is used to give the active_pokemon property), you'll see that it is filtered there, so we were actually double-filtering before.
8457112
to
d43f66d
Compare
…-battleorder-is-pass
Requires #667, #666
Currently, there is no way to send pass in poke-env. Having DefaultBattleOrder as the default option for the orders in DoubleBattleOrder is misleading, and it really should be pass. The user could just provide DefaultBattleOrder to the DoubleBattleOrder constructor if they want one of the sides to be default, so this is a strict empowerment for the user.