-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Apply player relationship colors to all player colors #20699
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.
OpenRA.Mods.Common/Widgets/Logic/Settings/DisplaySettingsLogic.cs
Outdated
Show resolved
Hide resolved
OpenRA.Mods.Common/Widgets/Logic/Settings/DisplaySettingsLogic.cs
Outdated
Show resolved
Hide 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.
OpenRA.Game/Player.cs
Outdated
readonly Color color; | ||
|
||
/// <returns> Player color with relationship colors applied </returns> | ||
public Color Color { get; private set; } | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why have a field color
, a property Color
and a method GetColor
to access color
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then this should be { get; init; }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how would that make sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because of the readonly field, but we don't have support for this yet anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
@@ -244,28 +249,40 @@ public bool IsAlliedWith(Player p) | |||
return RelationshipWith(p) == PlayerRelationship.Ally; | |||
} | |||
|
|||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not supposed to be used. It's there only for the lua interface and setting up the game state
var renderLength = length - skip; | ||
if (renderLength <= 1) | ||
return; | ||
|
||
var screenWidth = wr.ScreenVector(new WVec(1, 0, 0))[0]; | ||
var wcr = Game.Renderer.WorldRgbaColorRenderer; | ||
|
||
var startColor = this.startColor; | ||
if (usePlayerStartColor) | ||
startColor = Color.FromArgb(this.startColor.A, Player.ActorColor(owner)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These could use CachedTransform
s instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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