More robust battle tracking #666
Conversation
cameronangliss
force-pushed
the
fix-request-protocol-consume-order
branch
from
6a9b635 to
6c22de7
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #666 +/- ##
==========================================
+ Coverage 83.38% 84.94% +1.56%
==========================================
Files 39 42 +3
Lines 3918 4438 +520
==========================================
+ Hits 3267 3770 +503
- Misses 651 668 +17 |
cameronangliss
force-pushed
the
fix-request-protocol-consume-order
branch
8 times, most recently
from
f404a69 to
ae0777b
Compare
|
Love this change -- long overdue! Have you written tests to show this works? I'm looking at this and comparing this to Showdown protocol, but might be easier for hsahovic to if add tests (also so if anyone after you messes up, we'll know) |
|
Yes, will get to that hopefully tomorrow, I'll see when I can find time. |
|
@hsahovic this is all set! Passing to you |
src/poke_env/player/player.py
Outdated
| @@ -262,6 +262,9 @@ async def _handle_battle_message(self, split_messages: List[List[str]]): | |||
| :type split_message: str | |||
| """ | |||
| # Battle messages can be multiline | |||
| will_move = False | |||
| from_teampreview_request = False | |||
There was a problem hiding this comment.
i think these two could be named a bit more explicitly, for instance something like should_process_request and should_process_teampreview_request
There was a problem hiding this comment.
How's this: f13727b
src/poke_env/ps_client/ps_client.py
Outdated
| split_messages = protocol or [[f">{battle_tag}"]] | ||
| if request is not None: | ||
| split_messages += [request[1]] | ||
| await self._handle_battle_message(split_messages) # type: ignore |
There was a problem hiding this comment.
my understanding is that the changes in the client are to not treat requests and protocol updates separately, is that correct?
if so, i think this logic should live in Player._handle_battle_message, as the client should be agnostic to implementation details of the battling protocol
additionally, requests should be stored in battle objects instead of the protocol / player objects
There was a problem hiding this comment.
Not exactly. It is to swap the order that they are consumed. I would argue that the logic should be in the client since _handle_battle_message is built to take only one split_messages at a time. I tried to make it so we didn't have to combine the protocol and request as I have done here, but I wasn't able to get that to work unfortunately. The logic inside of _handle_battle_message was fighting back at me hahaha
There was a problem hiding this comment.
Stated simply, this is the change:
BEFORE: we consume messages as they come. Showdown sends the request before the protocol, so we consume request and then protocol.
AFTER: we consume messages as they come, EXCEPT for the request-protocol pair, which we swap the order of upon receiving them.
Refer to the PR description for why this is a good idea
There was a problem hiding this comment.
i see - in this case let's pause on this one for now; i want to take a look at possible alternatives
There was a problem hiding this comment.
Sure thing. One path forward I can see is to do this change and then make an effort towards the ps-client rework that we were talking about the other day. Let me know if you have any other questions.
There was a problem hiding this comment.
I've addressed this concern now, hopefully you like the change! I moved this logic to player.py.
cameronangliss
force-pushed
the
fix-request-protocol-consume-order
branch
from
8898d97 to
c80d8a2
Compare
cameronangliss
changed the title
|
@hsahovic I put elements of #659 into this PR because it ended up that this PR needed the changes from it, and also it needed the changes from this PR in order to handle Zoroark better. I've rebranded this as an effort to improve the battle tracking in poke-env. Also, I finally got around to pulling the logic that reverses the order of the request and protocol into player.py and out of ps_client.py like you asked!🥳 Let me know what you think! |
cameronangliss
force-pushed
the
fix-request-protocol-consume-order
branch
from
cebbf4d to
0ce7f67
Compare
|
I'm realizing that even though this seems to pass tests, this PR is NOT ready for merging. This PR is exposing bugs with the way that we handle Zoroark which show up (sparsely, but it does show up). We need to rewrite the way we process the request in order to correct for the mistakes we're making, since the request can get us out of this mess by telling us the ground truth of the battle. |
…request-protocol-consume-order
Fixes #616
Currently we consume the messages in the order they come in, which is request and then protocol message. We really should update the battle object with the protocol first, and then update with the battle request - and now we do! This is better because it prevents us from accidentally overwriting something that is definitely true with our interpretation of the protocol, which currently has bugs. Specifically, this guarantees that we can correctly track Zoroark if it's on our team, and we resolve several other bugs that have remained silent until now.