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

Replace $/min and increase the graph update rates #17055

Merged
merged 6 commits into from Oct 11, 2019

Conversation

@abcdefg30
Copy link
Member

abcdefg30 commented Sep 7, 2019

Depends on #17022 and #16858. (With those two merged I'll polish the code here a bit.)

Closes #17044.
Closes #3451.
Closes #10090.

Testcase is currently the earnings graph in TD, it will update every twenty seconds.
$/min in TD is also replaced by the "Income during the last 60 seconds" (just not renamed yet) as discussed on IRC.

@abcdefg30 abcdefg30 force-pushed the abcdefg30:stats branch from 398d3dc to 043bcd3 Sep 9, 2019
@RoosterDragon

This comment has been minimized.

Copy link
Member

RoosterDragon commented Sep 13, 2019

Dependencies are merged.

@abcdefg30 abcdefg30 force-pushed the abcdefg30:stats branch from 043bcd3 to 7fd6800 Sep 20, 2019
@abcdefg30

This comment has been minimized.

Copy link
Member Author

abcdefg30 commented Oct 5, 2019

This is basically ready for review since a few days. I/We just need to settle on a layout for the graph. At the moment it only covers 13 minutes, but already looks a bit cramped:
grafik

@pchote pchote added this to the Next Release milestone Oct 5, 2019
@abcdefg30 abcdefg30 force-pushed the abcdefg30:stats branch 3 times, most recently from 30cc81d to 2ea0808 Oct 5, 2019
@abcdefg30 abcdefg30 marked this pull request as ready for review Oct 6, 2019
@abcdefg30

This comment has been minimized.

Copy link
Member Author

abcdefg30 commented Oct 6, 2019

Updated. This now includes the first step towards fixing #17059.

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Oct 6, 2019

Needs some fixes, as discussed in IRC.

@abcdefg30 abcdefg30 force-pushed the abcdefg30:stats branch 2 times, most recently from 826034e to f58624a Oct 6, 2019
@abcdefg30 abcdefg30 force-pushed the abcdefg30:stats branch 2 times, most recently from 53e8a08 to 9d4ad2a Oct 6, 2019
@abcdefg30 abcdefg30 force-pushed the abcdefg30:stats branch from 9d4ad2a to 0b61fc4 Oct 6, 2019
@abcdefg30

This comment has been minimized.

Copy link
Member Author

abcdefg30 commented Oct 6, 2019

Updated.

@abcdefg30 abcdefg30 force-pushed the abcdefg30:stats branch from 0b61fc4 to 0df767e Oct 6, 2019
@pchote pchote added the PR: Needs +2 label Oct 9, 2019
Copy link
Contributor

matjaeck left a comment

Looks good, the new income data will help players to analyze their games 👍

using System.Collections.Generic;
using System.Linq;

This comment has been minimized.

Copy link
@teinarss

teinarss Oct 9, 2019

Contributor

nitpick: Not needed :P

@@ -45,8 +37,10 @@ public int Experience
}
}

// Low resolution (every 30 seconds) record of earnings, covering the entire game
public List<int> EarnedSamples = new List<int>(100);

This comment has been minimized.

Copy link
@teinarss

teinarss Oct 9, 2019

Contributor

Rename to IncomeSamples? Not sure if its worth to keep the naming consistent with the rest.

This comment has been minimized.

Copy link
@pchote

pchote Oct 9, 2019

Member

This really is sampling the earned resources. Income is calculated from the difference of the first and last earning samples.

This comment has been minimized.

Copy link
@teinarss

teinarss Oct 9, 2019

Contributor

Maybe I'm just confused but we are only adding Income to EarnedSamples and then its displayed in the IncomeGraph. The calculation of Income is not related to this property except its stored in this list.

@abcdefg30 abcdefg30 dismissed stale reviews from matjaeck and pchote via 475f24f Oct 11, 2019
@abcdefg30 abcdefg30 force-pushed the abcdefg30:stats branch from 0df767e to 475f24f Oct 11, 2019
@abcdefg30 abcdefg30 force-pushed the abcdefg30:stats branch from 475f24f to 5958918 Oct 11, 2019
@abcdefg30

This comment has been minimized.

Copy link
Member Author

abcdefg30 commented Oct 11, 2019

Rebased (first force push) and updated (second one; for inspection).

@reaperrr reaperrr merged commit f9f1167 into OpenRA:bleed Oct 11, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@abcdefg30 abcdefg30 deleted the abcdefg30:stats branch Oct 11, 2019
@reaperrr

This comment has been minimized.

Copy link
Contributor

reaperrr commented Oct 11, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.