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 Army Value statistics/graph #15767

Merged
merged 7 commits into from Jan 1, 2019

Conversation

Projects
None yet
6 participants
@ubitux
Copy link
Contributor

ubitux commented Oct 31, 2018

This feature makes possible to follow the army value over time in a game, useful for analysis in the competitive scene (typically to spot when good/bad trades happen).

There are a bunch of units for which I'm unsure whether I should count them as part of the army or not. Following is the list of my doubts, open to discussion. Note: some of the unit in RA are redundant in other mods (such as medic).

RA:
engie? -> no
medic? -> no
mechanic? -> no
spy? -> yes
minelayer? -> yes
mobile gap gen? -> yes
mobile radar jammer? -> yes
general? -> yes
thief? -> yes
hijacker? -> yes
transport? (ship) -> no
chinook? -> no

CNC:
dino? -> no
landing craft? -> no

D2K:
orni? -> no
thumper? -> no
saboteur? -> yes

TS:
mutant hijacker? -> yes

Here is a screenshot on how it looks in observer mode. There is also a new column in the Combat tab.

2018-10-31-223605_1920x1080_scrot

@ubitux ubitux force-pushed the ubitux:army-value-stats-graph branch 4 times, most recently from d9fe839 to 231bb5a Oct 31, 2018

@ubitux

This comment has been minimized.

Copy link
Contributor

ubitux commented Nov 1, 2018

Changes since initial PR:

  • split commits
  • set up observer for other mods
  • misc rules fixes

@ubitux ubitux force-pushed the ubitux:army-value-stats-graph branch 3 times, most recently from 73368bc to 7c7a78e Nov 1, 2018

@ubitux ubitux force-pushed the ubitux:army-value-stats-graph branch from 7c7a78e to 5293e26 Nov 8, 2018

@pchote pchote added this to the Next Release milestone Nov 21, 2018

@pchote
Copy link
Member

pchote left a comment

Code changes look reasonable, just a couple of minor nits.

Can we please get some player testing here?

Show resolved Hide resolved OpenRA.Mods.Common/Traits/Player/PlayerStatistics.cs Outdated
Show resolved Hide resolved OpenRA.Mods.Common/Traits/Player/PlayerStatistics.cs Outdated
@FRenzy-OpenRA

This comment has been minimized.

Copy link

FRenzy-OpenRA commented Nov 25, 2018

What is the reasoning behind removing these specific units from the feature ?

I'd suggest enabling them all by default. For instance, medics and mechanics often constitute a significant bulk of Allied armies. Removing them will flaw the comparison in AvS matchup.

@ubitux

This comment has been minimized.

Copy link
Contributor

ubitux commented Nov 25, 2018

What is the reasoning behind removing these specific units from the feature ?

I'd suggest enabling them all by default. For instance, medics and mechanics often constitute a significant bulk of Allied armies. Removing them will flaw the comparison in AvS matchup.

It followed the basic rule of thumb "any unit that impacts directly the enemy (by reducing enemy health, speed, vision, ...)". As a result, support units were out of this. I don't mind adding them, but then should transport units be in as well? What about the engie? I have no definite opinion on the matter, but a consensus on where to draw the line needs to be made. What I don't want though, is to have the harvesters counted in.

@FRenzy-OpenRA

This comment has been minimized.

Copy link

FRenzy-OpenRA commented Nov 25, 2018

On engies, harvesters, naval transports : not a problem to not count them.

Chinooks : I'm more cautious on this one, as their Soviet transport counterpart (APC) is weaponed and will be counted. By precaution, I'd say count it in ?
But in that case, and for consistency, naval transports would be counted as well.

@ubitux

This comment has been minimized.

Copy link
Contributor

ubitux commented Nov 25, 2018

On engies, harvesters, naval transports : not a problem to not count them.

Chinooks : I'm more cautious on this one, as their Soviet transport counterpart (APC) is weaponed and will be counted. By precaution, I'd say count it in ?

I don't know, again it's all about where you draw the line: look at engie for instance, you could argue that they do affect the enemy player if used as a mean to capture their building. Chinooks never do any damage, but they do if they crash, so should they be counted in? Even harvesters could be counted in since they can be used to tank and crush infantry.

So yeah again, the current patchset is where I drew the line, but obviously don't mind adjusting it to a certain extent (see remark on the harvs in my previous post) in one direction or another. That said, since the issue is debatable in many aspects, I'd prefer if a definite consensus was decided before I make any change.

@ubitux ubitux force-pushed the ubitux:army-value-stats-graph branch 2 times, most recently from 6fbc4d5 to 1e84002 Nov 25, 2018

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Nov 25, 2018

I expect that it is going to take too long to form a consensus here, so we're going to have to bump this to Next + 1.

@pchote pchote modified the milestones: Next Release, Next + 1 Nov 25, 2018

@ubitux

This comment has been minimized.

Copy link
Contributor

ubitux commented Nov 25, 2018

I expect that it is going to take too long to form a consensus here, so we're going to have to bump this to Next + 1.

Note that this is just informational statistics; it doesn't matter if they are adjusted over time or temporarily kind of inaccurate/incomplete.

@matjaeck

This comment has been minimized.

Copy link
Contributor

matjaeck commented Nov 25, 2018

Army is everything that is not a building or a harvester.

@ubitux ubitux force-pushed the ubitux:army-value-stats-graph branch from 1e84002 to 096c9c6 Nov 26, 2018

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Nov 27, 2018

There is still time to get this in for the playtest if a consensus can be found and reviews done in time. Choosing not to block the playtest over it doesn't have to mean pushing it out of the playtest completely.

@ubitux ubitux force-pushed the ubitux:army-value-stats-graph branch 2 times, most recently from 3853bf7 to 4532b09 Dec 7, 2018

@ubitux

This comment has been minimized.

Copy link
Contributor

ubitux commented Dec 7, 2018

OK so, I updated the branch with the following changes:

  • PlayerStatistics.cs: cache of UpdatesPlayerStatisticsInfo and cost
  • PlayerStatistics.cs: honor INotifyActorDisposing.Disposing()
  • PlayerStatistics.cs: key is now named UpdatesPlayerStatistics.AddToArmyValue (true/false)
  • mods: simplify a lot the Traits changes by applying AddToArmyValue=true to all soldiers (and cyborg for TS), vehicles, planes and ships, with the following 3 exceptions for vehicles: MCV, harvs, and supply truck. Doing so makes the diffs much smallers.
  • rebased on bleed

Note: I haven't retested all the mods again, so please double checks if more units need exceptions.

Show resolved Hide resolved mods/d2k/rules/defaults.yaml Outdated
@pchote

This comment has been minimized.

Copy link
Member

pchote commented Dec 31, 2018

Considering the subtleties revealed in my comments above, I would be happier if we could keep the defaults as they were (AddToArmyValue implicitly false) and then explicitly opt in every actor we want to be counted.

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Dec 31, 2018

Code changes otherwise look good, so I expect to 👍 and merge as soon as my final points above are resolved one way or another.

@ubitux ubitux dismissed stale reviews from Smittytron and matjaeck via bf8c3f2 Dec 31, 2018

@ubitux ubitux force-pushed the ubitux:army-value-stats-graph branch from f9c24da to bf8c3f2 Dec 31, 2018

@ubitux

This comment has been minimized.

Copy link
Contributor

ubitux commented Dec 31, 2018

Considering the subtleties revealed in my comments above, I would be happier if we could keep the defaults as they were (AddToArmyValue implicitly false) and then explicitly opt in every actor we want to be counted.

My initial PR was doing that, but the diff was much bigger and hard to review. Following the simpler rule makes IMO more sense here. I can change it back again if this is blocking.

@matjaeck

This comment has been minimized.

Copy link
Contributor

matjaeck commented Dec 31, 2018

keep the defaults as they were (AddToArmyValue implicitly false) and then explicitly opt in every actor we want to be counted.

This will make future refinements easier too. I don't think abstraction is very helpful here because feedback/gameplay might require some arbitrary choices and inheritance might have unexpected results.

Priority should be to get the PR merged, though.

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Dec 31, 2018

I can change it back again if this is blocking.

I'd prefer it if you could - I can see the inheritance chain causing problems in the future otherwise.
If you'd like to keep a degree of abstraction to reduce boilerplate you could implement the override as its own base template that you can Inherits@ARMYVALUE: ^AddToArmyValue at the top of the relevant actors.

@ubitux ubitux force-pushed the ubitux:army-value-stats-graph branch 2 times, most recently from f7db034 to 8012938 Dec 31, 2018

@ubitux

This comment has been minimized.

Copy link
Contributor

ubitux commented Dec 31, 2018

I can change it back again if this is blocking.

I'd prefer it if you could - I can see the inheritance chain causing problems in the future otherwise.
If you'd like to keep a degree of abstraction to reduce boilerplate you could implement the override as its own base template that you can Inherits@ARMYVALUE: ^AddToArmyValue at the top of the relevant actors.

Nah, it's fine, I'm keeping the 2-lines version since it can be kind of inconsistent with the existing UpdatesPlayerStatistics entries.

Branch is updated, but I also kept the old one @ bleed...ubitux:army-value-stats-graph-bak if you change your mind and prefer to merge that one.

☭ git diff --stat army-value-stats-graph-bak..army-value-stats-graph
 mods/cnc/rules/aircraft.yaml       |  6 ++++++
 mods/cnc/rules/defaults.yaml       |  6 ------
 mods/cnc/rules/infantry.yaml       | 14 ++++++++++++++
 mods/cnc/rules/ships.yaml          |  2 ++
 mods/cnc/rules/vehicles.yaml       | 32 ++++++++++++++++++++++++--------
 mods/d2k/rules/defaults.yaml       |  4 ----
 mods/d2k/rules/infantry.yaml       | 16 ++++++++++++++++
 mods/d2k/rules/vehicles.yaml       | 24 ++++++++++++++++++++----
 mods/ra/rules/aircraft.yaml        | 10 ++++++++++
 mods/ra/rules/defaults.yaml        |  5 -----
 mods/ra/rules/infantry.yaml        | 32 ++++++++++++++++++++++++++++++++
 mods/ra/rules/ships.yaml           | 12 ++++++++++++
 mods/ra/rules/vehicles.yaml        | 40 ++++++++++++++++++++++++++++++++++------
 mods/ts/rules/aircraft.yaml        | 18 ++++++++++++++++++
 mods/ts/rules/defaults.yaml        |  8 --------
 mods/ts/rules/gdi-infantry.yaml    |  8 ++++++++
 mods/ts/rules/gdi-vehicles.yaml    | 14 ++++++++++++++
 mods/ts/rules/nod-infantry.yaml    |  8 ++++++++
 mods/ts/rules/nod-vehicles.yaml    | 16 ++++++++++++++++
 mods/ts/rules/shared-infantry.yaml |  4 ++++
 mods/ts/rules/shared-vehicles.yaml |  4 ----
 21 files changed, 238 insertions(+), 45 deletions(-)

Please double check that I didn't forget something in the process.

@pchote
Copy link
Member

pchote left a comment

Double checking raised a couple of final things, then lets get this merged.

Show resolved Hide resolved mods/ra/rules/infantry.yaml Outdated
Show resolved Hide resolved mods/cnc/rules/infantry.yaml
Show resolved Hide resolved mods/ts/rules/aircraft.yaml Outdated
Show resolved Hide resolved mods/ts/rules/aircraft.yaml Outdated
Show resolved Hide resolved OpenRA.Mods.Common/Widgets/Logic/Ingame/ObserverStatsLogic.cs

@ubitux ubitux force-pushed the ubitux:army-value-stats-graph branch from 8012938 to 007123c Jan 1, 2019

@ubitux

This comment has been minimized.

Copy link
Contributor

ubitux commented Jan 1, 2019

Last changes requested done + bleed rebased

@pchote

pchote approved these changes Jan 1, 2019

@pchote pchote merged commit bb5e0ea into OpenRA:bleed Jan 1, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@ubitux ubitux deleted the ubitux:army-value-stats-graph branch Jan 1, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment