Added event quest rotation by kbarkevich · Pull Request #111 · sepalani/MH3SP

kbarkevich · 2023-07-03T03:50:56Z

This PR adds the foundation for an event/arena quest cycle, with a new list of event/arena quests able to be scheduled to be sent each day. Currently, the event cycle contains 14 unique temporal slots, for 14 days of available scheduling. This is in order to keep the schedule aligned with the Sandstorm event, which is also on a 14-day cycle.

InusualZ · 2023-07-04T14:28:28Z

LGTM

I do think that we need to stop hard coding the quests in code. We should come up with a system that enable us to define quest from a external file (and maybe even hot reload?)

sepalani

I'm not a huge fan of the way this is handled as the "temporal slots" are hardcoded and hooking more things (and lambda) into a not flexible already hardcoded system doesn't feel clean at all.

In other words, we shouldn't hardcode quests, nor its rotation system, nor tight it to the jhen event. Ideally, we should be able to change these settings without rebooting the server.

I'm open to suggestions regarding how this can be implemented. I've been thinking about hosting binary quest files and using a config file to schedule which quest files should be served.

InusualZ · 2023-07-05T16:25:55Z

I agree with you, but most if not all the requirement that you set would require a fairly robust config system. If we are going to "stop" the progression of game <-> server feature we should start asap trying to figure out the direction that we want to go

sepalani

Some comments regarding the code. Could you please also address the linter issues.

sepalani · 2023-10-03T16:32:31Z

other/utils.py



-def create_server(server_class, server_handler,
+def create_server(server_class, server_handler, binary_loader,


Can't the binary_loader be loaded from its module (using the module as singleton and/or providing a get_instance) when needed?

It's currently initialized a single time in master_server.py and a reference to that object is passed down to the children that require it. Since it's stateful (currently in that it tracks an incrementing version number, and in the future in that we might have it on an auto refresh timer to check for updated files periodically) I'd rather do it this way.

If its purpose is to be initialised once, then you should use the singleton pattern rather than passing it to every functions. I also suspect that it introduced a regression as server can't be launched independently anymore (e.g. python fmp_server.py).

sepalani · 2023-10-03T16:40:05Z

dump_quest.py

+http://divinewh.im/q/c/Grudge_Match:_Bird_and_Brute
+"""
+
+from mh.quest_utils import *


Please address this linter comment.

sepalani · 2023-10-03T16:40:27Z

dump_quest.py

@@ -0,0 +1,2937 @@
+"""Quest dumper.


This is missing a shebang and the copyright notice.

sepalani · 2023-10-03T16:44:50Z

dump_quest.py

+def EXPORT(quest):
+    import json
+    name = quest['quest_info']['name'].replace(':', '')
+    with open('event/{}.json'.format(name), "w") as outfile:
+        json.dump(quest, outfile, indent=4)
+
+
+QUESTS = []


This can be done in a main function. Creating a QUEST list isn't necessary here. Its creation can be deferred later when all quests are created. locals() and a list/dict comprehension (or similar) can be used if you want the quests to be automatically included in a list.

This has been fixed

sepalani · 2023-10-03T16:49:49Z

dump_quest.py

+for q in QUESTS:
+    EXPORT(q)


Please use if __name__ == "__main__" if the intended behaviour for this file is to be run as a script. Otherwise, this behaviour will be present if this is imported. You might also want to create a main function and use the argparse module if you intend to add more options to this script.

This has been fixed, as with above

sepalani · 2023-10-03T18:18:34Z

mh/pat.py

+        version = binary["version"]
+        if callable(version):
+            version = version(self.server.binary_loader)
+        content = get_pat_binary_from_version(binary_type, version)
        if callable(content):
-            content = content()
-        data = struct.pack(">II", binary["version"], len(content))
+            content = content(self.server.binary_loader)
+        data = struct.pack(">II", version, len(content))


Can't this logic/part be offloaded to the binary_loader?

Not all binaries currently come from the binary_loader, only the ones that have the potential to change from moment to moment. In other words yes, it could be moved (and probably should, once we have things like a rotating trading post), but since this PR is specifically about the event quests, I'll leave that to a future update.

I see, that makes sense.

sepalani · 2023-10-03T18:20:40Z

mh/quest_utils.py

+class QuestLoader:
+    def __init__(self, file_loc='event/quest_rotation.json'):
+        self.version = 0
+        self.version_incrementer = 28


Is this an arbitrary constant?

This gives us the ability to expand the quest rotation to up to 4 weeks (7 * 4 days) per update.

You might want to offload a version, base_version or whatever to the quest_rotation.json as it shouldn't be hardcoded.

sepalani · 2023-10-03T18:21:53Z

mh/quest_utils.py

+        self.load_quests()
+        self.version = 0
+
+    def load_quests(self, file_loc=None):


Is file_loc ever used?

No, it has been removed

sepalani · 2023-10-03T18:25:46Z

mh/quest_utils.py

+    def get_quest(self, idx):
+        return self.quests(idx)


Is this ever used?

Isn't self.quests a list rather than a callable?

This has been removed

sepalani · 2023-10-03T18:28:44Z

mh/quest_utils.py

+        trueIdx = idx - 1
+        return [self.quests[trueIdx][j][1] for j in range(len(self.quests[trueIdx]))]
+
+    def __len__(self):
+        # Get number of days
+        return len(self.quests)
+
+    def __getitem__(self, idx):
+        # Get quests for the day
+        trueIdx = idx - self.version - 1
+        print("idx is {}".format(trueIdx))
+        print("idx: {}, len is {}".format(trueIdx, len(self.quests[trueIdx])))
+        return [self.quests[trueIdx][j][0] for j in range(len(self.quests[trueIdx]))]


Please use snake_case.

Can't enumerate be used instead of range(len(...))?

These debug prints should be removed.

…ates and .json files for each quest Removed unneeded default parameter Finalized Jump Four Jaggi, added tentative Rage Match, and added more to the quest formatting system. Added "Flooded Forest Free-For-All" and fixed bug Changed quest rotation system to a .json-based system that's refreshable via the interactive (-i) menu Added bytes for small monster wave transition criteria Added ability to programatically add small monsters Added more programatically-added small monster information Addressed Sepalani comments

sepalani

Here is a cursory review. The PR can't be run on python3 nor a single server, only master_server is working.

sepalani · 2024-01-08T15:50:24Z

mh/pat.py

        self.info("Running on {} port {}".format(*address))
        self.debug_con = []
        self.debug_mode = debug_mode
+        self.binary_loader = binary_loader


Can't the binary_loader be instantiated here rather than passing as parameter to all of its parent functions (however, I'd rather prefer using a singleton if possible)?

from mh.quest_utils import QuestLoader # ... self.binary_loader = QuestLoader("event/quest_rotation.json")

sepalani · 2024-01-08T16:31:35Z

dump_quest.py

+    BowgunFrame, BowgunStock, BowgunBarrel
+
+
+def EXPORT(quest):


Function names should be snake_case'd. Feel free to use a more explicit name such as export_to_json or similar.

sepalani · 2024-01-08T16:33:29Z

dump_quest.py

+    QUESTS = []
+    QUESTS.append(QUEST_EVENT_JUMP_FOUR_JAGGI)
+    QUESTS.append(QUEST_EVENT_THE_PHANTOM_URAGAAN)
+    QUESTS.append(QUEST_EVENT_BLOOD_SPORT)
+    QUESTS.append(QUEST_EVENT_MERCY_MISSION)
+    QUESTS.append(QUEST_EVENT_FF_FREE_FOR_ALL)
+    QUESTS.append(QUEST_EVENT_RAGE_MATCH)
+    QUESTS.append(QUEST_EVENT_WORLD_EATER)
+    QUESTS.append(QUEST_EVENT_WHERE_GODS_FEAR_TO_TREAD)
+    QUESTS.append(QUEST_EVENT_GREEN_EGGS)
+    QUESTS.append(QUEST_EVENT_JUMP_FOURTY_EIGHT_JAGGI)
+    QUESTS.append(GRUDGE_MATCH_ROYAL_LUDROTH)
+    QUESTS.append(GRUDGE_MATCH_BIRD_BRUTE)
+    QUESTS.append(GRUDGE_MATCH_TWO_FLAMES)
+    QUESTS.append(GRUDGE_MATCH_LAND_LORDS)


Suggested change

QUESTS = []

QUESTS.append(QUEST_EVENT_JUMP_FOUR_JAGGI)

QUESTS.append(QUEST_EVENT_THE_PHANTOM_URAGAAN)

QUESTS.append(QUEST_EVENT_BLOOD_SPORT)

QUESTS.append(QUEST_EVENT_MERCY_MISSION)

QUESTS.append(QUEST_EVENT_FF_FREE_FOR_ALL)

QUESTS.append(QUEST_EVENT_RAGE_MATCH)

QUESTS.append(QUEST_EVENT_WORLD_EATER)

QUESTS.append(QUEST_EVENT_WHERE_GODS_FEAR_TO_TREAD)

QUESTS.append(QUEST_EVENT_GREEN_EGGS)

QUESTS.append(QUEST_EVENT_JUMP_FOURTY_EIGHT_JAGGI)

QUESTS.append(GRUDGE_MATCH_ROYAL_LUDROTH)

QUESTS.append(GRUDGE_MATCH_BIRD_BRUTE)

QUESTS.append(GRUDGE_MATCH_TWO_FLAMES)

QUESTS.append(GRUDGE_MATCH_LAND_LORDS)

QUESTS = [

QUEST_EVENT_JUMP_FOUR_JAGGI,

QUEST_EVENT_THE_PHANTOM_URAGAAN,

QUEST_EVENT_BLOOD_SPORT,

QUEST_EVENT_MERCY_MISSION,

QUEST_EVENT_FF_FREE_FOR_ALL,

QUEST_EVENT_RAGE_MATCH,

QUEST_EVENT_WORLD_EATER,

QUEST_EVENT_WHERE_GODS_FEAR_TO_TREAD,

QUEST_EVENT_GREEN_EGGS,

QUEST_EVENT_JUMP_FOURTY_EIGHT_JAGGI,

GRUDGE_MATCH_ROYAL_LUDROTH,

GRUDGE_MATCH_BIRD_BRUTE,

GRUDGE_MATCH_TWO_FLAMES,

GRUDGE_MATCH_LAND_LORDS

]

sepalani · 2024-01-08T17:04:25Z

mh/quest_utils.py

+class Enum(object):
+    def __getitem__(self, idx):
+        return getattr(self, idx)


This doesn't seem necessary and is very error prone:

You're hiding the name of the Enum derived classes you instantiated (but you forgot to instantiate WaveType which seems to be an oversight)

You can do Monster.barroth which should autocomplete and check on modern IDE whereas Monsters['barroth'] won't.

It doesn't seem worth it since you will have to parse the json anyway, so you'll need to check every places where the string refers to a resource (monster, location, ...), then get its value (which is single line of code no matter which way you choose: calling getattr or the brackets operator on the enum instance). Worse your current method add one line of code per enum which is overkill.

I have nothing against enum, python3 also has its own enum module. Similar implementations also exist in python2 and use a cleaner approach. Regardless, I don't think the approach you're suggesting is worth its overhead, especially if it's not currently used.

sepalani · 2024-01-08T17:06:54Z

mh/quest_utils.py

+def byteify(input):
+    if isinstance(input, dict):
+        return {byteify(key): byteify(value)
+                for key, value in input.iteritems()}
+    elif isinstance(input, list):
+        return [byteify(element) for element in input]
+    elif isinstance(input, unicode):
+        return input.encode('utf-8')
+    else:
+        return input


This will fail on python3 as reported by Ruff.

IIRC, comparing str and unicode should be safe, so I don't see why such a conversion is needed as you should be able to get your json data properly regardless.

sepalani · 2024-01-08T17:48:46Z

mh/pat.py

+        version = binary["version"]
+        if callable(version):
+            version = version(self.server.binary_loader)
+        content = get_pat_binary_from_version(binary_type, version)
        if callable(content):
-            content = content()
-        data = struct.pack(">II", binary["version"], len(content))
+            content = content(self.server.binary_loader)
+        data = struct.pack(">II", version, len(content))


I see, that makes sense.

sepalani · 2024-01-08T17:54:07Z

mh/quest_utils.py

+class QuestLoader:
+    def __init__(self, file_loc='event/quest_rotation.json'):
+        self.version = 0
+        self.version_incrementer = 28


You might want to offload a version, base_version or whatever to the quest_rotation.json as it shouldn't be hardcoded.

sepalani · 2024-01-08T17:57:17Z

other/utils.py



-def create_server(server_class, server_handler,
+def create_server(server_class, server_handler, binary_loader,


If its purpose is to be initialised once, then you should use the singleton pattern rather than passing it to every functions. I also suspect that it introduced a regression as server can't be launched independently anymore (e.g. python fmp_server.py).

mh3fan · 2024-05-09T12:39:45Z

Hello, first of all thank you for setting up this MH3SP project and for bringing this magnificent game back to life, I don't know if I'm in the right place to talk about this but a lot of important event quests are missing, let me allow myself so to inform you that I have just found "all" the event quests of the game at this link : (https://monsterhunter.fandom.com/wiki/MH3:_Event_Quests#Rage_Match) hoping that this will help you, (I could possibly translate the quests into French), unfortunately I cannot "yet" help you with the development because I am learning, therefore my skills will not be sufficient, in any case good luck to you and thank you again, hoping that the information will be transmitted ^^

InusualZ approved these changes Jul 4, 2023

View reviewed changes

sepalani requested changes Jul 4, 2023

View reviewed changes

InusualZ mentioned this pull request Jul 6, 2023

[RFC] Config Overhaul #112

Open

kbarkevich force-pushed the event-quest-rotation branch 2 times, most recently from cc7701c to 3bc8e5e Compare September 16, 2023 18:09

sepalani requested changes Oct 3, 2023

View reviewed changes

kbarkevich force-pushed the event-quest-rotation branch from 7ea1320 to 96b52b6 Compare January 7, 2024 21:41

kbarkevich force-pushed the event-quest-rotation branch from 96b52b6 to 1a17585 Compare January 7, 2024 21:55

Fixed linter issue

88c60b4

sepalani requested changes Jan 8, 2024

View reviewed changes



		def create_server(server_class, server_handler,
		def create_server(server_class, server_handler, binary_loader,

Conversation

kbarkevich commented Jul 3, 2023

Uh oh!

InusualZ commented Jul 4, 2023

Uh oh!

sepalani left a comment

Choose a reason for hiding this comment

Uh oh!

InusualZ commented Jul 5, 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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mh3fan commented May 9, 2024

Uh oh!

Reviewers

Assignees