Changeset View
Standalone View
binaries/data/mods/public/simulation/components/Player.js
| Show All 16 Lines | Player.prototype.Schema = | ||||
| "<element name='SharedDropsitesTech' a:help='Allies will share dropsites when this technology is researched. Leave empty to never share dropsites.'>" + | "<element name='SharedDropsitesTech' a:help='Allies will share dropsites when this technology is researched. Leave empty to never share dropsites.'>" + | ||||
| "<text/>" + | "<text/>" + | ||||
| "</element>" + | "</element>" + | ||||
| "<element name='SpyCostMultiplier'>" + | "<element name='SpyCostMultiplier'>" + | ||||
| "<ref name='nonNegativeDecimal'/>" + | "<ref name='nonNegativeDecimal'/>" + | ||||
| "</element>"; | "</element>"; | ||||
| /** | /** | ||||
| * Don't serialise diplomacyColor since it's modified by the GUI | |||||
elexis: (serialize, stuff should be en-US for global consistency, we had some colour -> color commits… | |||||
| */ | |||||
| Player.prototype.Serialize = function() | |||||
| { | |||||
| let data = {}; | |||||
| for (let key in this) | |||||
| if (this.hasOwnProperty(key) && key != "diplomacyColor") | |||||
| data[key] = this[key]; | |||||
| return data; | |||||
| }; | |||||
Not Done Inline ActionsI suppose after this patch, panelEntities can be removed too from serialization and the panelEntityClasses hardcoding could be deleted, passed from the GUI. elexis: I suppose after this patch, panelEntities can be removed too from serialization and the… | |||||
Done Inline Actionss/this/state elexis: s/this/state | |||||
Done Inline Actionsvar -> let elexis: var -> let | |||||
Done Inline ActionsI'm not convinced that one gets away with not introducing a Deserialize function. This one proves that the diplomacy color key are undefined after the deserialization, rather than having the value from init (for example false for displayDiplomacyColor): Player.prototype.OnGlobalDeserialized = function(msg) { warn(uneval(Object.keys(this))) };The CStdDeserializer::ReadScriptVal defines the default deserialize, which just seems to copy over all properties, but not sure if that is extending or replacing the previous object without looking further. Looking at the TechnologyManager (which doesn't have a deserialize function either), it sets the Init default for the one 'non-serialized' elexis: I'm not convinced that one gets away with not introducing a Deserialize function.
This one… | |||||
| /** | |||||
Done Inline Actionsright elexis: right | |||||
| * Which units will be shown with special icons at the top. | * Which units will be shown with special icons at the top. | ||||
| */ | */ | ||||
| var panelEntityClasses = "Hero Relic"; | var panelEntityClasses = "Hero Relic"; | ||||
| Player.prototype.Init = function() | Player.prototype.Init = function() | ||||
| { | { | ||||
| this.playerID = undefined; | this.playerID = undefined; | ||||
| this.name = undefined; // define defaults elsewhere (supporting other languages) | this.name = undefined; // define defaults elsewhere (supporting other languages) | ||||
| this.civ = undefined; | this.civ = undefined; | ||||
| this.color = undefined; | this.color = undefined; | ||||
| this.diplomacyColor = undefined; | |||||
Not Done Inline ActionsNaming: color? playerColor? Original sounds like it can be overwritten elexis: Naming: color? playerColor? Original sounds like it can be overwritten | |||||
Not Done Inline ActionscurrColor or currentColor maybe ? Stan: currColor or currentColor maybe ? | |||||
| this.popUsed = 0; // population of units owned or trained by this player | this.popUsed = 0; // population of units owned or trained by this player | ||||
| this.popBonuses = 0; // sum of population bonuses of player's entities | this.popBonuses = 0; // sum of population bonuses of player's entities | ||||
| this.maxPop = 300; // maximum population | this.maxPop = 300; // maximum population | ||||
| this.trainingBlocked = false; // indicates whether any training queue is currently blocked | this.trainingBlocked = false; // indicates whether any training queue is currently blocked | ||||
| this.resourceCount = {}; | this.resourceCount = {}; | ||||
| this.tradingGoods = []; // goods for next trade-route and its proba in % (the sum of probas must be 100) | this.tradingGoods = []; // goods for next trade-route and its proba in % (the sum of probas must be 100) | ||||
| this.team = -1; // team number of the player, players on the same team will always have ally diplomatic status - also this is useful for team emblems, scoring, etc. | this.team = -1; // team number of the player, players on the same team will always have ally diplomatic status - also this is useful for team emblems, scoring, etc. | ||||
| this.teamsLocked = false; | this.teamsLocked = false; | ||||
| ▲ Show 20 Lines • Show All 83 Lines • ▼ Show 20 Lines | Player.prototype.SetColor = function(r, g, b) | ||||
| // Used in Atlas | // Used in Atlas | ||||
| if (colorInitialized) | if (colorInitialized) | ||||
| Engine.BroadcastMessage(MT_PlayerColorChanged, { | Engine.BroadcastMessage(MT_PlayerColorChanged, { | ||||
| "player": this.playerID | "player": this.playerID | ||||
| }); | }); | ||||
| }; | }; | ||||
| Player.prototype.SetDiplomacyColor = function(r, g, b) | |||||
| { | |||||
| this.diplomacyColor = { "r": r/255.0, "g": g/255.0, "b": b/255.0, "a": 1.0 }; | |||||
elexisUnsubmitted Done Inline Actionsremove .0 elexis: remove .0 | |||||
templeAuthorUnsubmitted Not Done Inline Actions(I didn't know if they're needed or not, but it was in SetColor() so I copied.) temple: (I didn't know if they're needed or not, but it was in SetColor() so I copied.) | |||||
| Engine.BroadcastMessage(MT_PlayerColorChanged, { | |||||
| "player": this.playerID | |||||
| }); | |||||
elexisUnsubmitted Not Done Inline ActionsSmells like OOS: If this is true, don't use a simulation message but just call some update function in the affected components from the gui interface. elexis: Smells like OOS:
player 1 choses to view diplomacy colors and sends a synchronized, hashed… | |||||
templeAuthorUnsubmitted Not Done Inline ActionsI've whaled on the diplomacy button and haven't gotten any out of syncs, so I don't know. temple: I've whaled on the diplomacy button and haven't gotten any out of syncs, so I don't know.
(See… | |||||
elexisUnsubmitted Not Done Inline ActionsJust broadcasting a message by itself for one client but not another client ought to give an OOS if my expectation is accurate. One (I can try too if you prefer) could prove or disprove it in multiple ways: Alternative 1: On unix one can start an MP game in one pyrogenesis instance and join in a second instance. If one client broadcasts this message and different client doesn't and those simulation messages are indeed serialized and synced, then there should be an OOS dialog within 20 seconds I believe. Alternative 2: Starting an MP game, playing with diplo colors or not. Replaything that thing and looking if there is an OOS when toggling diplo colors within the same time as in 1. In singleplayer, replays don't contain hash sums of the simstate, only the MP ones store them. Alternative 3: Read a textual dump of the simstate (aka savegame) and read if the message numbers are in there. Did you try 1 already? Also I expect Engine.BroadcastMessage only to work for players, not multiplayer clients with playerID -1 (non-player observers) and replay viewers. Your screenshot disproves my theory or did you take it after having resigned? Hm, looks like Engine.BroadcastMessage directly calls into CComponentManager::BroadcastMessage without using any turn manager or such. @leper amirite that we should avoid broadcasting a simulation message to change GUI colors (even if the subscribers of the simulation message don't change the simstate) and instead call a setter in the GUIInterface.js that calls the color updaters in the simulation components? elexis: Just broadcasting a message by itself for one client but not another client ought to give an… | |||||
leperUnsubmitted Not Done Inline ActionsIf the sending of the message is caused by a network command, then that is fine (since all players get the same command). Given that the player colours are not synchronized anymore that should most likely work, once that is handled with network commands, instead of using GuiInterfaceCalls, which are not synchronized (which is one of the reasons why player colours are not synchronized anymore). What I'm mostly wondering is why we don't store both player and diplomacy colours (and serialize them), and just toggle something non-synchronized using a GuiInterfaceCall to use one or the other when displaying. leper: If the sending of the message is caused by a network command, then that is fine (since all… | |||||
elexisUnsubmitted Not Done Inline ActionsThe call in question is Engine.BroadcastMessage(MT_PlayerColorChanged. This does _not_ send a netowrked simulation message but directly inserts a simulation message of that type into the ComponentManager. There is no call in the entire GUI doing that - until now (but many in the simulation). Also it should not use the simulation system, because it is a GUI setting and different per client. Observers can't send simulation commands. Like every other GUI setting it should update instantly, not only every turn. GUI settings aren't send as simulation commands. The last sentence about you wondering why we dont use GUIInterface is precisely what I was asking :-) elexis: The call in question is `Engine.BroadcastMessage(MT_PlayerColorChanged`. This does _not_ send a… | |||||
leperUnsubmitted Not Done Inline ActionsNo, you are too deep down the callstack to make that the point. The issue here is that we go from GuiInterfaceCall to changing simulation state (that should be synchronized), one part of this is broadcasting a message that might end up changing other simulation state (which in the best case would be synchronized and tell us that someone did something wrong (aka OOS)). Well, since parts of the involved code is in the simulation (submitting the colours to the renderer) it is likely that the simulation is going to be involved a little. So having a non-synchronized variable that submits either the diplo colours, or the player colours (but makes sure to retrieve both, so changes to those don't break this). Or making this purely a GUI thing, and not changing the simulation at all. Not quite a yes, since if I read that last part correctly it sounds like you are also suggesting something like what is done in the current patch, which still breaks if anything synchronized is involved. Basically what I'm suggesting is having both colours for every player, then having a non-serialized boolean in the C++ components that do any rendering based on player colour, these can be changed from a GuiInterfaceCall, and those components just use the other set of colours based on that boolean. leper: No, you are too deep down the callstack to make that the point. The issue here is that we go… | |||||
elexisUnsubmitted Not Done Inline ActionsThanks for your response! My primary concern was triggering non-serialized simulation code by broadcasting a simulation message from the GUI as opposed to triggering that non-serialized code from the GUIInterface. Didn't really refer in this statement to whatever code was triggered followed by that. I'm not sure if the broadcast of the simulation message itself already isn't sufficient to trigger an OOS. Yes, people must be careful not to derive serialized data from non-serialized data either way. Why add a separate boolean to each C++ component however when all components can query the same source of truth - a single JS boolean (or color as is done here or similar)? elexis: Thanks for your response!
My primary concern was triggering non-serialized simulation code by… | |||||
leperUnsubmitted Not Done Inline ActionsWhy should messages be serialized? The commands that cause them are sent to everyone, and any change from the messages is serialized. Mostly to save that C++->JS call that'd most likely be done each frame, which sounds like a bad idea. leper: Why should messages be serialized? The commands that cause them are sent to everyone, and any… | |||||
| }; | |||||
| Player.prototype.GetColor = function() | Player.prototype.GetColor = function() | ||||
| { | { | ||||
| return this.color; | return this.diplomacyColor || this.color; | ||||
Not Done Inline ActionsWe expect functions in the simulation to be deterministic / synchronized by default too and the name Player.GetColor isn't clearly showing that it's not the property Color that was set at the beginning. It should be GetDisplayedColor to indicate GUI context (indeterministic values) (Assuming such a getter is going to be used) elexis: We expect functions in the simulation to be deterministic / synchronized by default too and the… | |||||
Not Done Inline ActionsThis function still gives me a bit stomach ache. Not only that simulation coders might assume that it returns something synchronized (which is the default in the simulation), but also it's not clear from the function name that it can also return the diplomacy color. I'd propose to keep GetColor returning the player color and adding a GetDisplayedColor to highlight the difference. (One of the ideas leper mentioned was to push the color into all components, this could perhaps even be done from the GUIInterface and the Player component could be agnostic of diplomacyColors. But that would have the disadvantage that the GUIInterface has to know about all components that should use the color (whereas the components pulling the color from the player component doesn't have that issue and since it's already the case - not a performance regression).) elexis: This function still gives me a bit stomach ache. Not only that simulation coders might assume… | |||||
Not Done Inline Actions... It seems pretty crazy to me, because we never want to get the player's original color, we always want to get their displayed color. Your GetColor would never be used, it'd be GetDisplayedColor everywhere. (We only use the original color itself for serialization.) It seems easier to just stop thinking of the "player color" as only his original color, and instead as the displayed color, which could be the original color or could be the diplomacy color (which can change throughout a game). Obviously we could rename every instance of GetColor to GetDisplayedColor to emphasize that fact, it just seems a little overkill, to me anyway. Well, maybe there's only like a dozen instances of GetColor, so I guess not as big of a deal as I'm making it out to be. temple: ...
It seems pretty crazy to me, because we never want to get the player's original color, we… | |||||
| }; | }; | ||||
| // Try reserving num population slots. Returns 0 on success or number of missing slots otherwise. | // Try reserving num population slots. Returns 0 on success or number of missing slots otherwise. | ||||
| Player.prototype.TryReservePopulationSlots = function(num) | Player.prototype.TryReservePopulationSlots = function(num) | ||||
| { | { | ||||
| if (num != 0 && num > (this.GetPopulationLimit() - this.GetPopulationCount())) | if (num != 0 && num > (this.GetPopulationLimit() - this.GetPopulationCount())) | ||||
| return num - (this.GetPopulationLimit() - this.GetPopulationCount()); | return num - (this.GetPopulationLimit() - this.GetPopulationCount()); | ||||
| ▲ Show 20 Lines • Show All 788 Lines • Show Last 20 Lines | |||||
(serialize, stuff should be en-US for global consistency, we had some colour -> color commits and the Armour component still has to be renamed sometime)