Page MenuHomeWildfire Games

Rating column in the lobby gamelist
ClosedPublic

Authored by elexis on Feb 4 2017, 8:03 AM.

Details

Summary

New Column for Showing Average Rank of Players in a Game for that Game in Lobby.
Spec Rank from a game is for searching good ppl to play them.

Test Plan

Test Resolution 1024/768 for fitness.

Diff Detail

Repository
rP 0 A.D. Public Repository
Branch
/ps/trunk
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 1564
Build 2471: Vulcan BuildJenkins
Build 2470: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
elexis added inline comments.Feb 4 2017, 8:30 AM
lobby.js
255 ↗(On Diff #446)

This is out of scope of the gamerank sorting. so the change has to be removed in this patch.
(Different people, different taste, we could save the most recent checkbox state in the user config perhaps).

324 ↗(On Diff #446)

Delete code that is commented out

328 ↗(On Diff #446)

sortOrder *= -1 ?

534 ↗(On Diff #446)

g_ prefix only used for globals, rename it to gameList

536 ↗(On Diff #446)

Use a for ... of loop, since you don't use i anywhere, thus the next line becomes unneeded too

540 ↗(On Diff #446)

Use let instead of var throughout this file (except for globals at the begin of the file)

541 ↗(On Diff #446)

pls -> players

544 ↗(On Diff #446)

{ to the next line

546 ↗(On Diff #446)

just use players[j].Name instead of introducing a new variable (same true for team)

548 ↗(On Diff #446)

The lastIndex reset can be avoided by nuking the re variable and doing /...regex.../g.exec(....)

551 ↗(On Diff #446)

What is the use case of the average spectator ranking? Finding the game where the good players observe, so one can join that game too?

559 ↗(On Diff #446)

Superfluous braces. they can be removed if it contains only one statement

lobby.xml
218 ↗(On Diff #446)

1024x768 is the minimum resolution that is supported. We can see on the screenshot that not everything fits.

I suggest to set the size of the "map type" column to 0 (since the information provided isn't that useful, we already have a mapname column and the distinction between random and skirmish/scenario maps is contained in the map-size column too).

If players have a higher resolution and thus unused screen space, we can write a new patch to show more columns, like the now hidden map-type column.

This revision now requires changes to proceed.Feb 4 2017, 8:30 AM

changed many points but resolution issue.

some more points

ffffffff updated this revision to Diff 451.Feb 4 2017, 6:31 PM
ffffffff edited edge metadata.

updated from proposals.

elexis requested changes to this revision.Feb 4 2017, 6:51 PM

Besides the codestyle still broken with 1024x768

lobby.js
532 ↗(On Diff #451)

A JS map function would be neat, like

g_GameList = Engine.GetGameList().map(game => {
...
}).filter(the one from below)
556 ↗(On Diff #451)

A "-" in the table would mess up the sorting (consider average > 1200 and < 1200), so use numbers only.
Also the if statement can be avoided by passing the initial 1200 as a new argument to reduce:
https://developer.mozilla.org/de/docs/Web/JavaScript/Reference/Global_Objects/Array/Reduce

Same for the other reduce.
(Didn't actually try but it sounds like it would return 1200 for empty arrays)

So if you're lucky, you can avoid 2 more variables and end up with game.nRank = reduce....

548 ↗(On Diff #446)

Nuke the peculiar lastIndex line and fix it by nuking / inlining re

lobby.xml
221 ↗(On Diff #451)

Should consist of words that actually exist.
Perhaps "Rank (Observers)" maybe?

This revision now requires changes to proceed.Feb 4 2017, 6:51 PM

@scythetwirler Do you believe the column should display many entries with "1200" (so as to keep it sortable) or "-" instead (so that its consistent with the playerlist and hides annoying duplication)?

ffffffff edited the summary of this revision. (Show Details)Feb 4 2017, 8:53 PM
ffffffff updated this revision to Diff 487.Feb 9 2017, 5:30 PM
ffffffff edited edge metadata.
elexis requested changes to this revision.Feb 10 2017, 9:02 AM

Thanks for the update, one step closer to committing it ;-)
Don't forget we want to change if (foo) { to

if (foo)
{

I recall you encountered a player without name, but that is not a bug introduced by your patch, so we (others, likely Imarok) have to fix the origin of the bug, so don't add the playername check in your patch and let it error out in case.

The observer rank can be kept, but hidden (until we can let users chose on their own which columns they want to see). Also 1024x768 thing.

This revision now requires changes to proceed.Feb 10 2017, 9:02 AM
ffffffff updated this revision to Diff 507.Feb 10 2017, 6:41 PM
ffffffff edited edge metadata.
ffffffff updated this revision to Diff 508.Feb 10 2017, 6:44 PM
Imarok added a subscriber: Imarok.Feb 10 2017, 6:56 PM
Imarok added inline comments.
lobby.js
541 ↗(On Diff #508)

ternary?
(player.Team == "observer" ? specRatings : ratings).push(rating);

Personally, I'm not sure really sure having an extra column really helps that much (1200 could mean a 1500 + 900 vs two 1200s), especially when we're already pressed for space in that box.

Personally, I'm not sure really sure having an extra column really helps that much (1200 could mean a 1500 + 900 vs two 1200s), especially when we're already pressed for space in that box.

I tried the patch in alpha 21, it really helps finding the two games out of the 20 games where the better players are playing in. It even works if there are some noobs in it, as the better players have far better score.
As mentioned before, we have not enough space for a new column without removing an old one, hence suggested to remove (as in hide) the maptype one (who needs that? also the mapsize one implied the maptype partially).

There are also further columns missing, for example the gamestate one (gamesetup / ingame). The plan is that in the future, more columns will be shown automatically if more screenspace is available (much space unused for regular sized monitors atm), and that players can chose which columns to display if we have many of them.

This comment was removed by fatherbushido.

waouhhh

Indeed, I didn't think about that.

ffffffff added a comment.EditedFeb 11 2017, 8:15 AM
In D125#4881, @elexis wrote:

I tried the patch in alpha 21, it really helps finding the two games out of the 20 games where the better players are playing in.

waouhhh

FPREFPREFPREPFREPFPREPFPREPFPREPFPREPFPREPFPRPEPFEPREPFPERPEPFEPREPFPERPEPFEPRPEPFEPREPFEPRPRE

nerverErverErver

.................

lobby.js
532 ↗(On Diff #451)

no map call below

556 ↗(On Diff #451)

cannot be avoided due to its unclear for recude call on empty object.

540 ↗(On Diff #446)

how to hide the error player.Name?

lobby.xml
221 ↗(On Diff #451)

HOW TO GET SRANK 0 and when fullscreen MORE SIZE?

I'd rather see the game list only hold concrete values (attributes of the game itself) instead of derived fields, but after talking with elexis, this feature seems like it would only be used by a subset of the lobby; perhaps a config option customizing columns?

elexis commandeered this revision.Feb 14 2017, 5:17 PM
elexis edited reviewers, added: ffffffff; removed: elexis.
elexis updated this revision to Diff 528.Feb 14 2017, 5:21 PM
  • Making this feature optional and disabled by default. If enabled, hides the mapType column and shows the average rating column. Required C++ changes to allow hiding columns. Notice the hiding is only possible from JS, as the use case is changing it dynamically. Statically hidden columns could be removed altogether.
  • Added a filter with the choices < 1000, < 1100, < 1200, > 1200, > 1300, > 1400 and > 1500.
  • Remove the spec rating, as I'm not convinced it is useful.
  • Increase gamename column width from 27% to 33% so that the sum of the widths of all columns is 100%. Reduce width of playernr filter by 10px for consistency.

Code style:

  • Use map instead of a loop, thus get rid of the gameList variable.
  • Use let instead of var.
  • The 1200 default of the reduce call was in the wrong place, that's why it didn't work with empty arrays. Fixing it allows to remove the length>0 check and the default "1200" (which should have been 1200). That in turn allows getting rid of the average variable.
  • Remove the two unneeded case calls as they can be moved to the generic case (which works in everytime where sortBy is equal to the key we sort for)
  • Rename ratings to playerRatings, nRank to averageRating as the n prefix was only used for nPlayers which stands for NumberOf I guess
  • Since we now have a map extending game information before the sorting and filtering, we can get rid of the duplicate translate calls of the map title, which were also buggy as sometimes a plain translate was used, other times translateMapTitle
elexis added a comment.EditedFeb 14 2017, 5:32 PM

The use case is finding a game where the players have a specified skill, in particular
(1) Finding the games with the players that play this game frequently and thus have a greater score than average in order to play there
(2) Finding that game in order to spectate it ("0ad-tv")
(3) Finding a game where the other participants are newcomers too

Since the ratings don't always correlate with their skill and since this does averaging, it is false in some cases, but it is true in the average case and as close to the truth as we can get.
While the lobby page definitely needs means to find games that contain specific players, this doesn't exactly cover the use case, as only a single playername could be searched for.

Notice we should also have this option for the replay menu. The shared code can be moved to common/, but I'd wish to have this accepted prior.

Vulcan added a subscriber: Vulcan.Feb 14 2017, 6:05 PM

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/359/ for more details.

elexis added a reviewer: Restricted Owners Package.Feb 14 2017, 11:43 PM

@scythetwirler perhaps the greater part of the concern of the average rating not reflecting the actual players skills is due to considering the gamesetup, rather than the session (as the gamesetup doesn't distinguish between assigned players and observers, contrary to the session)?
In that case we might reconsider adding the observer distinction to the gamesetup lobby stanza, as that would also help judging whether it's worth to join the game too.

elexis retitled this revision from Game Rank/Spec Rank in Lobby to Average rating in the lobby gamelist.Feb 15 2017, 4:11 PM
ffffffff accepted this revision.Feb 15 2017, 10:48 PM

ok.
can we have some more columns when bigger screen, specRating and specCount used/available slots.

lobby.xml
221 ↗(On Diff #451)

so we write a new script???

want to have spec column for my view

.....................

Indeed a number of columns should be added, the ones I have in mind:

  • Victory Condition (Conquest, Regicide, Wonder, ...)
  • 0ad-tv (late observer mode enabled)
  • Status (running or gamesetup)
  • Time started
  • Number of observers
  • Average observer rating if it has to be

The player should be able to chose on his own which of these to display, as not all of them can fit into that screen.

It will have to be done in a separate patch. (In particular writing the patch when this one isn't committed is likely to result in wasted time otherwise.)

vladislavbelov added inline comments.
binaries/data/mods/public/gui/lobby/lobby.js
412

Why not the right order initially?

859

Unnecessary empty line.

870

Should this value be hardcoded? Maybe need a constant?

What do you think about the C++ change?
Is that `// TODO: Ideally we could read the size of the columns of the COList instead``` acceptable (as in doing that in another diff to limit the complexity of this diff and since there are only 4 numbers hardcoded)?

binaries/data/mods/public/gui/lobby/lobby.js
412

I felt this one would be more readable

870

hm, might be right actually, in particular since its used more than once

What do you think about the C++ change?

It's ok, looks similar to the lists receiving.

Is that `// TODO: Ideally we could read the size of the columns of the COList instead``` acceptable (as in doing that in another diff to limit the complexity of this diff and since there are only 4 numbers hardcoded)?

Hmm, I think, it's not acceptable, because widths of columns are set in XML files.

COList diff moved to D228, will likely address the TODO as well

elexis updated this revision to Diff 978.Mar 28 2017, 2:27 AM

Remove the COlist.cpp part which has been extended to XML and committed in D228.

elexis edited reviewers, added: Imarok; removed: Restricted Owners Package.Mar 28 2017, 2:28 AM
This revision is now accepted and ready to land.Mar 28 2017, 2:28 AM

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (305 tests).................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (305 tests).................................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/624/ for more details.

elexis updated this revision to Diff 1763.May 8 2017, 11:54 AM

Rebase after the buddy patch merge and unify rating split function.
Rename average rating to game rating.
Still keep that one line that actually allows testing the code that obviously will be removed when committing.
Make global for the default rating.

Vulcan added a comment.May 9 2017, 1:04 AM

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/1087/ for more details.

elexis retitled this revision from Average rating in the lobby gamelist to Rating column in the lobby gamelist.May 18 2017, 1:00 PM
elexis added inline comments.May 18 2017, 1:50 PM
binaries/data/mods/public/gui/lobby/lobby.js
387

These 4 numbers were the only complaint Vladislav and causative stated.

D457 was created to address the case, but the implementation

  • is controversial (it could be implemented as a read-only function, or a setting. See also D316 which adds a setting whose write operation is not implemented)
  • the current proposal fails to address the actual reason why we have these 2 numbers here. We don't need to change the column position but the position of the filters above the columns while not being able to get the column widths from the GUI object.

So these 2 lines provide a test to check whether D457 addresses this purpose. If it doesn't remove these 4 hardcoded numbers, it doesn't work.
Vladislav agreed on irc that D457 can be done afterwards.

D228 was split from this patch and committed separately.

(Also created #4578, so that this column becomes hidden if the ratings are disabled entirely, along with the other rating GUI elements)

415

Adding translation context.

This revision was automatically updated to reflect the committed changes.