Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding support for the 36442 base build #187

Open
wants to merge 20 commits into
base: lotv
Choose a base branch
from

Conversation

StoicLoofah
Copy link

@GraylinKim I feel like an idiot: I had no idea how the different numbers in the s2protocol file corresponded to the types, and then I realized it was just a dictionary into the types. After that, it was easy to figure out.

For posterity (and anyone else who might want to contribute), I:

  1. took a diff between the last 2 s2protocol builds
  2. for new/different field, I went into sc2reader/readers.py and found where it fit in (probably just below the previous field)
  3. for that field, I took the number type from s2protocol, found another field with the same type, searched that field in readers.py, and copied that type for the new line and added the base_build check as well
  4. None of the fields were marked as optional, but if they were, I would have handled that
  5. for the event changes, I copied the last GameEventsReader and updated its parent. I tweaked it appropriately
  6. I registered the new GameEventsReader in sc2reader/resources.py

and it seemed like that worked.

Let me know if there's anything that I might have missed. I'm popping this into production for lotv.spawningtool.com right now, so I will let you know if I see any other errors.

@@ -579,6 +579,7 @@ def register_default_readers(self):
self.register_reader('replay.game.events', readers.GameEventsReader_26490(), lambda r: 26490 <= r.base_build < 27950)
self.register_reader('replay.game.events', readers.GameEventsReader_27950(), lambda r: 27950 <= r.base_build < 34784)
self.register_reader('replay.game.events', readers.GameEventsReader_34784(), lambda r: 34784 <= r.base_build)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to put an upper bound on this condition now. Same as the ones before it.

@GraylinKim
Copy link
Owner

I don't have any newer replays myself but you figured it out pretty well. Fix the comment above and I'd be happy to merge.

@StoicLoofah
Copy link
Author

Updated with the condition as you mentioned. If you want to play around with it yourself, here's a replay on this build http://lotv.spawningtool.com/903/download/

@StoicLoofah
Copy link
Author

I have been using my own fork in production, and it has been working here. @GraylinKim poking if you want to merge this into your branch?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants