Page MenuHomeWildfire Games
Differential D754 Diff 4065 binaries/data/mods/public/simulation/components/GuiInterface.js

Changeset View

Standalone View

View Options

binaries/data/mods/public/simulation/components/GuiInterface.js

function GuiInterface() {} function GuiInterface() {}
GuiInterface.prototype.Schema = GuiInterface.prototype.Schema =
"<a:component type='system'/><empty/>"; "<a:component type='system'/><empty/>";
GuiInterface.prototype.Serialize = function() GuiInterface.prototype.Serialize = function()
{ {
// This component isn't network-synchronised for the biggest part // This component isn't network-synchronised for the biggest part
// So most of the attributes shouldn't be serialized // So most of the attributes shouldn't be serialized
// Return an object with a small selection of deterministic data // Return an object with a small selection of deterministic data
return { return {
elexisUnsubmitted
Done
Inline Actions

We don't want globals in simulation prototypes. Also you're passing the colors from the GUI to the GUI interface as an argument and then don't use that argument. So nuke this global and use the argument.

elexis: We don't want globals in simulation prototypes. Also you're passing the colors from the GUI to…
templeAuthorUnsubmitted
Done
Inline Actions

I was only passing a bool saying whether to use diplomacy colors or not. But I suppose I could send all the colors instead, if that's a better way of doing it.

temple: I was only passing a bool saying whether to use diplomacy colors or not. But I suppose I could…
"timeNotifications": this.timeNotifications, "timeNotifications": this.timeNotifications,
"timeNotificationID": this.timeNotificationID "timeNotificationID": this.timeNotificationID
}; };
}; };
GuiInterface.prototype.Deserialize = function(data) GuiInterface.prototype.Deserialize = function(data)
{ {
this.Init(); this.Init();
▲ Show 20 Lines • Show All 69 Lines • ▼ Show 20 Lines for (let j = 0; j < numPlayers; ++j)
mutualAllies[j] = cmpPlayer.IsMutualAlly(j); mutualAllies[j] = cmpPlayer.IsMutualAlly(j);
neutrals[j] = cmpPlayer.IsNeutral(j); neutrals[j] = cmpPlayer.IsNeutral(j);
enemies[j] = cmpPlayer.IsEnemy(j); enemies[j] = cmpPlayer.IsEnemy(j);
} }
ret.players.push({ ret.players.push({
"name": cmpPlayer.GetName(), "name": cmpPlayer.GetName(),
"civ": cmpPlayer.GetCiv(), "civ": cmpPlayer.GetCiv(),
"color": cmpPlayer.GetColor(), "color": cmpPlayer.GetColor(),
elexisUnsubmitted
Not Done
Inline Actions

This should never return the diplo color, right?

elexis: This should never return the diplo color, right?
templeAuthorUnsubmitted
Not Done
Inline Actions

Depends if you want to rewrite the GUI or not. Session grabs the player color from here and uses that everywhere in the GUI, so it should be the same as the displayed color. (In session I set g_Players[i].color directly rather than doing onSimulationUpdate because I don't need to update all this other data plus I need to calculate the array of diplomacy colors anyway.)

temple: Depends if you want to rewrite the GUI or not. Session grabs the player color from here and…
templeAuthorUnsubmitted
Not Done
Inline Actions

(Sorry, it wasn't onSimulationUpdate but updatePlayerData that refreshes the colors.)

temple: (Sorry, it wasn't onSimulationUpdate but updatePlayerData that refreshes the colors.)
"controlsAll": cmpPlayer.CanControlAllUnits(), "controlsAll": cmpPlayer.CanControlAllUnits(),
"popCount": cmpPlayer.GetPopulationCount(), "popCount": cmpPlayer.GetPopulationCount(),
"popLimit": cmpPlayer.GetPopulationLimit(), "popLimit": cmpPlayer.GetPopulationLimit(),
"popMax": cmpPlayer.GetMaxPopulation(), "popMax": cmpPlayer.GetMaxPopulation(),
"panelEntities": cmpPlayer.GetPanelEntities(), "panelEntities": cmpPlayer.GetPanelEntities(),
"resourceCounts": cmpPlayer.GetResourceCounts(), "resourceCounts": cmpPlayer.GetResourceCounts(),
"trainingBlocked": cmpPlayer.IsTrainingBlocked(), "trainingBlocked": cmpPlayer.IsTrainingBlocked(),
"state": cmpPlayer.GetState(), "state": cmpPlayer.GetState(),
▲ Show 20 Lines • Show All 762 Lines • ▼ Show 20 Lines if (!cmpBuilder)
continue; continue;
for (let building of cmpBuilder.GetEntitiesList()) for (let building of cmpBuilder.GetEntitiesList())
if (buildableEnts.indexOf(building) == -1) if (buildableEnts.indexOf(building) == -1)
buildableEnts.push(building); buildableEnts.push(building);
} }
return buildableEnts; return buildableEnts;
}; };
GuiInterface.prototype.UpdatePlayerColors = function(player, data)
{
elexisUnsubmitted
Done
Inline Actions

(can be shortened by one line)

elexis: (can be shortened by one line)
for (let i = 0; i < data.length; ++i)
{
let cmpPlayer = QueryPlayerIDInterface(i, IID_Player);
if (!cmpPlayer)
continue;
elexisUnsubmitted
Not Done
Inline Actions

could pass data[i]

elexis: could pass `data[i]`
templeAuthorUnsubmitted
Not Done
Inline Actions

SetColor is done this way so I was being consistent.

temple: SetColor is done this way so I was being consistent.
cmpPlayer.SetDiplomacyColor(data[i].r, data[i].g, data[i].b);
}
};
GuiInterface.prototype.SetSelectionHighlight = function(player, cmd) GuiInterface.prototype.SetSelectionHighlight = function(player, cmd)
elexisUnsubmitted
Done
Inline Actions

Gaia entities are not update (aka initialized?) anymore

elexis: Gaia entities are not update (aka initialized?) anymore
StanUnsubmitted
Done
Inline Actions

I remember leper saying comments for getter and setter were useless . has the policy changed ?

Stan: I remember leper saying comments for getter and setter were useless . has the policy changed ?
elexisUnsubmitted
Not Done
Inline Actions

Keeping MT_PlayerColorChanged while adding an MT_DisplayedPlayerColorChanged or MT_AsyncDisplayedPlayerColorChanged changed event might be sufficient to stop simulation authors from executing serialization relevant code in the processing of that event and would allow removing the hardcoded component names in the GUIInterface however.

We also remember the idea to push the color from the GUI Interface to the components Update function or message (this way it wouldn't have to be saved in the Player component and the serialization function wouldn't have to be changed).

But the current code is sufficient and good (and has the advantage of not having it possible for people to misuse the MT_..)

elexis: Keeping `MT_PlayerColorChanged` while adding an `MT_DisplayedPlayerColorChanged` or…
{ {
templeAuthorUnsubmitted
Done
Inline Actions

extra line

temple: extra line
let playerColors = {}; // cache of owner -> color map let playerColors = {}; // cache of owner -> color map
elexisUnsubmitted
Not Done
Inline Actions

Reading this function, one might still think it's cleaner to pass the diplomacy color as an argument here.

In case the diplomacyColor display is disabled, the GUIInterface would have to either pass the playerColor to the simulation components or the simulation components conditionally get their color from two different components.
Both seem worse choices than what is proposed here (making the conditionally playercolor-using components pull their color from the player component).

Not sure if avoiding the hardcoded simulation component names by sending an MT_NonSyncPlayerColor simulation message would be worth the invitation to add a non-synced code. Also no clue about the performance overhead of the messages.

elexis: Reading this function, one might still think it's cleaner to pass the diplomacy color as an…
for (let ent of cmd.entities) for (let ent of cmd.entities)
{ {
let cmpSelectable = Engine.QueryInterface(ent, IID_Selectable); let cmpSelectable = Engine.QueryInterface(ent, IID_Selectable);
if (!cmpSelectable) if (!cmpSelectable)
continue; continue;
// Find the entity's owner's color: // Find the entity's owner's color:
let owner = -1; let owner = -1;
elexisUnsubmitted
Done
Inline Actions

INVALID_PLAYER is the observer playerID here? I suspect we want an own constant for that, (I've left it at -1 in some observer places iirc since not sure)

The condition is independent of the entityID, so it can be computed once outside of the loop. And if we do that already, we might just as well construct the IID array which is also independent of the entityID outside of the loop too. And then we can add an UpdateColor funciton to cmpStatusBars that calls RegenerateSprites (and possibly more in the future) to unify that part under the IID loop too.

elexis: INVALID_PLAYER is the observer playerID here? I suspect we want an own constant for that, (I've…
let cmpOwnership = Engine.QueryInterface(ent, IID_Ownership); let cmpOwnership = Engine.QueryInterface(ent, IID_Ownership);
if (cmpOwnership) if (cmpOwnership)
owner = cmpOwnership.GetOwner(); owner = cmpOwnership.GetOwner();
let color = playerColors[owner]; let color = playerColors[owner];
elexisUnsubmitted
Not Done
Inline Actions

(Sending a simulation event is indeed something that nicely avoids having to specify to hardcode the components that need to be informed.
It's just that every simulation event is currently considered to be deterministic, including PlayerColorChanged.
So there apparently isnt an OOS but the design was broken IMO.
Perhaps we could implement a specific type for GUI events, so that it becomes clear that this is indeterministic.)

elexis: (Sending a simulation event is indeed something that nicely avoids having to specify to…
if (!color) if (!color)
elexisUnsubmitted
Not Done
Inline Actions

Maybe that becomes more readable (some irrelevant nanoseconds more performant) ìf we don't pass an IID array but call updateEntityColors for one IID. The entities wouldn't be updated one by one anymore. Anyway, very good code as is.

elexis: Maybe that becomes more readable (some irrelevant nanoseconds more performant) ìf we don't pass…
{ {
color = { "r":1, "g":1, "b":1 }; color = { "r":1, "g":1, "b":1 };
let cmpPlayer = QueryPlayerIDInterface(owner); let cmpPlayer = QueryPlayerIDInterface(owner);
if (cmpPlayer) if (cmpPlayer)
color = cmpPlayer.GetColor(); color = cmpPlayer.GetColor();
elexisUnsubmitted
Not Done
Inline Actions

(Using a ternary or else-statement would only assign a value if its used)

elexis: (Using a ternary or else-statement would only assign a value if its used)
elexisUnsubmitted
Done
Inline Actions

Looks good! packed, no redundancy. :-)
The second IID loop could be the outer loop, then the array could be inlined.
Perhaps we could create a helper function in this function:
{{{
let updateEntityColors = (iids, entityIDs) => {

for iiids...
    for entityIDs...
         cmpUpdate

};
}}}

elexis: Looks good! packed, no redundancy. :-) The second IID loop could be the outer loop, then the…
elexisUnsubmitted
Done
Inline Actions

(If it becomes a function, the IIDs can remain the inner loop too)

elexis: (If it becomes a function, the IIDs can remain the inner loop too)
playerColors[owner] = color; playerColors[owner] = color;
} }
cmpSelectable.SetSelectionHighlight({ "r": color.r, "g": color.g, "b": color.b, "a": cmd.alpha }, cmd.selected); cmpSelectable.SetSelectionHighlight({ "r": color.r, "g": color.g, "b": color.b, "a": cmd.alpha }, cmd.selected);
let cmpRangeVisualization = Engine.QueryInterface(ent, IID_RangeVisualization); let cmpRangeVisualization = Engine.QueryInterface(ent, IID_RangeVisualization);
elexisUnsubmitted
Done
Inline Actions

We should replace that -1 with a constant some day.

elexis: We should replace that -1 with a constant some day.
elexisUnsubmitted
Not Done
Inline Actions

RegenerateSprites is run twice for the same selected entity owned by a player if showAllStatusBars is true.
Wondering if a owner == gaia condition would be worth the performance.
On the other side, currently the GUI decided to prohibit gaia + other entity selection in observermode, but this can and might probably likely should change in some way eventually.

elexis: `RegenerateSprites` is run twice for the same selected entity owned by a player if…
templeAuthorUnsubmitted
Not Done
Inline Actions

We can select at most 200 units, so there's a cap on how bad this part can be (~20ms, maybe).
If observers have showAllStatusBars then that can affect a lot of units, but it doesn't seem too bad? Doing 2000 units in the editor, it's a total 100ms with showAllStatusBars and 20ms without (on my old computer).

temple: We can select at most 200 units, so there's a cap on how bad this part can be (~20ms, maybe).
elexisUnsubmitted
Not Done
Inline Actions

That 200 might become 300 eventually, not set in stone, but realistically capped yes.
Yeah not bad, just noticed it. leper said "doing pointless work is just that" in such a situation :-)
But adding the code to prevent the redundancy might make things more complex than what is neat and useful.

elexis: That 200 might become 300 eventually, not set in stone, but realistically capped yes. Yeah not…
if (!cmpRangeVisualization || player != owner && player != -1) if (!cmpRangeVisualization || player != owner && player != -1)
continue; continue;
cmpRangeVisualization.SetEnabled(cmd.selected, this.enabledVisualRangeOverlayTypes, false); cmpRangeVisualization.SetEnabled(cmd.selected, this.enabledVisualRangeOverlayTypes, false);
} }
elexisUnsubmitted
Done
Inline Actions

The entire block could be minimized by mandating the updated functions to have an UpdateDisplayedPlayerColor function and calling that in a loop over the component interface IDs.

elexis: The entire block could be minimized by mandating the updated functions to have an…
}; };
GuiInterface.prototype.EnableVisualRangeOverlayType = function(player, data) GuiInterface.prototype.EnableVisualRangeOverlayType = function(player, data)
{ {
this.enabledVisualRangeOverlayTypes[data.type] = data.enabled; this.enabledVisualRangeOverlayTypes[data.type] = data.enabled;
}; };
GuiInterface.prototype.GetEntitiesWithStatusBars = function() GuiInterface.prototype.GetEntitiesWithStatusBars = function()
▲ Show 20 Lines • Show All 877 Lines • ▼ Show 20 Lines
* @param idleclasses Array of class names to include. * @param idleclasses Array of class names to include.
* @param excludeUnits Array of units to exclude. * @param excludeUnits Array of units to exclude.
* *
* Returns an object with the following fields: * Returns an object with the following fields:
* - idle - true if the unit is considered idle by the filter, false otherwise. * - idle - true if the unit is considered idle by the filter, false otherwise.
* - bucket - if idle, set to the index of the first matching idle class, undefined otherwise. * - bucket - if idle, set to the index of the first matching idle class, undefined otherwise.
*/ */
GuiInterface.prototype.IdleUnitFilter = function(unit, idleClasses, excludeUnits) GuiInterface.prototype.IdleUnitFilter = function(unit, idleClasses, excludeUnits)
{ {
elexisUnsubmitted
Done
Inline Actions

cmpPlayer2.SetDiplomacyColor(...g_DiplomacyColors[teamColors[team] || i]);?

elexis: `cmpPlayer2.SetDiplomacyColor(...g_DiplomacyColors[teamColors[team] || i]);`?
templeAuthorUnsubmitted
Done
Inline Actions

Neat.

temple: Neat.
let cmpUnitAI = Engine.QueryInterface(unit, IID_UnitAI); let cmpUnitAI = Engine.QueryInterface(unit, IID_UnitAI);
if (!cmpUnitAI || !cmpUnitAI.IsIdle() || cmpUnitAI.IsGarrisoned()) if (!cmpUnitAI || !cmpUnitAI.IsIdle() || cmpUnitAI.IsGarrisoned())
return { "idle": false }; return { "idle": false };
let cmpIdentity = Engine.QueryInterface(unit, IID_Identity); let cmpIdentity = Engine.QueryInterface(unit, IID_Identity);
if(!cmpIdentity) if(!cmpIdentity)
return { "idle": false }; return { "idle": false };
let bucket = idleClasses.findIndex(elem => MatchesClassList(cmpIdentity.GetClassesList(), elem)); let bucket = idleClasses.findIndex(elem => MatchesClassList(cmpIdentity.GetClassesList(), elem));
if (bucket == -1 || excludeUnits.indexOf(unit) > -1) if (bucket == -1 || excludeUnits.indexOf(unit) > -1)
return { "idle": false }; return { "idle": false };
return { "idle": true, "bucket": bucket }; return { "idle": true, "bucket": bucket };
}; };
GuiInterface.prototype.GetTradingRouteGain = function(player, data) GuiInterface.prototype.GetTradingRouteGain = function(player, data)
{ {
if (!data.firstMarket || !data.secondMarket) if (!data.firstMarket || !data.secondMarket)
elexisUnsubmitted
Done
Inline Actions

We need i at most once. Are those good property names? Color changed from player X to player X?

elexis: We need `i` at most once. Are those good property names? Color changed from player X to player…
templeAuthorUnsubmitted
Done
Inline Actions

I did it this way so I wouldn't have to copy/paste cpp code since MT_OwnershipChanged was doing the same thing in Minimap and Selectable, and almost the same thing in TerritoryManager.

Maybe we could remove the MT_OwnershipChanged case in Minimap and Selectable and instead use MT_ColorChanged, and instead of from/to send the r/g/b values. It seems that MT_OwnershipChanged is only posted once, in Player.js, so it'd be easy enough to also post an MT_ColorChanged message there.

TerritoryManager is still awkward though.

temple: I did it this way so I wouldn't have to copy/paste cpp code since MT_OwnershipChanged was doing…
return null; return null;
return CalculateTraderGain(data.firstMarket, data.secondMarket, data.template); return CalculateTraderGain(data.firstMarket, data.secondMarket, data.template);
}; };
GuiInterface.prototype.GetTradingDetails = function(player, data) GuiInterface.prototype.GetTradingDetails = function(player, data)
{ {
let cmpEntityTrader = Engine.QueryInterface(data.trader, IID_Trader); let cmpEntityTrader = Engine.QueryInterface(data.trader, IID_Trader);
▲ Show 20 Lines • Show All 179 Lines • ▼ Show 20 Lineslet exposedFunctions = {
"GetNonGaiaEntities": 1, "GetNonGaiaEntities": 1,
"DisplayRallyPoint": 1, "DisplayRallyPoint": 1,
"AddTargetMarker": 1, "AddTargetMarker": 1,
"SetBuildingPlacementPreview": 1, "SetBuildingPlacementPreview": 1,
"SetWallPlacementPreview": 1, "SetWallPlacementPreview": 1,
"GetFoundationSnapData": 1, "GetFoundationSnapData": 1,
"PlaySound": 1, "PlaySound": 1,
"FindIdleUnits": 1, "FindIdleUnits": 1,
"UpdatePlayerColors": 1,
"HasIdleUnits": 1, "HasIdleUnits": 1,
"GetTradingRouteGain": 1, "GetTradingRouteGain": 1,
"GetTradingDetails": 1, "GetTradingDetails": 1,
"CanAttack": 1, "CanAttack": 1,
"GetBatchTime": 1, "GetBatchTime": 1,
"IsMapRevealed": 1, "IsMapRevealed": 1,
"SetPathfinderDebugOverlay": 1, "SetPathfinderDebugOverlay": 1,
Show All 20 Lines
Wildfire Games · Phabricator