Time calculations incorrect

[dsjoerg]

From @kpaleniu on January 1, 2016 17:8

The way time is calculated from LotV replays seems to be incorrect. See the following replay. The replay is actually 14:47 long. The graph on the main page says the match lasted 20:43. Is this a known issue?

Copied from original issue: dsjoerg/ggtracker#16

[dsjoerg]

Yes it is, thanks for reporting it. There are a set of time-related issues that were introduced by 3.0 / LotV, including some bugs inside the SC2 client itself. We are looking for someone who would like to dig in to the time issues and fix them!

[dsjoerg]

From @Anzumana on January 9, 2016 2:31

If the issue is still unresolved when my semester break starts i will take a look.

[dsjoerg]

@anzumana great!

[dsjoerg]

Here's a summary of the time issues.

Before 3.0, 1.4 Starcraft minutes = 1 real minute = 960 game frames (loops). After 3.0 / LotV, 1 Starcraft minute = 1 real minute = 960 game frames (loops).

All of GGTracker was built assuming that 960 game frames / loops = 1.4 Starcraft minutes. So anything related to time in GGTracker is off by a factor of 1.4. That includes game length, APM and income.

To fix it, broadly, the steps are:

• set up a development environment with the full GGTracker system: sc2reader, ggpyjobs, gg, esdb, ggtracker
• make a targeted set of code changes; maybe 20-30 lines in all
• test that the resulting system works properly

[dsjoerg]

If someone is interested in giving this a go, I can set up a Dockerfile that has all of sc2reader, ggpyjobs, gg, esdb, ggtracker running so that you can get started more easily and test whatever changes need to be made.

[dsjoerg]

From @agwblack on April 7, 2016 15:33

I may be able to help. If you could provide the dev environment then that would be great!

[dsjoerg]

OK @agwblack, will do and will ping you when it's ready

[dsjoerg]

From @Anzumana on April 10, 2016 14:29

Won't be able to contribute as i was planning on. Hope you guys can fix it. Have a good one.

[dsjoerg]

From @gravelweb on April 10, 2016 22:33

I use your tool a lot, and am interested in contributing if i can. If you would provide a docker or vagrant dev environment, I'm happy to help speed this along. Getting this issue and the mini-map issue resolved would be totally great.

[dsjoerg]

Hey @gravelweb great news. @nickelsen is working on a vagrant setup, see https://github.com/dsjoerg/ggtracker/pull/20

And your catch about 1v1 vs other game modes makes this one quite killable.

I'll take a look and see if I can narrow down where the bug is happening.

[dsjoerg]

From @gravelweb on April 11, 2016 14:37

@dsjoerg That's great! A vagrant setup would be really encouraging for new devs on this project.

[dsjoerg]

@gravelweb once you've got ggtrackerstack working in your dev environment, we can talk more specifically about how to make the time-related changes / fixes. Let me know when you're ready?

The first design question is whether we care about preserving sc2reader / ggtracker correctness for pre-3.0 or pre-3.2 replays. Of course, the work is trickier if we do.

[dsjoerg]

From @gravelweb on April 13, 2016 13:31

@dsjoerg Sounds good. I still need to get settled in with my dev environment and work out the kinks. I'll probably take the rest of the week to do that.

[dsjoerg]

From @gravelweb on April 14, 2016 12:18

@dsjoerg Wondering about version and expansion support for ggtacker.

1. Do WoL and HotS replays need to be supported? If the graphs change for LotV (like the chronoboost stat), the old graphs should be kept around if the detected expansion is one of those two.
2. I'm not sure if WoL and HotS were affected by the game time change in 3.0.0.
   • if they were not affected, should ggtracker continue to support the old game time?
   • if they were affected, should ggtracker have support for pre 3.0.0 replays?

[dsjoerg]

@gravelweb ideally we support all versions correctly.

However, I don't want the perfect to be the enemy of the good. I figure that first we'll get things working for the current version, and break all previous versions. Then, if there's interest, we can go back and add backwards compatibility.

[dsjoerg]

From @gravelweb on April 14, 2016 12:59

@dsjoerg well put. something to consider, but not to prioritize. I suppose we should be able to start tackling this issue shortly

[dsjoerg]

Before the fixes done in this issue can be released to production, two serious problems: 1) the combat regions that are highlighted on all the charts are now misaligned 2) the last ~40% of the APM chart is cut off

To see this for yourself, upload a replay from GGTracker onto your ggtrackerstack and look at the two pages side by side.

Once these are fixed I'll deploy everything to production and re-generate the maps; the map fixes will go out at the same time.

[dsjoerg]

By the way @agwblack dev environment is ready!

[dsjoerg]

Remaining work before the improvements can roll into production: 1) spending skill leagues are very different for a replay between dev and prod. not sure if that's because of time changes or because of data diffs between prod and dev. 2) econ badges are totally missing in dev.

I'll take a first whack at these.

[dsjoerg]

Econ badges are missing because the econ badges are computed based on benchmarks computed from existing replays in the DB. See esdb/games/sc2/match/entity.rb, compute_saturation_skill_benchmarks()

[dsjoerg]

OK, the spending skill change is a real problem. Spending skill is computed by using historical matches, and doing a lookup that includes the match length. With this change we are changing the meaning of match length.

[dsjoerg]

Easy hack might be to introduce an adjustment factor here: https://github.com/dsjoerg/esdb/blob/master/esdb/games/sc2/match/entity_summary.rb#L146

and here: https://github.com/dsjoerg/esdb/blob/master/Rakefile#L122 (and to the two other references to duration_seconds on lines 136-137)

@nickelsen what do you think?

The essence of the hack proposal is to keep the replays_sq_skill_stat table in "old minutes", to minimize the amount of changes required.

I suppose the alternative is to convert the data to "new minutes" in a DB migration. I like the hack proposal because it's a bit simpler. In particular I'm not sure what happens with the existing data and if it's handled correctly if we convert it all, the 5 to 30 minute thing... gotta go back later

[dsjoerg]

After further reflection I think the hack proposal is the way to go, so that we can get all this good new code out into production.

The main liabilities of this hack can be addressed in a separate piece of work: 1) pages such as this will be totally wrong as they are still using "old minutes" 2) the code itself will be confusing to people since it will be using old minutes

however, rather than trying to fix these issues in this release, i think it's better to do the "easy hack", deploy, and address those issues in a separate piece of work.

[dsjoerg]

on further thought, hacking the code there isn't the right way to do the hack. instead i need to adjust the existing data with a migration, and then fix the spending skill code so that it works.

[nickelsen]

We could also revert the time-change I did in sc2reader to fix duration_seconds. Then everything in esdb is still in HotS seconds and spending skill should not be affected. Then we 'just' need to recalculate match duration where it is shown. This might be an easier fix for the time issues, although, I haven't tried it, so I'm not sure if it impacts the stat-graphs or other elements that we already fixed - it just seems that we anyway had to fix the graphs even after fixing duration_seconds, so impact of reverting might not be too big.

The general strategy would then be separate concerns and to keep everything in HotS seconds in ggpyjobs and esdb and only convert to LotV seconds in ggtracker when we show stuff. Do you think such a strategy could fly?

[dsjoerg]

Yes @nickelsen I think that is also a viable strategy. However, I think that my immediately-previously stated strategy is the easiest way to get something out and deployed and running right now.

[nickelsen]

Great, let's go with that then. 👍

[dsjoerg]

OK, I've tested spending skill in the testing environment and I'm satisfied that it's going to work OK. Tomorrow I'l do a deploy to production. My plan:

• manually: * convert LOTV matches to have the correct (shorter) duration (update m.duration_seconds for all matches since 2015-11-09) * recompute spending skill table based on those durations (manually run the SQL) \ get api.ggtracker.com/api/v1/spending_skill/should_nuke to clear redis
• later, when everything else is confirmed to be working OK, manually blow away map DB rows so that their bounds are re-computed and then minimaps will work for newly uploaded replays.

[nickelsen]

Great - that's not an easy task! Let me know right away if I need to help or look into something.

[dsjoerg]

Deployed and working in production! Thanks & congrats @nickelsen and @gravelweb.

[nickelsen]

Awesome job @dsjoerg - thanks for all the help and introductions to the code base!

[gravelweb]

Nice job @nickelsen. Awesome fixes these past few weeks :+1:

[nickelsen]

You too, @gravelweb - super nice with the install script for the stack too!