Added timer-based & console-based announcement support by kbarkevich · Pull Request #75 · sepalani/MH3SP

kbarkevich · 2022-11-09T16:10:23Z

This pull requests implements a basic server-wide announcement system, similar to that which Capcom used to display their "Capcom says:" messages.

One basic "thanks for playing" message is scheduled to dispatch every 30 minutes
Added broadcast_message function, callable from the --interactive console, to send any message to all online users

sepalani

I'm going to test it against the Japanese version, asap. This version tended to have different char limit for message boxes.

sepalani · 2022-11-09T17:42:58Z

fmp_server.py

+    broadcast_segments = []
+    words = text.split(" ")
+
+    mod_words = []
+
+    for word in words:
+        curr_word = word
+        while len(curr_word) > 30:
+            mod_words.append(curr_word[0:30])
+            curr_word = curr_word[30:]
+        if len(curr_word) > 0:
+            mod_words.append(curr_word)
+
+    curr_broadcast = mod_words[0]
+    for word in mod_words[1:]:
+        if len(curr_broadcast) + 1 + len(word) <= 30:
+            curr_broadcast += " "
+            curr_broadcast += word
+        else:
+            broadcast_segments.append(curr_broadcast)
+            curr_broadcast = word
+    if len(curr_broadcast) > 0:
+        broadcast_segments.append(curr_broadcast)


Isn't there a simpler way to perform this? It looks like a lot of code for a simple task.

Didn't Capcom used newlines instead of creating a message for each line?

It has to trim down lengthy words to segments that are at most the maximum character length, then do the math word-by-word to see if each one fits onto the current line when including a space. Those are 2 distinct processes, thus the 2 different for loops.

I DID completely forget to use the constant MAX_CHAR_LENGTH I defined though. So, oops.

Also, I don't think Capcom did? The messages always showed up as distinct lines, each with their own sender name.

Also, I don't think Capcom did? The messages always showed up as distinct lines, each with their own sender name.

That seems correct, they are sending 4 messages as part of their announcement as seen on these videos:

https://youtu.be/ZTwdF9e9Kk0?t=2943

https://youtu.be/K5dX4KhUFUM?t=1570

sepalani · 2022-11-09T17:43:24Z

fmp_server.py

+        info.text_color = pati.Long(0x00fffff0)
+        info.sender_id = pati.String("000000")
+        info.sender_name = pati.String("[MH3SP]")


These should probably be optional parameters too.

sepalani · 2022-11-09T17:43:59Z

fmp_server.py

+        info.sender_name = pati.String("[MH3SP]")
+        data += info.pack()
+        data += pati.lp2_string(broadcast_text)
+        print("BROADCAST: " + broadcast_text)


Can't we log this message properly, somewhere, rather than using raw prints?

By the time it gets to the FmpServer, it's already bundled up into a binary. This function doesn't have access to any loggers since it's not local to any of the PatServer classes.

Is this print necessary since the server could log any packets including broadcast ones?

sepalani · 2022-11-09T17:48:26Z

master_server.py

+    broadcast_queues = {"FmpServer": queue.Queue(), "OpnServer": queue.Queue(),
+                        "LmpServer": queue.Queue(), "RfpServer": queue.Queue()}
    threads = [
-        threading.Thread(target=server.serve_forever)
+        threading.Thread(target=server.serve_forever, name=server.__class__.__name__,
+            args=(broadcast_queues[server.__class__.__name__], ))


If you plan to use server.__class__.__name__ then you should also use it in broadcast_queues rather than hardcoding the (dict's key) string.

sepalani · 2022-11-09T17:52:12Z

master_server.py

+    def broadcast_message(text):
+        broadcast = FMP.construct_broadcast(text)
+        for broadcast_segment in broadcast:
+            broadcast_queues["FmpServer"].put(broadcast_segment, block=True)
+


Maybe you should add a comment stating that it's for easier use in interactive mode. Moreover, you shouldn't use "FmpServer" string and prefer using .__class__.__name__ when possible.

sepalani · 2022-11-09T17:53:42Z

mh/server.py

+            if event == selectors.EVENT_WRITE:
+                try:
+                    if handler.session.layer > 0:  # in-game only
+                        handler.send_packet(packet[0], packet[1], packet[2])


If packet is a tuple of 3 elements, use the * operator instead: *packet.

Addressed Sepalani comments fixed issue where MAX_CHAR_LENGTH constant wasn't being used

sepalani

A very quick review.

sepalani · 2022-12-27T08:56:18Z

fmp_server.py


+def construct_broadcast(text="Hello, World!", text_color=0x00fffff0,
+                    sender_id="000000", sender_name="[MH3SP]"):
+    assert len(text) > 0


assert text should work.

sepalani · 2022-12-27T09:01:51Z

fmp_server.py

+    broadcast_segments = []
+    words = text.split(" ")
+
+    mod_words = []
+
+    for word in words:
+        curr_word = word
+        while len(curr_word) > MAX_CHAR_LENGTH:
+            mod_words.append(curr_word[0:MAX_CHAR_LENGTH])
+            curr_word = curr_word[MAX_CHAR_LENGTH:]
+        if len(curr_word) > 0:
+            mod_words.append(curr_word)
+
+    curr_broadcast = mod_words[0]
+    for word in mod_words[1:]:
+        if len(curr_broadcast) + 1 + len(word) <= MAX_CHAR_LENGTH:
+            curr_broadcast += " "
+            curr_broadcast += word
+        else:
+            broadcast_segments.append(curr_broadcast)
+            curr_broadcast = word
+    if len(curr_broadcast) > 0:
+        broadcast_segments.append(curr_broadcast)


Can't the module textwrap and textwrap.wrap be used instead?

sepalani · 2022-12-27T09:06:00Z

fmp_server.py

+        info.sender_name = pati.String("[MH3SP]")
+        data += info.pack()
+        data += pati.lp2_string(broadcast_text)
+        print("BROADCAST: " + broadcast_text)


Is this print necessary since the server could log any packets including broadcast ones?

sepalani · 2022-12-27T09:11:51Z

master_server.py

+    def broadcast_message(text):
+        # Function for easier broadcasting in interactive mode.
+        broadcast = FMP.construct_broadcast(text)
+        for broadcast_segment in broadcast:
+            broadcast_queues[FMP.BASE.cls.__name__].put(broadcast_segment, block=True)


Can't we iterate over the servers and call their broadcast_message method rather than "hijacking" their broadcast_queues?

sepalani · 2022-12-27T09:13:14Z

mh/server.py

+    def serve_forever(self, broadcast_queue):
+        self.broadcast_queue = broadcast_queue


Passing the broadcast_queue as a parameter seems overkill. Can't it be created in the server's constructor method?

sepalani · 2022-12-27T09:14:31Z

mh/server.py

            with self.selector as selector:
                selector.register(self, selectors.EVENT_READ)

+                event_watch = Timer()


It will do for the time being but a clock should be used. Having a single timer prevent handling multiple timed events.

kbarkevich changed the base branch from master to dev November 9, 2022 16:10

sepalani requested changes Nov 9, 2022

View reviewed changes

kbarkevich force-pushed the server-announcements branch from 88c4026 to 68e7942 Compare November 9, 2022 20:01

Added announcement support

2e3f0fa

Addressed Sepalani comments fixed issue where MAX_CHAR_LENGTH constant wasn't being used

kbarkevich force-pushed the server-announcements branch from 23cb573 to 2e3f0fa Compare November 9, 2022 20:16

sepalani requested changes Dec 27, 2022

View reviewed changes

		def serve_forever(self, broadcast_queue):
		self.broadcast_queue = broadcast_queue

Conversation

kbarkevich commented Nov 9, 2022

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!

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!

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!

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!

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!

2 participants