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.
Details
Details
Test Resolution 1024/768 for fitness.
Diff Detail
Diff Detail
- Repository
- rP 0 A.D. Public Repository
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
There are a very large number of changes, so older changes are hidden. Show Older Changes
| 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. |
| 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. |
Comment Actions
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. Same for the other reduce. 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. |
Comment Actions
@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)?
Comment Actions
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.
| lobby.js | ||
|---|---|---|
| 541 ↗ | (On Diff #508) | ternary? |
Comment Actions
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.
Comment Actions
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.
Comment Actions
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? |
Comment Actions
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?
Comment Actions
- 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
Comment Actions
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.
Comment Actions
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.
Comment Actions
@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.
Comment Actions
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 ..................... |
Comment Actions
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.)
Comment Actions
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 | ||
|---|---|---|
| 272 ↗ | (On Diff #528) | I felt this one would be more readable |
| 592 ↗ | (On Diff #528) | hm, might be right actually, in particular since its used more than once |
Comment Actions
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.
Comment Actions
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.
Comment Actions
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.
Comment Actions
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.
| binaries/data/mods/public/gui/lobby/lobby.js | ||
|---|---|---|
| 387 ↗ | (On Diff #1763) | These 4 numbers were the only complaint Vladislav and causative stated. D457 was created to address the case, but the implementation
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. 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 ↗ | (On Diff #1763) | Adding translation context. |