Implemented Warping by kbarkevich · Pull Request #93 · sepalani/MH3SP

kbarkevich · 2023-03-11T23:41:02Z

This PR handles warping in the following cases:

Same Gate, City->City
Same Gate, Gate->City
Same Gate, City->Gate
Same Server, Different Gate, City->City
Same Server, Different Gate, Gate->City
Same Server, Different Gate, City->Gate
Same Server, Different Gate, Gate->Gate
Different Server, City->City
Different Server, Gate->City
Different Server, City->Gate
Different Server, Gate->Gate

"Different Server" warps are a multi-step process that periodically checks to see if the destination is full (or empty, in the case of a city.) The steps are as follows:

RecLayerJump/AnsLayerJump (Game initiates warp. First check to see if destination is valid happens.)
ReqLayerJumpReady/NtcLayerJumpReady (Game asks to initiate Server->Server hop, does so using standard FMP disconnect/connect sequences upon reply with Ntc packet. Second check to see if destination is valid happens.)
ReqLayerJumpGo/NtcLayerJumpGo (Game, now having completed the Server->Server hop, asks to enter their destination Gate/City. Third check to see if destination is valid happens.)

Testing has shown that the process is able to be gracefully aborted through an AnsAlert packet at any step. For example, if the Server->Server jump is completed, and then the process is aborted, the user will be returned to the "server select" screen. If the process is aborted before the Server->Server jump is completed, the user will remain in their current location.

TODO in a future PR: Implementation of Gate locking at the relevant steps. Currently, City locking is implemented.

Added checks to ensure destination is not full Added additional full checks and documented intention to add locks prior to gate/city jumps Removed debug True Removed newline Added City locking to warp logic

sepalani

This is my first pass on this PR. I'll try to have a look at these if/elif/else to find a cleaner way to refactor it.

sepalani · 2023-03-12T08:13:18Z

fmp_server.py

+            self.sendAnsAlert(PatID4.AnsLayerJump,
+                        "<LF=8><BODY><CENTER>You're already there.<END>",
+                        seq)


Are these alert messages necessary? I remember when I was reversing the Warp features, the game performed these checks without asking the server nor sending the packet. Isn't an early return statement missing as well?

sepalani · 2023-03-12T08:31:39Z

mh/session.py

+        return self.state == SessionState.CITY or\
+            self.state == SessionState.CIRCLE or\
+            self.state == SessionState.CIRCLE_STANDBY or\
+            self.state == SessionState.QUEST


return self.state in (SessionState.CITY, SessionState.CIRCLE, SessionState.CIRCLE_STANDBY, SessionState.QUEST)

sepalani · 2023-03-12T08:42:23Z

fmp_server.py

+                        "<LF=8><BODY><CENTER>Destination is full.<END>",
+                        seq)
+                return
+        else:


If the previous if/elif return early, this else statement can be removed and its code indentation reduced. Let's avoid if/elif/else forests when we have the opportunity. Most of the code in this else block can be deduplicated too.

sepalani · 2023-03-12T09:14:46Z

fmp_server.py

+        JP: レイヤ移動要求（位置バイナリ）
+        TR: Layer move request (position binary)
+
+        A relay of the target user's layer_host.


You should add a comment stating that this code is "racy" and the sanity checks are provided on a best-efforts basis.

As already stated we have no reasonable way to ensure the validity of these pieces of information. Preventing the warp due to outdated information is fine, IMHO. Indeed, we have no control over the other distant servers and their state can change between our check and the warp. The worst case being: a server changing from being available to full which these checks can't account for unless we lock the whole server.

Moreover, enforcing/locking a city or even worse a gate or server will severely harm performance, even more so if it's a distant peer. Which will prevent all its population to alter its state when someone is warping to it. If there are no strong reasons, you should avoid using lock here or add a comment to justify its benefits.

For instance, locking a city or a gate from another (distant) server shouldn't be possible. I really doubt that Capcom did these sanity checks anyway, though, I might be wrong.

I was thinking about that, and my idea was that they would have a method of communicating to the other server to reserve a slot. That way you avoid having an issue of state changing mid flight. But I don't know if they actually did that or if it would be fast enough to not create a noticeable slowdown in the process.

sepalani · 2023-03-12T11:32:35Z

mh/session.py

+    def get_server_full(self, server_id):
+        return DB.get_server(server_id).is_full()
+
+    def get_gate_full(self, server_id, gate_id):
+        return DB.get_gate(server_id, gate_id).is_full()
+
+    def get_city_full(self, server_id, gate_id, city_id):
+        return DB.get_city(server_id, gate_id, city_id).is_full()
+
+    def get_city_empty(self, server_id, gate_id, city_id):
+        return DB.get_city(server_id, gate_id, city_id).is_empty()
+
+    def find_gate(self, server_id, gate_id):
+        return DB.get_gate(server_id, gate_id)
+
+    def find_city(self, server_id, gate_id, city_id):
+        return DB.get_city(server_id, gate_id, city_id)


I'm really not a huge fan of this since the session module is mainly used to cache the server data and hide the implementation logic (for instance which DB backend we use, which function to call based on the packet we received and its parameters, etc.). That's also why most of its functions don't ask for the server_id as parameter and use the local_info when needed rather than being a simple wrapper to call the default database instance.

I'll let it slide for now since we don't provide a better abstraction to query the database directly, especially for other servers. AFAICT, this point seems still valid with the state abstraction you proposed in another PR because parts of the session modules (including the ones this PR added) needs to take into account whether or not the city/gate/server is handled by the current server process or not.

sepalani · 2023-03-12T11:36:40Z

fmp_server.py

+    def sendAnsLayerJump(self, seq):
+        """NtcLayerJumpGo packet.
+
+        ID: 64170200
+        JP: レイヤ予約移動実行返答
+        TR: Layer reservation move execution reply


Suggested change

def sendAnsLayerJump(self, seq):

"""NtcLayerJumpGo packet.

ID: 64170200

JP: レイヤ予約移動実行返答

TR: Layer reservation move execution reply

def sendAnsLayerJump(self, seq):

"""AnsLayerJump packet.

ID: 64100200

JP: レイヤ移動返答（位置バイナリ）

TR: Layer move reply (position binary)

sepalani · 2023-03-12T11:39:06Z

fmp_server.py

+    def recvReqLayerJumpReady(self, packet_id, data, seq):
+        """AnsLayerJump packet.
+
+        ID: 64100200
+        JP: レイヤ移動返答（位置バイナリ）
+        TR: Layer move reply (position binary)


Suggested change

def recvReqLayerJumpReady(self, packet_id, data, seq):

"""AnsLayerJump packet.

ID: 64100200

JP: レイヤ移動返答（位置バイナリ）

TR: Layer move reply (position binary)

def recvReqLayerJumpReady(self, packet_id, data, seq):

"""ReqLayerJumpReady packet.

ID: 64160100

JP: レイヤ予約移動確認要求

TR: Layer reservation move confirmation request

sepalani · 2023-03-12T11:43:14Z

fmp_server.py

+    def sendNtcLayerJumpReady(self, seq):
+        """ReqLayerJumpReady packet.
+
+        ID: 64160200
+        JP: レイヤ予約移動確認返答
+        TR: Layer reservation move confirmation reply
+        """


Suggested change

def sendNtcLayerJumpReady(self, seq):

"""ReqLayerJumpReady packet.

ID: 64160200

JP: レイヤ予約移動確認返答

TR: Layer reservation move confirmation reply

"""

def sendNtcLayerJumpReady(self, seq):

"""NtcLayerJumpReady packet.

ID: 64160200

JP: レイヤ予約移動確認返答

TR: Layer reservation move confirmation reply

"""

sic: I think this function was wrongly identified when I generated the constants so we should rename it AnsLayerJumpReady later at some point or add a comment stating that this function was wrongly named in game if that was the case.

sepalani · 2023-03-12T11:45:08Z

fmp_server.py

+    def sendNtcLayerJumpGo(self, seq):
+        """NtcLayerJumpGo packet.
+
+        ID: 64170200
+        JP: レイヤ予約移動実行返答
+        TR: Layer reservation move execution reply
+        """


sic: Ditto, regarding its name. Feel free to ignore this comment.

Implemented warping between cities, gates, and servers

20ae122

Added checks to ensure destination is not full Added additional full checks and documented intention to add locks prior to gate/city jumps Removed debug True Removed newline Added City locking to warp logic

sepalani requested changes Mar 12, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implemented Warping#93

Implemented Warping#93
kbarkevich wants to merge 1 commit intosepalani:devfrom
kbarkevich:warping

kbarkevich commented Mar 11, 2023

Uh oh!

sepalani left a comment

Uh oh!

sepalani Mar 12, 2023

Uh oh!

sepalani Mar 12, 2023

Uh oh!

sepalani Mar 12, 2023

Uh oh!

sepalani Mar 12, 2023

Uh oh!

InusualZ Mar 12, 2023 •

edited

Loading

Uh oh!

sepalani Mar 12, 2023

Uh oh!

sepalani Mar 12, 2023

Uh oh!

sepalani Mar 12, 2023

Uh oh!

sepalani Mar 12, 2023

Uh oh!

sepalani Mar 12, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

kbarkevich commented Mar 11, 2023

Uh oh!

sepalani left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

InusualZ Mar 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

InusualZ Mar 12, 2023 •

edited

Loading