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

Add the Oil Derrick count to the economy statistics #17022

Merged
merged 2 commits into from Sep 8, 2019

Conversation

@abcdefg30
Copy link
Member

commented Aug 31, 2019

Only RA and TD have Oil Derricks.

@abcdefg30 abcdefg30 marked this pull request as ready for review Aug 31, 2019

@pchote

This comment has been minimized.

Copy link
Member

commented Aug 31, 2019

I'm not sure that blindly counting CashTricklers is a good idea. Would be better IMO if we added a tag trait for this.

@GraionDilach

This comment has been minimized.

Copy link
Contributor

commented Sep 1, 2019

Agreeing with the tag trait - an "Oil Refinery" usecase to increase the amount of cash produced from the Oil Derricks via flipping between active CashTricklers would already give this method an issue.

@abcdefg30 abcdefg30 force-pushed the abcdefg30:oilStat branch 2 times, most recently from c33d994 to c10b0a2 Sep 1, 2019

@abcdefg30

This comment has been minimized.

Copy link
Member Author

commented Sep 1, 2019

Updated.

@pchote

This comment has been minimized.

Copy link
Member

commented Sep 1, 2019

I guess we should include this in the next playtest, so adding to the milestone.

@pchote pchote added this to the Next Release milestone Sep 1, 2019

@pchote

This comment has been minimized.

Copy link
Member

commented Sep 6, 2019

If we can't do a quick fix (like removing "Earned this min") then we may need to drop this from the milestone until someone can relabel / redesign the headers to not be constrained by these long labels.

@abcdefg30

This comment has been minimized.

Copy link
Member Author

commented Sep 6, 2019

We could gain some space by using something like $ this min (we have $/min as well), but I don't think it would be enough. The question is if earned this minute is even that useful, since you kind of already have the graph displaying that information(?). (Would work good with #17044 imho.)

@abcdefg30 abcdefg30 force-pushed the abcdefg30:oilStat branch from c10b0a2 to d157afc Sep 6, 2019

@abcdefg30

This comment has been minimized.

Copy link
Member Author

commented Sep 6, 2019

Updated.

@pchote

This comment has been minimized.

Copy link
Member

commented Sep 7, 2019

LGTM, although I did notice #17059 while testing this. Can you please merge the last two commits?

@abcdefg30 abcdefg30 force-pushed the abcdefg30:oilStat branch from d157afc to 5156750 Sep 7, 2019

@abcdefg30

This comment has been minimized.

Copy link
Member Author

commented Sep 7, 2019

Rebased and squashed.

@yaaaaa

This comment has been minimized.

Copy link

commented Sep 8, 2019

Hmm... why statistic needs oil derrick count? oil derrick count, and its plasements is static. The maps have own oil derrick counts, and name of The Map is exist and most importent parametr.

@pchote
pchote approved these changes Sep 8, 2019

@pchote pchote added the PR: Needs +2 label Sep 8, 2019

@teinarss teinarss merged commit 912a424 into OpenRA:bleed Sep 8, 2019

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:oilStat branch Sep 8, 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.