Skip to content

Civ primary and secondary colors#887

Open
stavrosfa wants to merge 3 commits intoC7-Game:Developmentfrom
stavrosfa:enhancement/civ-colours
Open

Civ primary and secondary colors#887
stavrosfa wants to merge 3 commits intoC7-Game:Developmentfrom
stavrosfa:enhancement/civ-colours

Conversation

@stavrosfa
Copy link
Contributor

Implemented primary and secondary colors, with some functionality to try and avoid duplicates.

Also moved the shader caching, as well as the new color calculations to a new Util class as they are specific to the gameplay and not the pure loading of files/colors

…try and avoid duplicates.

Also moved the shader caching as well as the new color calculations to a new Util class as they are specific to the gameplay and not the pure loading of files/colors
private static Dictionary<int, ShaderMaterial> materialCache = [];
private static Dictionary<ID, int> colorsInUseCache = [];

public static int GetPlayerColor(this Player player) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you document what the returned int is? Is it the key in the material cache?

if (colorsInUseCache.TryGetValue(player.id, out int colorIndex))
return colorIndex;

if (colorsInUseCache.ContainsValue(player.primaryColorIndex)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Would this be an accurate comment?

// If the player's primary color is in use, fall back to their secondary color.
// This may already be in use, but that's ok; we don't have a third
// color for each civ to fall back to.

Copy link
Contributor

Choose a reason for hiding this comment

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

And maybe document how this interacts with the second run in InitializeCivColors?

@stavrosfa
Copy link
Contributor Author

I added some documentation, let me know if it makes sense

@TomWerner
Copy link
Contributor

Perfect, thanks!

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants