Opened 10 years ago

Closed 7 years ago

Last modified 6 years ago

#1580 closed enhancement (fixed)

[PATCH] Change player color in the gamesetup

Reported by: O.Davoodi Owned by: stanislas69
Priority: Nice to Have Milestone: Alpha 19
Component: UI & Simulation Keywords: patch
Cc: Patch:

Description (last modified by elexis)

Attachments (17)

customColorSupport.patch (10.7 KB ) - added by Adrián Chaves 9 years ago.
Patch based on the work started by Spahbod.
1580.diff (11.1 KB ) - added by stanislas69 7 years ago.
WIP patch, it's currently broken for some reason, help is appreciated. I can't get the drop down to appear
1580.2.diff (9.9 KB ) - added by stanislas69 7 years ago.
WIP2
1580.3.diff (6.2 KB ) - added by stanislas69 7 years ago.
Working Color Dropdown
1580.4.diff (7.0 KB ) - added by stanislas69 7 years ago.
1580.5.diff (8.6 KB ) - added by stanislas69 7 years ago.
1580.6.diff (8.5 KB ) - added by stanislas69 7 years ago.
Some Cleanup
1580.7.diff (7.0 KB ) - added by elexis 7 years ago.
1580.8.diff (6.1 KB ) - added by stanislas69 7 years ago.
l10n_longestStrings.7z (203.3 KB ) - added by elexis 7 years ago.
Mock language that contains the longest strings found in the translations of r17026.
1580.patch (6.2 KB ) - added by stanislas69 7 years ago.
Answered previous comments.
1580.10.diff (6.2 KB ) - added by stanislas69 7 years ago.
Version Without Space as it breaks on smaller resolutions this version still needs the c++ patch
1580.10.Workaround.diff (6.2 KB ) - added by stanislas69 7 years ago.
Same as previous, but without the need for c++
1580.11.diff (8.7 KB ) - added by elexis 7 years ago.
1580.12.diff (8.3 KB ) - added by elexis 7 years ago.
1580.13.diff (9.7 KB ) - added by elexis 7 years ago.
There you go, skirmish supported again. Colors that are not identical to the colors in player_defaults.json will be replaced with the closest match!
1580.14.patch (9.9 KB ) - added by elexis 7 years ago.
Ensures player colors are unique after matching to player_defaults.json.

Download all attachments as: .zip

Change History (43)

comment:1 by Kieran P, 9 years ago

Milestone: Alpha 12Backlog

comment:2 by historic_bruno, 9 years ago

Owner: O.Davoodi removed

comment:3 by Adrián Chaves, 9 years ago

Owner: set to Adrián Chaves

by Adrián Chaves, 9 years ago

Attachment: customColorSupport.patch added

Patch based on the work started by Spahbod.

comment:4 by Adrián Chaves, 9 years ago

Keywords: review patch added
Milestone: BacklogAlpha 15

Based on the previous work that Spahbod posted in the forums, I’m attaching a patch which in addition does the following:

  • Keeps color display names in a string of their own, in preparation for the internationalization feature (#67).
  • Sorts colors in the drop-down list by color.

Although I was planning on doing so, I refrained from including ancient-themed colors. I think that needs more discussion (even if we all agree on doing it, we still need to agree on the RGB and display name of the values to be added), and I’ll try to bump the forum thread about it.

Adding new colors is as easy as editing the relevand JavaScript object in code. In the future, we could even move such data into a separated .json file if needed.

Note also that the implementation is prepared to deal with a color defined in simulation/data/player_defaults.json not being available in the JavaScript object that defined colors and color names in the GUI code. This is to keep simulation and gui code separated, as they should not depend in one another. When this occurs (a color is unknown in the GUI), the drop-down list shows empty, and lets you either keep the current, unknown color, or change to another color; if you change, you can not select back any unknown color. (you’ll probably need to actually read the code to understand this, just look for “-1” in the patch)

comment:5 by Adrián Chaves, 9 years ago

Summary: Adding the ability to change player colour in game setup[PATCH] Adding the ability to change player colour in game setup

comment:6 by Adrián Chaves, 9 years ago

Owner: Adrián Chaves removed

comment:7 by leper, 8 years ago

Milestone: Alpha 15Alpha 16

comment:8 by sanderd17, 8 years ago

Keywords: color gamesetup review removed

Can you make this patch work for skirmish maps too?

And the patch causes some problems when using a low resolution. The colour picker should be made smaller. Maybe only showing coloured squares instead of colour names?

comment:9 by sanderd17, 8 years ago

Milestone: Alpha 16Alpha 17

comment:10 by leper, 8 years ago

Milestone: Alpha 17Backlog

comment:11 by stanislas69, 7 years ago

Owner: set to stanislas69

by stanislas69, 7 years ago

Attachment: 1580.diff added

WIP patch, it's currently broken for some reason, help is appreciated. I can't get the drop down to appear

comment:12 by stanislas69, 7 years ago

Note that the json file will be removed, I added it in the patch, cause I forgot to delete it. It actually use default_players to get the colors.

comment:13 by elexis, 7 years ago

  • Please create the patch relatively to the trunk directory, so that we can apply the patch from there

settings.js

  • PlayerColors instead of Player_Colors
  • the code was updated

gamesetup.js

  • ++o instead of o++. I'd prefer n or i over o. But even better would be to remove that loop and just use array.find(...).length if possible.
  • getIndexOfColor don't need to do var playerColorsSorted = getPlayerColorsSorted(); every cycle, only once. replace the for loop by a call to findIndex
  • rgbToHsl - exact same function as in lobby.js. we should annoy the devs with a ticket that moves rgbToHsland hslToRgb to a new file common/color.js (the other color functions from lobby.js are almost all baloney, for example the rainbow colored pony-nicknames, and must be deleted in #3383 or so instead of being moved)

getPlayerColorsSorted()

  • early return
  • [] instead of new Array()
  • replacing for with map might be possible
  • if the block has only one line, the brackets can be removed (unless neede for aesthetics)
  • let instead of var in nested blocks so that theoretically a bit of performance can be saved
  • outerloop: try to avoid jumping to labels, I bet we can use a map or something similar for the inner loop and omit the problem that way

copyRgbValuesFromColorIntoColor

  • whitespace

gamesetup.xml

  • a tab missing
  • the position/size you gave the color picker is almost identical to the position of the player assignment dropdown. That might be why the former is invisible (but shows the tooltip when hovering) and why the latter seems disabled. The values are "left top right bottom". The "right" value of the left element must be smaller than the "left" value of the right element, etc. Where do you want to palce the color picker? Left of the playername? The xml should be moved to the correct location too.

Some rtfm's: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/indexOf and the other array functions linked there https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/Arrow_functions http://trac.wildfiregames.com/wiki/Coding_Conventions

by stanislas69, 7 years ago

Attachment: 1580.2.diff added

WIP2

by stanislas69, 7 years ago

Attachment: 1580.3.diff added

Working Color Dropdown

by stanislas69, 7 years ago

Attachment: 1580.4.diff added

by stanislas69, 7 years ago

Attachment: 1580.5.diff added

by stanislas69, 7 years ago

Attachment: 1580.6.diff added

Some Cleanup

by elexis, 7 years ago

Attachment: 1580.7.diff added

comment:14 by elexis, 7 years ago

Milestone: BacklogAlpha 19
  • hardcore cleanup
  • gaia color not selectable anymore
  • colors are actually used ingame after selecting them

TODO:

  • only host can chose colors
  • dropdown element should list only colors that are not picked yet
  • discovered a dropdown list GUI bug. looks like the width of the scrollbar is subtracted even if the scrollbar doesn't exist. Might also have something to do with the xml / width.

by stanislas69, 7 years ago

Attachment: 1580.8.diff added

comment:15 by stanislas69, 7 years ago

Keywords: review added
  • As agreed on IRC, it's better for now that only the host can change colors, so that there is no fight about it.
  • There is no need for not showing colors, since we swap between them when they are being used.
  • Only the gui bug remains on certain resolutions. see #3415
Last edited 7 years ago by stanislas69 (previous) (diff)

comment:16 by elexis, 7 years ago

Description: modified (diff)
Summary: [PATCH] Adding the ability to change player colour in game setup[PATCH] Change player color in the gamesetup

by elexis, 7 years ago

Attachment: l10n_longestStrings.7z added

Mock language that contains the longest strings found in the translations of r17026.

comment:17 by elexis, 7 years ago

As leper mentioned on IRC:

  • We should have a comment near that splice, like
    // Gaia color not selectable
    
  • Remove that last space in l387 of gamesetup.js
  • The column that has "Player 1" to "Player 8" in it might be too small to fit for all translations. Make sure that it fits for all translations. You could check with the mock language above, which was generated using http://trac.wildfiregames.com/wiki/Implementation_of_Internationalization_and_Localization#LongStringsLocale. leper said there might be some false positives (unfinished / wrong translations or something). The translation files should be edited in that case and the script run again.

by stanislas69, 7 years ago

Attachment: 1580.patch added

Answered previous comments.

by stanislas69, 7 years ago

Attachment: 1580.10.diff added

Version Without Space as it breaks on smaller resolutions this version still needs the c++ patch

by stanislas69, 7 years ago

Attachment: 1580.10.Workaround.diff added

Same as previous, but without the need for c++

comment:18 by elexis, 7 years ago

  • Found a bug: Select an unused color and increase the number of players. That color will be used twice. Fixed it by fixing the object instead of blindly reseting the colors.
  • Moved the logic to a new function as initMain() is crowded.
  • Updated gamesetup.xml to use proper positions and hide the colorpicker caption too for scenario maps.
  • For 1600x1200 that gui bug #3415 still applies, even with scrollbar="false" (not a dealbreaker imho).
  • Updated that gaia comment.
  • Fixed a missing semicolon.

Patch by all three of us.

by elexis, 7 years ago

Attachment: 1580.11.diff added

comment:19 by stanislas69, 7 years ago

Can confirm it works on 1366x768 too.

by elexis, 7 years ago

Attachment: 1580.12.diff added

comment:20 by elexis, 7 years ago

  • Drops support for colorpicking on skirmish maps, as they can define custom colors, which would introduce the "bug" of having players with seemingly identical colors. Reproduce with prior versions:
    • select skirmish map median oasis (4)
    • make player 3 teal
    • => two players are teal, but not the exact same teal! argh!!11
      rgb(50, 170, 170)
      rgb(64, 163, 191)
  • Fixed edge-case of the "duplicate color" bug described in comment:18 by processing the array in reverse order
  • New GUI style as proposed by mimo (see http://www.imagebam.com/image/856651436341384)
    • Removed the grey overlay sprite of the dropdown list
    • Resized the element, so that the rectangle is invisible unless opening the list
  • Some more simplifications and cleanup
Last edited 7 years ago by elexis (previous) (diff)

comment:21 by stanislas69, 7 years ago

I'm not very fond of the new layout but if the function is here I'm fine.

For skirmish maps see comment 9.

by elexis, 7 years ago

Attachment: 1580.13.diff added

There you go, skirmish supported again. Colors that are not identical to the colors in player_defaults.json will be replaced with the closest match!

comment:22 by stanislas69, 7 years ago

Thx great work :D

by elexis, 7 years ago

Attachment: 1580.14.patch added

Ensures player colors are unique after matching to player_defaults.json.

comment:23 by mimo, 7 years ago

Resolution: fixed
Status: newclosed

In 17040:

change player color in gamesetup in random and skirmish maps, patch by elexis and stanislas69, fixes #1580

comment:24 by mimo, 7 years ago

Keywords: review removed

Thanks for the patch, that's a nice feature.

comment:25 by elexis, 7 years ago

Refs three bugs: #3415 #3431 #3571

comment:26 by elexis, 6 years ago

In 18739:

Gamesetup player assignment column header position adjustment for the smallest supported resolution 1024x768.

Increase the size of the color label by stealing some space from neighboring fields,
so it works for all translations with the minimum resolution, fixes #3761, refs #1580.

Right-align civ-info and reset buttons with the dropdowns for consistent appearance, refs #3805.
Ensure column headers/buttons can't overlap by giving neighboring fields identical left/right values.

Note: See TracTickets for help on using tickets.