Reconcile graylinkim/master and ggtracker/upstream #16
Conversation
* PacketEvent is now ProgressEvent. * SetToHotkeyEvent is now SetControlGroupEvent. * AddToHotkeyEvent is now AddToControlGroupEvent. * GetFromHotkeyEvent is now GetControlGroupEvent. * PlayerAbilityEvent is no longer part of the event hierarchy. * event.name is no longer a class property; it can only be accessed from an event instance.
People that want map attribute information can run the relevant s2gs files through and get proper mappings for their use case.
This way people can reliably catch this issue and deal with it as they wish.
Renames all ability events as following: * AbilityEvent -> CommandEvent * AbilityEvent -> BasicCommandEvent * TargetAbilityEvent -> TargetUnitCommandEvent * LocationAbilityEvent -> TargetPointCommandEvent As such, all references to these classes, statements that check the event name, and engine plugin event handlers need to be renamed. Its not ideal but it is much better than being wrong.
adding a missing change from the list
The dynamically created classes don't play well with pickle and were unncessarily complex. The only change here is that you can't use this anymore. unit._type_class.__class__.__name__ Instead you can use the shorter: unit._type_class.name No problem.
Remove registered readers/datapacks on picking. We won't need them anymore, the replay should already be loaded.
…_name decoding upgrade_type_name so that it reads as a string not bytes in python 3
Conflicts: CHANGELOG.rst sc2reader/data/__init__.py sc2reader/data/attributes.json sc2reader/engine/plugins/apm.py sc2reader/engine/plugins/context.py sc2reader/events/game.py sc2reader/events/tracker.py sc2reader/factories/plugins/replay.py sc2reader/readers.py sc2reader/resources.py sc2reader/scripts/sc2parse.py setup.py test_replays/test_all.py
|
I'm terrified of accepting this PR because it may have changed things that GGTracker depends on. I'll try it on https://github.com/dsjoerg/ggpyjobs now, but I expect the tests to fail and that it would be a lot of work to debug and deal with the issues. I expect that a more targeted PR would be much less work to integrated into GGTracker. Anyway I'll try it now with https://github.com/dsjoerg/ggpyjobs |
|
Yeah it dies instantly, the tests won't even run. |
|
yeah, there were some big refactors that happened in there that we would have to tackle, but that seems like a necessary price to pay for re-unifying the 2 forks. Even if we weren't combining them, it's something that we would have to tackle if the main line was maintained anyways, right? Will the refactor affect the ggtracker app more deeply than ggpyjobs? I don't mind going through and cleaning up references in there if that's the obstacle. I thinkn Graylin did a good job of documenting the renaming in the CHANGELOG, so I can just work off of that to get the tests passing there. On the plus side, I would note that although full crashes seem bad, I think they're pretty representative of the complexity here. If we fix those, then I don't think that we will have a lot of lingering issues |
|
i was imagining that instead of trying to port ggpyjobs to use the newer graylinkim sc2reader, it might be possible for spawningtool to be ported to use the older ggtracker-style sc2reader. because i'm lazy and don't want to do any work regarding unification. |
|
And to answer your question, ggpyjobs is the only part of ggtracker that uses sc2reader. If the ggpyjobs tests can pass then i'm pretty optimistic that the whole thing will work. |
|
Got it. Would you mind if I took a shot at ggpyjobs to see if I could get that working against this branch? |
|
@StoicLoofah go for it! It's open source, in fact all of ggtracker is, see https://github.com/ggtracker/ggtrackerstack But really if the tests in https://github.com/dsjoerg/ggpyjobs pass then I'm optimistic that the rest will work. |
|
Great! Thanks for your support and patience in getting this shipped. I'm also running it in production, and it appears to be working for me so far as well. Let me know when you mark a release version and get it on pypi so I can update references in my project and socialize the change as well. |
From a side discussion, we would like to make ggtracker/sc2reader the canonical, maintained repo for sc2reader because it is correctly patched for recent versions of the StarCraft 2 replay versions. However, it has fallen behind GraylinKim/sc2reader while moving ahead in other directions, so this is my attempt to resolve all merge conflicts.
Originally, I ran into 3 issues when trying to use ggtracker/sc2reader instead of StoicLoofah/sc2reader/lotv for StoicLoofah/spawningtool
It looks like PIL is a requirement for this build but was not as part of the original sc2reader. This is more than a requirements update normal issue because on my ubuntu 16.04 instance, I had to install the libjpeg package separately to get pillow to install properly.
"replay.gateway" was renamed to "replay.region", which is what I'm using GraylinKim@0e6286f
i needed this fix GraylinKim@c5d22d6
Rather than just patch those issues, I went ahead and tried to merge all of GraylinKim/sc2reader, which appeared to work. All of the tests are passing after resolving the merge conflicts (in the merge itself) and making a few small patches to the tests.
Overall, it was pretty straightforward, with 2 notable refactors:
Unless you see a reason why this is a bad idea, I will plug this into production for Spawning Tool and see if anything crops up.
Also flagging @GraylinKim in case you have any thoughts on this.
One more thing: props to you guys on this library. I have only lightly dabbled with it, but I am always impressed by the scope when I have to poke into it like this. Great work.