It is not actually necessary to resend the request if the rqid
is wrong because the player will have received the request after
sending in the move, so the client will still be in the move
selection phase.
The following sequence of events was causing a crash:
1) User sends in decision to server (with the wrong rqid specified).
2) Server receives decision and sends message M to simulator process.
3) User leaves battle and sends the server a message to inform of this.
4) Server removes the user from his or her battle in the main process,
which sets the user's entry in simulator.player to null.
5) The simulator process processes message M and, since the rqid in
the decision is wrong, send a resendrequest message back to the
main process.
6) The main process in resendRequest() tries to access properties of
the player, which are now null as of event (4), causing a crash.
This commit fixes the problem by making sure that user is not null
before accessing its properties.
Previously, Simulator.prototype.resendRequest() called user.sendTo()
with an invalid room specified, causing the server to send the
message to all rooms, leading to problems on the live server.
This refactored version checks the rqid as part of the decision
data, rather than introducing a new /ackrequest command. This
change is done in a backward-compatible way.
Most of the code does not change compared to the previous version.
The corresponding client commit is Zarel/Pokemon-Showdown-Client@3b82cf5cec
In certain laggy situations, the following sequence of events
was previously possible in a battle:
1) Server makes request R_1 and R_2.
2) User 1 selects decision D_1 for request R_1
3) User 2 selects decision D_2 for request R_2
4) Server receives D_1 and D_2 and runs the turn
5) Server makes request R_3 and R_4.
4) Due to lag, User 1 does not realise that the server has
already run the turn. User 1 wants to change his or her move
for request R_1, so User 1 cancels decision D_1 and sends in
decision D_3, intending it to be for request R_1.
5) User 2 selects decision D_4 for R_4.
6) Server receives D_3 and considers it to be the response to R_3,
rather than the response to R_1 (which was what User 1 intended).
7) Server receives D_4 and considers it to be the response to R_4.
8) Server runs the turn.
The result of this sequence of events is that a move that User 1
intended to be for request R_1 is instead used for R_3, which has
the effect of depriving the user of the ability to make a reasoned
choice of which move to use for R_3.
This commit attempts to solve this problem by requiring the client
to acknowledge having received a request before the server will
accept a decision for that request.
Unfortunately, there is no seamless way to implement this in a
backward-compatible way. In other words, if the client were updated
to always send acks, it would no longer be able to work with old
versions of the server. For example, if the ack were added to the
choice string, old servers would no longer be able to handle choice
strings sent from the client. Similarly, if the ack were added
in the form of a new command (say, /ackrequest), old servers would
emit an unrecognised command error on receiving the command.
This commit maintains backward-compatibility by introducing a version
field to the init data structure sent to the client when joining a
battle room. This version should be incremented for any future
backward-incompatible changes as well.
The implementation of the ack request system is that the client must
send in "/ackrequest rqid" for each request, where rqid is the ID of
the request. If the server has not received an ack for request 'rqid'
when it receives a decision for request 'rqid', it discards the
decision and asks the client for another decision.
The corresponding client commit is Zarel/Pokemon-Showdown-Client@ccf0a97733
- Random battles and automatic detection sets Hidden Power IVs correctly
- Hidden Power now always does damage based on user's IVs
- User's Hidden Power base power is now reported
Previously, Follow Me (and anything else that redirects moves, such as
various abilities) was unable to redirect the target of a
'randomNormal' target move, because validTargetLoc() always returned
false for a 'randomNormal' type move; this reflects the fact that the
client is not allowed to specify the target for such a move, because it
is chosen randomly. However, the internal game logic does need to be
able to specify a target for 'randomNormal' moves.
This commit adds a new parameter to validTargetLoc that specifies whether
the proposed target was chosen by the client or by the internal game logic.
If the proposed target was chosen by the internal game logic, then the same
targeting rules apply as in the case of a "normal" target move.
Note that it is necessary to add this new parameter, rather than just making
'randomNormal' behave the same as 'normal', because without the userSelected
check, a user could send a crafted choice string to the server specifying a
target for a 'randomNormal' move, and the server would respect the choice.
This implementation correctly prevents the client from specifying the target
of a 'randomNormal' move.
