Skip to content
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

Merged
merged 5 commits into from
May 4, 2024

Conversation

PunkPun
Copy link
Member

@PunkPun PunkPun commented Feb 25, 2023

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

@pchote
Copy link
Member

pchote commented Feb 25, 2023

you want to be able to differentiate between super weapon timers

Display player names as part of the timer.

Add variance
Find a way to have relationship colors when spectating

Allow multiple (e.g. 8) to be set, then loop through them.

@Mailaender

This comment was marked as resolved.

@PunkPun

This comment was marked as resolved.

@PunkPun PunkPun force-pushed the accesibility branch 3 times, most recently from fa2468d to 6b59e6b Compare February 28, 2023 09:59
penev92

This comment was marked as resolved.

@PunkPun

This comment was marked as resolved.

Mailaender
Mailaender previously approved these changes Feb 28, 2023
@penev92

This comment was marked as resolved.

Comment on lines 54 to 60
readonly Color color;

/// <returns> Player color with relationship colors applied </returns>
public Color Color { get; private set; }

Copy link
Contributor

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?

Copy link
Member Author

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

Copy link
Member

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; }

Copy link
Member Author

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?

Copy link
Member

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.

Copy link
Member Author

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

Copy link
Member

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.

@Mailaender

This comment was marked as resolved.

@PunkPun PunkPun force-pushed the accesibility branch 2 times, most recently from 6601029 to 1c84480 Compare October 27, 2023 10:49
@@ -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;
Copy link
Member

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?

Copy link
Member Author

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

OpenRA.Game/Player.cs Outdated Show resolved Hide resolved
OpenRA.Game/Player.cs Outdated Show resolved Hide resolved
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));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These could use CachedTransforms instead.

Copy link
Member Author

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

@Porenutak
Copy link
Contributor

Porenutak commented Feb 2, 2024

This feature should be always disabled in Replays and for spectators. Otherwise all players will have same color.
Side note, wouldn't blue color suit better for player allies?

@PunkPun
Copy link
Member Author

PunkPun commented Feb 3, 2024

This feature should be always disabled in Replays and for spectators. Otherwise all players will have same color.

it's disabled now

Side note, wouldn't blue color suit better for player allies?

no, red & blue is not a good default color scheme, especially for color blind people

@Mailaender
Copy link
Member

The branch is called accesibility yet when enabling this I get red vs. green remap which is indistinguishable by the most common form of color blindness where I live.

grafik

@PunkPun
Copy link
Member Author

PunkPun commented May 4, 2024

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

@Mailaender
Copy link
Member

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.

@Mailaender Mailaender merged commit 6b463f9 into OpenRA:bleed May 4, 2024
3 checks passed
@PunkPun PunkPun deleted the accesibility branch May 4, 2024 14:32
@Mailaender
Copy link
Member

Changelog
FAQ

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants