search

GraylinKim / sc2reader

A python library that extracts data from various Starcraft II resources to power tools and services for the SC2 community. Who doesn't want to hack on the games they play?
http://sc2reader.readthedocs.org
MIT License
411 stars 85 forks source link

Adds coherence to the upgrade names as a first step. #173

Open EHadoux opened 10 years ago

EHadoux commented 10 years ago

I started with giving coherence to all the upgrade names such that: Upgrade{Race}{Type}Level{n}. This impacts BasicCommandEvent.ability_name. This change reflects UpgradeCompleteEvent names for the upgrades. However, some names have to be changed in the later, like ShieldWall or PunisherGrenades because the name in ability_lookup.csv (and thus in BasicCommandEvent.ability_name) is more suitable. Where can I do that? Maybe do not merge this pull-request until this and further tests have been done.

I also fixed a typo for the file to be displayed properly in Github (with the nice table and stuff like that).

GraylinKim commented 10 years ago

You'll notice that the change in names has caused some tests to fail, if it is necessary to change tests to keep them passing with these changes please do so. Because these sorts of changes are not backwards compatible it would be far better if they were done in one large, complete, update. I'll hold on merging anything to master until we have something that resembles a complete solution.

There is currently no mapping for UpgradeCompleteEvent names. You will need to create one, load it with the rest of the data, and add logic that fixes the names to the UpgradeCompleteEvent processing. Normally when I do such things I will leave the original name intact on the event and have a separate field with the corrected or normalized value.

EHadoux commented 10 years ago

You'll notice that the change in names has caused some tests to fail, if it is necessary to change tests to keep them passing with these changes please do so

Yep, the log on Travis tells that the problem come from the Coveralls installation. No sc2reader tests have failed.

Because these sorts of changes are not backwards compatible it would be far better if they were done in one large, complete, update. I'll hold on merging anything to master until we have something that resembles a complete solution.

I completely agree with that, hence my question you answer to after.

Normally when I do such things I will leave the original name intact on the event and have a separate field with the corrected or normalized value.

I'll do this that way so. Thanks :)

EHadoux commented 10 years ago

I've test it against all the replays from the last DreamHack.

EHadoux commented 10 years ago

I'm running it on all the WCS replays. Please wait until it is finished to merge.

EHadoux commented 10 years ago

This commit passes all the replays from the WCS.

GraylinKim commented 10 years ago

Can you talk about what sort of testing your "running it on all the WCS replays" entails? Is there some specific script you are running to make sure all the mappings make sense or are you just trying to weed out any unrelated bugs in the upgrade handling?

I have two remaining concerns re: your pull request.

  1. I think it would be good to move the normalized lookup to the data package along with the other lookups. Even better would be to create an UpgradeClass in the style of the Ability class and add methods to the Build class for looking them up. This way we could include upgrade cost data in the project as well. If you aren't interested in going that far that's fine, just thought I'd mention it while we are here.
  2. These normalized names have never been part of the documentation. If you had some ideas on how it might be best to include the names that sc2reader uses in the documentation I'd like to hear them. I think having a couple pages listing out what strings people should look for in game data would be much better than having them browse the source. Since we are making changes to this layer now it'd be nice if we could supply documentation as well. I think having a couple pages listing out what strings people should look for in game data would be much better than having people search through the data files in our source tree.

Otherwise, thanks for the effort @EHadoux. I'll take a look at your pull request in more detail in the next couple days.

EHadoux commented 10 years ago

Can you talk about what sort of testing your "running it on all the WCS replays" entails? Is there some specific script you are running to make sure all the mappings make sense or are you just trying to weed out any unrelated bugs in the upgrade handling?

You're right I forgot that. I have tested that every finishing upgrade matches an upgrade that have been launched earlier.

I think it would be good to move the normalized lookup to the data package along with the other lookups.

I've tried to do that but I didn't manage to find a way to get the dict from outside this file (I'm rather new at Python).

Even better would be to create an UpgradeClass in the style of the Ability class and add methods to the Build class for looking them up. This way we could include upgrade cost data in the project as well. If you aren't interested in going that far that's fine, just thought I'd mention it while we are here.

No problem, I'll try that but I cannot guarantee you a result. It will worth the effort but maybe in a second time.

These normalized names have never been part of the documentation. If you had some ideas on how it might be best to include the names that sc2reader uses in the documentation I'd like to hear them. I think having a couple pages listing out what strings people should look for in game data would be much better than having them browse the source. Since we are making changes to this layer now it'd be nice if we could supply documentation as well. I think having a couple pages listing out what strings people should look for in game data would be much better than having people search through the data files in our source tree

Ok, i'll add that.

EHadoux commented 10 years ago

I will definitely add an upgrade object to have the time until completion because you cannot really know when an update is started if it is queued (unless you know some way). However, i'll do this a bit after, in an other pull request.

coveralls commented 10 years ago

Coverage Status

Coverage increased (+0.08%) when pulling 33b011f602b62c5c452c7a23c064b50b8ca8bcde on MakozFriends:hotfix/upgradenames into 1a6220be30dc07268d83ace5f7ee20aa482a49af on GraylinKim:master.

EHadoux commented 10 years ago

Unless other comments, I have no other commit to do in this pull request.

GraylinKim commented 10 years ago

Hi, sorry, not ignoring you. I am currently moving to a new apartment and can't look at this right now. I'll review and merge as soon as possible, thanks for the work!

EHadoux commented 10 years ago

No worries, thank you :)

EHadoux commented 10 years ago

Hi, feel free to change some stuffs in the docs to comply with your standards :)

coveralls commented 10 years ago

Coverage Status

Coverage increased (+0.08%) when pulling c333e6e59d5465a76c3b802dfe1b8c99bf983c50 on MakozFriends:hotfix/upgradenames into 1a6220be30dc07268d83ace5f7ee20aa482a49af on GraylinKim:master.

coveralls commented 10 years ago

Coverage Status

Coverage increased (+0.39%) when pulling a312c9a558f7c1499269e9cf3caca71b4b3ef3f0 on MakozFriends:hotfix/upgradenames into c860d8e23d6fbd3a9258deff9fcbe8796da6c900 on GraylinKim:master.

EHadoux commented 10 years ago

I just rebase the branch with the last commits from @dsjoerg. However, I'm not sure about the doc (the table), I cannot compile it ATM.