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

Unit Composition plugin #164

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

StoicLoofah
Copy link

Hopefully it's fairly straightforward about what the approach. I have tested it some (and actually looked at the replay to verify), but I'm thinking there are probably still edge cases out there to consider.

Graylin, I'm thinking that you probably have a better sense for what these edge cases are regarding unit types and such.

Hopefully this plugin is generally useful for others. I know other sites are already extracting compositions in their own way, but it would be kind of nice to have a standard method for doing it.

@StoicLoofah
Copy link
Author

One other approach I tried was to actually go event by event. It mostly worked but ended up messy, and I switched to this approach at your suggestion. If this approach seems bad, I can always fall back to that

@StoicLoofah
Copy link
Author

Now that I think about it, it might also be arguable that the out should key on unit types, not strings. I ended up doing it this way because this is ultimately what I want, but send it back if you want that changed and think users should rely on their own packages (like spawningtool) to get it the rest of the way.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.27%) when pulling e24a042 on StoicLoofah:composition-plugin-history into 52e5be9 on GraylinKim:master.

@GraylinKim
Copy link
Owner

You broke the build! Likely it is because the code isn't Python3 compatible; all new contributions need compatibility. I advise putting the following at the top of every file so that you can catch many of these issues while running python2.7:

from __future__ import absolute_import, print_function, unicode_literals, division

You also can't use iteritems() anymore. It is just items() now. There may be other issues but those are the ones I can see from the diff.

As for the plugin itself, I'll have think on it a bit more. My current thoughts though:

  • I'm not sure that I like the approach of polling for unit counts on a given interval.
  • Its been a while since I've been down in the weeds on sc2reader data but I think you did a good job covering edge cases.
  • A white list would be much more effective than a blacklist for keeping unintended types out.

I'm trying to work on the upcoming patch so it may be a while before I really get around to this; fyi.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.36%) when pulling 85eb7c2 on StoicLoofah:composition-plugin-history into 52e5be9 on GraylinKim:master.

@StoicLoofah
Copy link
Author

Thanks for the heads up on python 3. I tried to tackle all of the ones I could easily from my python2.7 environment. In addition to the things you mentioned, I also needed to use // for some int division.

With regards to polling, the other approaches I considered were to either update a new composition at every change or to add a function that would spit out the composition at a given time. I didn't get with the former because that would take a lot of space, and I didn't do the latter because it didn't fit well with what I'm hoping to do with the data and, if used, would be much less efficient. Even so, I'm open to ideas or other ways of getting it.

If you think a white list would be better, I'm down for that, too. I actually have mappings and filters for getting unit types into readable types at each level of my application, so I really have no idea what the best approach is.

No rush on reviewing: I have procrastinated on writing this forever anyways. Let me know if you have thoughts on other things you would like to see me fix on this pull request.

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