Skip to content

Comments

Add support for build version 53644 replays#26

Merged
dsjoerg merged 4 commits intoggtracker:upstreamfrom
frugs:build_53644
Jun 15, 2017
Merged

Add support for build version 53644 replays#26
dsjoerg merged 4 commits intoggtracker:upstreamfrom
frugs:build_53644

Conversation

@frugs
Copy link
Contributor

@frugs frugs commented Jun 9, 2017

@dsjoerg Please look over some of the changes made to abilities_lookup.csv. I have overwritten some of the previous command names with ones based on the command ids in the balance data export. This provides better overall naming consistency, at the cost of potentially breaking clients that are dependent on the pre-existing names.

However, given the command names have been broken for several months (in fact, quite possibly over a year) already, I doubt that any such clients actually exist.

I will add PR comments on some of the changed lines to make it easier for you to locate them.

BanelingNestResearch,EvolveCentrifugalHooks,EvolveTunnelingJaws,,,,,,,,,,,,,,,,,,,,,,,,,,,,,CancelBanelingNestResearch,
BansheeCloak,CloakBanshee,DecloakBanshee,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,
BanelingNestResearch,ResearchCentrificalHooks,EvolveTunnelingJaws,,,,,,,,,,,,,,,,,,,,,,,,,,,,,CancelBanelingNestResearch,
BansheeCloak,On,Off,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,
Copy link
Contributor Author

@frugs frugs Jun 9, 2017

Choose a reason for hiding this comment

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

Overwritten old command name with command id from data export

AttackWarpPrism,AttackWarpPrism,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,
BanelingNestResearch,EvolveCentrifugalHooks,EvolveTunnelingJaws,,,,,,,,,,,,,,,,,,,,,,,,,,,,,CancelBanelingNestResearch,
BansheeCloak,CloakBanshee,DecloakBanshee,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,
BanelingNestResearch,ResearchCentrificalHooks,EvolveTunnelingJaws,,,,,,,,,,,,,,,,,,,,,,,,,,,,,CancelBanelingNestResearch,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Overwritten old command name with 'research' prefix rather than 'evolve' prefix. This reflects a change in the structure of the balance data export.

@dsjoerg
Copy link
Member

dsjoerg commented Jun 9, 2017

As long as these changes don't break GGTracker, I'll take them. This weekend or tonight I'll work the changes through the tests and see how they do.

@frugs
Copy link
Contributor Author

frugs commented Jun 13, 2017

Noticed some related open issues:
GraylinKim#88
GraylinKim#129

From the discussion in GraylinKim#88, it seems like preferring ability names that match to Blizzard's internal names for them (as I have implemented) was the preferred option to migrate towards.

@dsjoerg
Copy link
Member

dsjoerg commented Jun 14, 2017

Oooh actually this will cause a problem for ggtracker.
For example see https://github.com/dsjoerg/ggpyjobs/blob/a1d9f7d87609980c792facbd08eff5ed57c30406/sc2parse/plugins.py#L720

The existing unit tests didnt catch this problem, I'm worried now about the best way to test/validate this before pushing it into production and fielding complaints from users...

From my perspective the easiest way forward would be to not change the name of any of the existing commands that were previously correct. @frugs would that work for you?

@frugs
Copy link
Contributor Author

frugs commented Jun 15, 2017

@dsjoerg I should be able to modify the generation script such that old names are preserved. I'll update this pull request with the newly generated files. If they past testing, I'll submit a new pull request for the modifications I had to make to the generation script.

@frugs
Copy link
Contributor Author

frugs commented Jun 15, 2017

ability_lookup.csv now preserves the original command names if a commands already exist under the ability name with a matching command index. There are still some commands that are keyed with new ability names, however, so these ones may still be problematic. Ones to watch out for in particular are the command to create a Nuke at a Ghost Academy and all Evolution Chamber upgrades.

@dsjoerg
Copy link
Member

dsjoerg commented Jun 15, 2017

Thanks @frugs !! Looks good to me. I'm going to push this into GGTracker production and we'll see if it causes problems. (The unit tests were already passing, so if there's still a problem, prod is the way to find it now)

@dsjoerg dsjoerg merged commit 944c9a0 into ggtracker:upstream Jun 15, 2017
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.

2 participants