HomeWildfire Games

Update the color of the selection ring, rallypoints and territory outline when…
AuditedrP19965

Description

Update the color of the selection ring, rallypoints and territory outline when the playercolor is changed in atlas.
Merge duplicate Selectable component playercolor code.

Differential Revision: https://code.wildfiregames.com/D687
Fixes #4643
Refs D623
Patch in cooperation with: Stan

Event Timeline

elexis added inline comments.Aug 11 2017, 10:02 PM
/ps/trunk/source/simulation2/components/CCmpSelectable.cpp
422

(The querying of the cmpPlayerManager could be done inside the if-statement to save few cycles when selecting actors in atlas.)

temple raised a concern with this commit.Nov 1 2017, 5:38 PM
temple added a subscriber: temple.

I submitted this comment to the wrong commit, so I'm now adding it to the correct one:

Start atlas, place a unit for player one, change his civ, then start the game. There's errors starting with:

ERROR: Error calling component script function GetColor

When you change the civ and attempt to start the game, for some reason (which I didn't explore) it changes ownership to gaia before the changing ownership back to player 1. In that second step it calls GetColor but the player color isn't defined yet (again for some reason that I didn't explore). This all happens before session starts, I think.

I guess when you change a player's civ the color of his unit is invalidated? Why would that be? Or at least it's invalidated before the session starts.

A quick "fix" is to use this in Player.js:

Player.prototype.GetColor = function()
{
	return this.color || { "r": 1.0, "g": 0.0, "b": 1.0, "a": 1.0 };
};

The units will be pink on the minimap, but it won't give errors.
If you reset and place new units, then play again, the new units will have correct color on the minimap but the old ones will still be pink.

(I had problems with the minimap in D754, too.)

(I believe this patch is the problem, I reverse-diffed just this one and everything was fine.)

elexis replied:

(Atlas support for changing colors and civs was never or never well supported before alpha 23 (and still is not completely supported as IIRC there are other bugs when doing things with these properties and running the simulation, or for instance reducing the number of players).)

This commit now has outstanding concerns.Nov 1 2017, 5:38 PM
elexis requested verification of this commit.Jan 22 2018, 12:18 AM

After some hours I found that the only reason why we have to change the player civ in order to trigger the error is that LoadPlayerSettings of simulation/helpers/Player.js compares each entity civ with the player civ GetCurrentTemplateName and only calls cmpPlayer.ReplacePlayer if it doesn't match.

After some more hours I found that it only throws an error because ReplacePlayer is called after cmpPlayer.Init but before cmpPlayer.SetColor.

Copying over the playercolor in ReplacePlayer solves the issue correctly without specifying a fallback.

This commit now requires verification by auditors.Jan 22 2018, 12:18 AM
temple accepted this commit.Jan 22 2018, 1:15 AM

Thanks.

All concerns with this commit have now been addressed.Jan 22 2018, 1:15 AM