Skip to content

Conversation

nickelsen
Copy link
Contributor

This seems to fix the army chart current time for lotv replays, while keeping backward compatibility.

I'm not strong enough in Angular to understand how I manually test the timeToFrame function, so I don't know if that it working correctly yet.

@dsjoerg could you point in the right direction?

@gravelweb
Copy link
Contributor

Looks like there's a mismatch now between the army graph and the stats graphs, which could be a little confusing.

@nickelsen
Copy link
Contributor Author

Nice catch @gravelweb - I didn't notice that at all. All other charts than the army chart seem to be cut off instead of scaled. I do, however, it is due to the change in ggtracker/sc2reader#10 and not the one in this PR, so maybe the length fix was too good to be true. I'll investigate some more.

@dsjoerg
Copy link
Owner

dsjoerg commented Apr 17, 2016

My ggtrackerstack is installing as we speak! This is so cool

@dsjoerg
Copy link
Owner

dsjoerg commented Apr 17, 2016

omg my ggtrackerstack works!

@gravelweb
Copy link
Contributor

@dsjoerg Don't sound so surprised :P

fps *= 1.4;
}
minute = Math.floor(frame / (60 * fps));
second = Math.floor((frame / fps) % 60).toString();
Copy link
Owner

Choose a reason for hiding this comment

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

just to doublecheck carefully:
Pre-LotV: 960 frames = 1:00 on the game clock = 0:43 on a real clock
LotV: 960 frames = 0:43 on the game clock = 0:43 on a real clock

so, yeah there used to be 16 frames per game second, and now there's ~16 * 1.4 frames per game second. looks good to me.

@dsjoerg
Copy link
Owner

dsjoerg commented Apr 17, 2016

this change looks good to me. I can't get the timeToFrame function to trigger at all; it may be vestigial, i thought i could get it to trigger by clicking on the right stuff but it wasn't working. i read the code carefully and it looks great so i say let's merge this in and proceed.

i'm not planning to deploy this as it would be a little confusing to have the armychart on the fixed timescale and the other charts not. but i'll take it on at master and probably we won't need to deploy until the other charts are fixed as well.

it's so awesome having ggtrackerstack!

@dsjoerg dsjoerg merged commit 1c12808 into dsjoerg:master Apr 17, 2016
@nickelsen nickelsen deleted the fix-lotv-army-chart branch April 18, 2016 07:08
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