Apply player relationship colors to all player colors#20699
Apply player relationship colors to all player colors#20699Mailaender merged 5 commits intoOpenRA:bleedfrom
Conversation
Display player names as part of the timer.
Allow multiple (e.g. 8) to be set, then loop through them. |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
fa2468d to
6b59e6b
Compare
This comment was marked as resolved.
This comment was marked as resolved.
6b59e6b to
bbfaca3
Compare
bbfaca3 to
dff388b
Compare
This comment was marked as resolved.
This comment was marked as resolved.
| readonly Color color; | ||
|
|
||
| /// <returns> Player color with relationship colors applied </returns> | ||
| public Color Color { get; private set; } | ||
|
|
There was a problem hiding this comment.
Why have a field color, a property Color and a method GetColor to access color?
There was a problem hiding this comment.
For a multitude of reasons.
color is the actual color assigned to the player, and we do not want any code to use this value. Baring a few exception where a player instance is saved / transferred. For those cases GetColor exists
As for Color, it is basically a performance cheap way to apply various colors to the player. Currently it only happens once at the start of the game, but it should be dynamic in the future
There was a problem hiding this comment.
Then this should be { get; init; }
There was a problem hiding this comment.
how would that make sense?
There was a problem hiding this comment.
Because of the readonly field, but we don't have support for this yet anyway.
There was a problem hiding this comment.
it isn't readonly, it's set way after initialisation. And in the future we will want to set it multiple times
There was a problem hiding this comment.
The field above this property.
c7e91c1 to
488c179
Compare
This comment was marked as resolved.
This comment was marked as resolved.
6601029 to
1c84480
Compare
|
|
||
| public static Color PlayerRelationshipColor(Actor a) | ||
| /// <summary>Returns <see cref="color"/>, ignoring player relationship colors.</summary> | ||
| public static Color GetColor(Player p) => p.color; |
There was a problem hiding this comment.
Having p.Color return the colour with relationship colours and Player.GetColor(p) return the colour without relationship colours feels quite obtuse.
Can we please have a single method which takes an argument as to whether relationship colours should be used or ignored?
There was a problem hiding this comment.
It's not supposed to be used. It's there only for the lua interface and setting up the game state
|
|
||
| var startColor = this.startColor; | ||
| if (usePlayerStartColor) | ||
| startColor = Color.FromArgb(this.startColor.A, Player.ActorColor(owner)); |
There was a problem hiding this comment.
These could use CachedTransforms instead.
There was a problem hiding this comment.
there's no clean way to implement it
032148d to
3af9b7b
Compare
|
This feature should be always disabled in Replays and for spectators. Otherwise all players will have same color. |
it's disabled now
no, red & blue is not a good default color scheme, especially for color blind people |
|
the original version had allowed the user to set colors to bet set to whatever he wished. That functionality was removed for this PR #20699 (comment) the current branch as well as bleed has these colors set in chrome metrics |
|
I see. According to https://tl.net/forum/starcraft-2/423420-new-color-mode-with-patch-2010 StarCraft II also defaults to red/green with red/blue and orange/blue as color-blind options while we in theory have infinite options then. I am fine with that. |

Related #15027, #15833, #7497, #2053
I'm additionally removing the colouring on healthbars. Now with units being accordingly colored, having healthbars match feels weird. Also the removal of contrails color cache might need a rethink, though it should not really cause any performance concerns.
This PR can be seen as a jumping off point for future ingame player color changing PR's