Changeset View
Standalone View
binaries/data/mods/public/gui/session/session.js
| const g_IsReplay = Engine.IsVisualReplay(); | const g_IsReplay = Engine.IsVisualReplay(); | ||||
| const g_CivData = loadCivData(false, true); | const g_CivData = loadCivData(false, true); | ||||
| const g_Ceasefire = prepareForDropdown(g_Settings && g_Settings.Ceasefire); | const g_Ceasefire = prepareForDropdown(g_Settings && g_Settings.Ceasefire); | ||||
| const g_MapSizes = prepareForDropdown(g_Settings && g_Settings.MapSizes); | const g_MapSizes = prepareForDropdown(g_Settings && g_Settings.MapSizes); | ||||
| const g_MapTypes = prepareForDropdown(g_Settings && g_Settings.MapTypes); | const g_MapTypes = prepareForDropdown(g_Settings && g_Settings.MapTypes); | ||||
| const g_PopulationCapacities = prepareForDropdown(g_Settings && g_Settings.PopulationCapacities); | const g_PopulationCapacities = prepareForDropdown(g_Settings && g_Settings.PopulationCapacities); | ||||
| const g_StartingResources = prepareForDropdown(g_Settings && g_Settings.StartingResources); | const g_StartingResources = prepareForDropdown(g_Settings && g_Settings.StartingResources); | ||||
| const g_VictoryConditions = prepareForDropdown(g_Settings && g_Settings.VictoryConditions); | const g_VictoryConditions = prepareForDropdown(g_Settings && g_Settings.VictoryConditions); | ||||
| const g_VictoryDurations = prepareForDropdown(g_Settings && g_Settings.VictoryDurations); | const g_VictoryDurations = prepareForDropdown(g_Settings && g_Settings.VictoryDurations); | ||||
| var g_GameSpeeds; | var g_GameSpeeds; | ||||
| /** | /** | ||||
| * The displayed player colors are either the original player colors or the diplomacy colors | |||||
| * (self/ally/neutral/enemy as a player, teams as an observer), as determined by the toggle. | |||||
elexis: (since it's convention to have operators at the end of the previous line rather than at the… | |||||
| * The diplomacy color palette stores the self/ally/neutral/enemy color codes. | |||||
Done Inline Actionsevery declaration on a separate line. elexis: every declaration on a separate line.
Keeping backups ("original") feels a bit messy (). Can't… | |||||
elexisUnsubmitted Done Inline Actions(Distribute the three sentences to the three variables?) elexis: (Distribute the three sentences to the three variables?) | |||||
| */ | |||||
| var g_DisplayedPlayerColors; | |||||
Done Inline Actionssimulation? elexis: simulation? | |||||
Done Inline ActionsI don't know what you mean. Should the file be put somewhere else? temple: I don't know what you mean. Should the file be put somewhere else? | |||||
Not Done Inline Actionsdone temple: done | |||||
Not Done Inline ActionsThe original player colors shouldn't even need caching as they can never change in the sim (guess thats related to the other inline comments). (I would have chosen g_PlayerColors, g_DiplomacyColors, g_DisplayDiplomacyColors but your names are good too) elexis: The original player colors shouldn't even need caching as they can never change in the sim… | |||||
Done Inline ActionsIMO good practice to have one very short JS doc comment for each global, if one can add information (i.e. something non-obvious) elexis: IMO good practice to have one very short JS doc comment for each global, if one can add… | |||||
| var g_DiplomacyColorPalette; | |||||
| var g_DiplomacyColorsToggle = false; | |||||
Done Inline Actions
elexis: * throw new Error, because we want a stack too in case (recent commit changing throw calls)
*… | |||||
Not Done Inline Actionsdone temple: done | |||||
| /** | |||||
| * Colors to flash when pop limit reached. | * Colors to flash when pop limit reached. | ||||
| */ | */ | ||||
| var g_DefaultPopulationColor = "white"; | var g_DefaultPopulationColor = "white"; | ||||
| var g_PopulationAlertColor = "orange"; | var g_PopulationAlertColor = "orange"; | ||||
| /** | /** | ||||
| * Seen in the tooltip of the top panel. | * Seen in the tooltip of the top panel. | ||||
| */ | */ | ||||
| ▲ Show 20 Lines • Show All 240 Lines • ▼ Show 20 Lines | if (initData) | ||||
| Engine.GetGUIObjectByName("gameSpeedButton").hidden = g_IsNetworked; | Engine.GetGUIObjectByName("gameSpeedButton").hidden = g_IsNetworked; | ||||
| } | } | ||||
| else if (g_IsReplay)// Needed for autostart loading option | else if (g_IsReplay)// Needed for autostart loading option | ||||
| g_PlayerAssignments.local.player = -1; | g_PlayerAssignments.local.player = -1; | ||||
| LoadModificationTemplates(); | LoadModificationTemplates(); | ||||
| updatePlayerData(); | updatePlayerData(); | ||||
Done Inline ActionsJust a map call should be sufficient. elexis: Just a map call should be sufficient.
| |||||
| g_BarterSell = g_ResourceData.GetCodes()[0]; | g_BarterSell = g_ResourceData.GetCodes()[0]; | ||||
| initializeMusic(); // before changing the perspective | initializeMusic(); // before changing the perspective | ||||
| initSessionMenuButtons(); | initSessionMenuButtons(); | ||||
| for (let slot in Engine.GetGUIObjectByName("panelEntityPanel").children) | for (let slot in Engine.GetGUIObjectByName("panelEntityPanel").children) | ||||
| initPanelEntities(slot); | initPanelEntities(slot); | ||||
| g_DiplomacyColorPalette = Engine.ReadJSONFile(g_SettingsDirectory + "diplomacy_colors.json"); | |||||
| g_DisplayedPlayerColors = g_Players.map(player => player.color); | |||||
| updateViewedPlayerDropdown(); | updateViewedPlayerDropdown(); | ||||
Done Inline Actionsused only once -> inline elexis: used only once -> inline
Interesting call, never seen that. Nice way to avoid the fill.
Setting… | |||||
Not Done Inline Actionslist is set in updateViewPlayerNames() which is called in updatePlayerColors which is a few lines above. I actually had it here too before realizing it was repeated, but you have a point so I'll add it back. temple: list is set in `updateViewPlayerNames()` which is called in `updatePlayerColors` which is a few… | |||||
Not Done Inline ActionsFuture feature: elexis: Future feature:
Not sure if they shoudn't even be in default.cfg instead. (Would just have to… | |||||
Done Inline ActionsI believe this call is unneeded. All components start with the player color after init / deserialization, otherwise it should be considered a component bug IMO. elexis: I believe this call is unneeded. All components start with the player color after init /… | |||||
| // Select "observer" in the view player dropdown when rejoining as a defeated player | // Select "observer" in the view player dropdown when rejoining as a defeated player | ||||
| let player = g_Players[Engine.GetPlayerID()]; | let player = g_Players[Engine.GetPlayerID()]; | ||||
| Engine.GetGUIObjectByName("viewPlayer").selected = player && player.state == "defeated" ? 0 : Engine.GetPlayerID() + 1; | Engine.GetGUIObjectByName("viewPlayer").selected = player && player.state == "defeated" ? 0 : Engine.GetPlayerID() + 1; | ||||
| // If in Atlas editor, disable the exit button | // If in Atlas editor, disable the exit button | ||||
| if (Engine.IsAtlasRunning()) | if (Engine.IsAtlasRunning()) | ||||
| Engine.GetGUIObjectByName("menuExitButton").enabled = false; | Engine.GetGUIObjectByName("menuExitButton").enabled = false; | ||||
| if (hotloadData) | if (hotloadData) | ||||
| g_Selection.selected = hotloadData.selection; | g_Selection.selected = hotloadData.selection; | ||||
| Engine.SetBoundingBoxDebugOverlay(false); | Engine.SetBoundingBoxDebugOverlay(false); | ||||
| initChatWindow(); | initChatWindow(); | ||||
| sendLobbyPlayerlistUpdate(); | sendLobbyPlayerlistUpdate(); | ||||
Not Done Inline ActionsWhy is this done after updateViewedPlayerDropdown, why are g_DisplayedPlayerColors initialized twice in this function an why is updateViewedPlayerDropdown called twice on init? elexis: Why is this done after `updateViewedPlayerDropdown`, why are `g_DisplayedPlayerColors`… | |||||
| onSimulationUpdate(); | onSimulationUpdate(); | ||||
| setTimeout(displayGamestateNotifications, 1000); | setTimeout(displayGamestateNotifications, 1000); | ||||
| // Report the performance after 5 seconds (when we're still near | // Report the performance after 5 seconds (when we're still near | ||||
| // the initial camera view) and a minute (when the profiler will | // the initial camera view) and a minute (when the profiler will | ||||
| // have settled down if framerates as very low), to give some | // have settled down if framerates as very low), to give some | ||||
| // extremely rough indications of performance | // extremely rough indications of performance | ||||
| // | // | ||||
| Show All 19 Lines | playerData.push({ | ||||
| "name": playerState.name, | "name": playerState.name, | ||||
| "civ": playerState.civ, | "civ": playerState.civ, | ||||
| "color": { | "color": { | ||||
| "r": playerState.color.r * 255, | "r": playerState.color.r * 255, | ||||
| "g": playerState.color.g * 255, | "g": playerState.color.g * 255, | ||||
| "b": playerState.color.b * 255, | "b": playerState.color.b * 255, | ||||
| "a": playerState.color.a * 255 | "a": playerState.color.a * 255 | ||||
| }, | }, | ||||
| "team": playerState.team, | "team": playerState.team, | ||||
Done Inline ActionsCould avoid g_DiplomacyColors.push repetition with a ternaries elexis: Could avoid `g_DiplomacyColors.push` repetition with a ternaries | |||||
Not Done Inline ActionsLike this? temple: Like this? | |||||
| "teamsLocked": playerState.teamsLocked, | "teamsLocked": playerState.teamsLocked, | ||||
| "cheatsEnabled": playerState.cheatsEnabled, | "cheatsEnabled": playerState.cheatsEnabled, | ||||
| "state": playerState.state, | "state": playerState.state, | ||||
| "isAlly": playerState.isAlly, | "isAlly": playerState.isAlly, | ||||
| "isMutualAlly": playerState.isMutualAlly, | "isMutualAlly": playerState.isMutualAlly, | ||||
| "isNeutral": playerState.isNeutral, | "isNeutral": playerState.isNeutral, | ||||
| "isEnemy": playerState.isEnemy, | "isEnemy": playerState.isEnemy, | ||||
| "guid": undefined, // network guid for players controlled by hosts | "guid": undefined, // network guid for players controlled by hosts | ||||
| "offline": g_Players[i] && !!g_Players[i].offline | "offline": g_Players[i] && !!g_Players[i].offline | ||||
| }); | }); | ||||
| } | } | ||||
| for (let guid in g_PlayerAssignments) | for (let guid in g_PlayerAssignments) | ||||
| { | { | ||||
| let playerID = g_PlayerAssignments[guid].player; | let playerID = g_PlayerAssignments[guid].player; | ||||
Not Done Inline ActionsInlining data and loading the shared data in a let colors prevents spaghetti code object construction elexis: Inlining `data` and loading the shared data in a `let colors` prevents spaghetti code object… | |||||
| if (!playerData[playerID]) | if (!playerData[playerID]) | ||||
| continue; | continue; | ||||
| playerData[playerID].guid = guid; | playerData[playerID].guid = guid; | ||||
| playerData[playerID].name = g_PlayerAssignments[guid].name; | playerData[playerID].name = g_PlayerAssignments[guid].name; | ||||
Not Done Inline ActionsupdateViewedPlayerDropdown? (This way we indicate that this is a function that updates the GUI without creating or processing variables. This way we can eventually sort/split functions according to the input-processing-output model. I didn't look but the functions created here should also be put near logically similar functions for the same reason. elexis: updateViewedPlayerDropdown? (This way we indicate that this is a function that updates the GUI… | |||||
| } | } | ||||
| g_Players = playerData; | g_Players = playerData; | ||||
| } | } | ||||
| /** | /** | ||||
Done Inline Actions/** elexis: /** | |||||
| * Updates the displayed colors of players in the simulation and GUI. | |||||
| */ | |||||
| function updateDisplayedPlayerColors() | |||||
| { | |||||
| if (g_DiplomacyColorsToggle) | |||||
| { | |||||
Done Inline ActionsWhat did we say about splitting filenames when we did the minimap panel button commit? elexis: What did we say about splitting filenames when we did the minimap panel button commit? | |||||
Done Inline ActionsI forgot since we ended up doing something different. var g_IdleWorkerIcons = {
"default": "stretched:session/minimap-idle.png",
"highlight": "stretched:session/minimap-idle-highlight.png",
"disabled": "stretched:session/minimap-idle-disabled.png"
}Okay, I'll make a global variable to store them. temple: I forgot since we ended up doing something different.
```
var g_IdleWorkerIcons = {
"default"… | |||||
Done Inline ActionsThe important part is that we can do string searches for filenames, so those shouldn't be split. elexis: The important part is that we can do string searches for filenames, so those shouldn't be split. | |||||
Not Done Inline ActionsOh, right. temple: Oh, right. | |||||
Not Done Inline Actions(Imagine the guy who wants to find out where an icon is used, if it can be renamed, replaced or deleted. Also works the other way around, reading the filename first makes it easier to open the image file than piecing together the filename) (About the globals, In general splitting the code from the data is good practice (https://en.wikipedia.org/wiki/Separation_of_concerns pattern) and having them as globals means mods can change the global in a newly added file. But then again they can just replace the file and we observe the added complexity, especially when they were grouped logically, so having the png globals is not relevant here.) elexis: (Imagine the guy who wants to find out where an icon is used, if it can be renamed, replaced or… | |||||
| let teamRepresentatives = {}; | |||||
Done Inline ActionsIf it's named colors it should contain colors, not playerIDs. elexis: If it's named colors it should contain colors, not playerIDs. | |||||
| for (let i = 1; i < g_Players.length; ++i) | |||||
| if (g_ViewedPlayer <= 0) | |||||
| { | |||||
| // Observers and gaia see team colors | |||||
Done Inline Actions-() elexis: -() | |||||
| let team = g_Players[i].team; | |||||
| g_DisplayedPlayerColors[i] = g_Players[teamRepresentatives[team] || i].color; | |||||
Not Done Inline ActionsTeam colors should also be distinct from player colors IMO. The hueness could be increased with a computation too (like rP17362), but that should either be done in both cases or in both cases we should read from a file. elexis: Team colors should also be distinct from player colors IMO. The hueness could be increased with… | |||||
Not Done Inline ActionsEh, I think I'm fine with it as is. The idea is for observers to see all players on one team in the same color, but it's not really important what color that is. temple: Eh, I think I'm fine with it as is. The idea is for observers to see all players on one team in… | |||||
| if (team != -1 && !teamRepresentatives[team]) | |||||
| teamRepresentatives[team] = i; | |||||
Not Done Inline ActionsThis smells like it breaks if theres a 1v1 with teams 3 and 4. elexis: This smells like it breaks if theres a 1v1 with teams 3 and 4. | |||||
Not Done Inline ActionsHow so? They'll be seen with their original colors. temple: How so? They'll be seen with their original colors. | |||||
| } | |||||
| else | |||||
| // Players see colors depending on diplomacy | |||||
Done Inline ActionsOtherwise -> Players (requires the reader to conclude less) elexis: Otherwise -> Players (requires the reader to conclude less) | |||||
| g_DisplayedPlayerColors[i] = | |||||
| g_ViewedPlayer == i ? g_DiplomacyColorPalette.Self : | |||||
| g_Players[g_ViewedPlayer].isAlly[i] ? g_DiplomacyColorPalette.Ally : | |||||
| g_Players[g_ViewedPlayer].isNeutral[i] ? g_DiplomacyColorPalette.Neutral : | |||||
| g_DiplomacyColorPalette.Enemy; | |||||
| g_DisplayedPlayerColors[0] = g_Players[0].color; | |||||
Done Inline ActionsPerhaps its easier to pass the colors in the GUIInterface call below. elexis: Perhaps its easier to pass the colors in the GUIInterface call below. | |||||
| } | |||||
| else | |||||
| g_DisplayedPlayerColors = g_Players.map(player => player.color); | |||||
Done Inline Actionsx -> player elexis: x -> player | |||||
| Engine.GuiInterfaceCall("UpdateDisplayedPlayerColors", { | |||||
| "displayedPlayerColors": g_DisplayedPlayerColors, | |||||
| "displayDiplomacyColors": g_DiplomacyColorsToggle, | |||||
| "showAllStatusBars": g_ShowAllStatusBars, | |||||
| "selected": g_Selection.toList() | |||||
| }); | |||||
Not Done Inline ActionsWondering if we should move it to updateGUIObjects elexis: Wondering if we should move it to `updateGUIObjects` | |||||
Not Done Inline ActionsThen it would be called every simulation update, which seems bad for performance. temple: Then it would be called every simulation update, which seems bad for performance. | |||||
Not Done Inline ActionsupdateViewedPlayerDropdown, not the GUIInterface call elexis: updateViewedPlayerDropdown, not the GUIInterface call | |||||
Not Done Inline ActionsThat's what I meant, and maybe it doesn't matter for performance that much, but it seems weird to keep recreating the dropdown list when it's not changing. temple: That's what I meant, and maybe it doesn't matter for performance that much, but it seems weird… | |||||
Not Done Inline ActionsMostly saying from D322. The entire gamesetup is rebuilt very often, whereas before it had this kind of performance optimization. The cost of that however were a lot of complexity, as the developer always had to know exactly the order of updates and when which update was necessary and when not. Thats both complicated and complex when you're dealing with 30 to 50 GUI objects. Just updating everything all the time means one doesn't have to think anymore and in the gamesetup the performance doesn't matter. In the session things are not as unified however and performance a bit more relevant. Still better to make things as easy as possible when we don't buy a performance bottlneck. elexis: Mostly saying from D322. The entire gamesetup is rebuilt very often, whereas before it had this… | |||||
| updateGUIObjects(); | |||||
Not Done Inline ActionsWhen someone reads updateDisplayedPlayerColors he might expect that only this part is updated, not the entire GUI. Seems better practice to have one function that calls all individual updates than having one special case call another special case calling the umbrella function. I guess it's the https://en.wikipedia.org/wiki/Inversion_of_control pattern. The session init is also a bit messy IMO. But there's the performance factor and it seems better to have this discussion in a separate session GUI update refactoring patch. Not having the last two statements in this function but after each caller would mean that the init function doesn't have to call updateGUIObjects and updateViewedPlayerDropdown twice, dunno. (Either way it should become more simple while adding features.) elexis: When someone reads `updateDisplayedPlayerColors` he might expect that only this part is updated… | |||||
Not Done Inline ActionsThis is here because otherwise the GUI colors wouldn't be updated when the game is paused. temple: This is here because otherwise the GUI colors wouldn't be updated when the game is paused. | |||||
Not Done Inline ActionsI didn't mean that the call should be removed, but can be moved. Anyway, rearrangement can be done separately. elexis: I didn't mean that the call should be removed, but can be moved. Anyway, rearrangement can be… | |||||
| } | |||||
| /** | |||||
| * Depends on the current player (g_IsObserver). | * Depends on the current player (g_IsObserver). | ||||
| */ | */ | ||||
| function updateHotkeyTooltips() | function updateHotkeyTooltips() | ||||
| { | { | ||||
| Engine.GetGUIObjectByName("chatInput").tooltip = | Engine.GetGUIObjectByName("chatInput").tooltip = | ||||
| translateWithContext("chat input", "Type the message to send.") + "\n" + | translateWithContext("chat input", "Type the message to send.") + "\n" + | ||||
| colorizeAutocompleteHotkey() + | colorizeAutocompleteHotkey() + | ||||
| colorizeHotkey("\n" + translate("Press %(hotkey)s to open the public chat."), "chat") + | colorizeHotkey("\n" + translate("Press %(hotkey)s to open the public chat."), "chat") + | ||||
| ▲ Show 20 Lines • Show All 102 Lines • ▼ Show 20 Lines | function selectViewPlayer(playerID) | ||||
| if (g_DevSettings.changePerspective) | if (g_DevSettings.changePerspective) | ||||
| { | { | ||||
| Engine.SetPlayerID(g_ViewedPlayer); | Engine.SetPlayerID(g_ViewedPlayer); | ||||
| g_IsObserver = isPlayerObserver(g_ViewedPlayer); | g_IsObserver = isPlayerObserver(g_ViewedPlayer); | ||||
| } | } | ||||
| Engine.SetViewedPlayer(g_ViewedPlayer); | Engine.SetViewedPlayer(g_ViewedPlayer); | ||||
| updateDisplayedPlayerColors(); | |||||
| updateTopPanel(); | updateTopPanel(); | ||||
| updateChatAddressees(); | updateChatAddressees(); | ||||
| updateHotkeyTooltips(); | updateHotkeyTooltips(); | ||||
| updateGameSpeedControl(); | updateGameSpeedControl(); | ||||
Not Done Inline Actions(This is what I mean with complexity that had made the gamesetup terrible and could be simplified to one updateGUIObjects that updates the entire GUI state from the given state variables. (input-processing-output model).) elexis: (This is what I mean with complexity that had made the gamesetup terrible and could be… | |||||
| // Update GUI and clear player-dependent cache | // Update GUI and clear player-dependent cache | ||||
| g_TemplateData = {}; | g_TemplateData = {}; | ||||
| Engine.GuiInterfaceCall("ResetTemplateModified"); | Engine.GuiInterfaceCall("ResetTemplateModified"); | ||||
| onSimulationUpdate(); | onSimulationUpdate(); | ||||
| if (g_IsDiplomacyOpen) | if (g_IsDiplomacyOpen) | ||||
| openDiplomacy(); | openDiplomacy(); | ||||
| ▲ Show 20 Lines • Show All 330 Lines • ▼ Show 20 Lines | function confirmExit() | ||||
| if (g_IsNetworked && !g_IsNetworkedActive) | if (g_IsNetworked && !g_IsNetworkedActive) | ||||
| return; | return; | ||||
| closeOpenDialogs(); | closeOpenDialogs(); | ||||
| // Don't ask for exit if other humans are still playing | // Don't ask for exit if other humans are still playing | ||||
| let isHost = g_IsController && g_IsNetworked; | let isHost = g_IsController && g_IsNetworked; | ||||
| let askExit = !isHost || isHost && g_Players.every((player, i) => | let askExit = !isHost || isHost && g_Players.every((player, i) => | ||||
| i == 0 || | i == 0 || | ||||
Done Inline ActionsIf some of that can be committed separately, I'd commit that. elexis: If some of that can be committed separately, I'd commit that. | |||||
| player.state != "active" || | player.state != "active" || | ||||
| g_GameAttributes.settings.PlayerData[i].AI != ""); | g_GameAttributes.settings.PlayerData[i].AI != ""); | ||||
| let subject = g_PlayerStateMessages[g_ConfirmExit]; | let subject = g_PlayerStateMessages[g_ConfirmExit]; | ||||
| if (askExit) | if (askExit) | ||||
| subject += "\n" + translate("Do you want to quit?"); | subject += "\n" + translate("Do you want to quit?"); | ||||
| messageBox( | messageBox( | ||||
| ▲ Show 20 Lines • Show All 55 Lines • ▼ Show 20 Lines | function updateGUIObjects() | ||||
| if (!g_IsObserver) | if (!g_IsObserver) | ||||
| { | { | ||||
| // Update music state on basis of battle state. | // Update music state on basis of battle state. | ||||
| let battleState = Engine.GuiInterfaceCall("GetBattleState", g_ViewedPlayer); | let battleState = Engine.GuiInterfaceCall("GetBattleState", g_ViewedPlayer); | ||||
| if (battleState) | if (battleState) | ||||
| global.music.setState(global.music.states[battleState]); | global.music.setState(global.music.states[battleState]); | ||||
| } | } | ||||
| updateViewedPlayerDropdown(); | |||||
| updateDiplomacy(); | updateDiplomacy(); | ||||
| } | } | ||||
| function onReplayFinished() | function onReplayFinished() | ||||
| { | { | ||||
| closeOpenDialogs(); | closeOpenDialogs(); | ||||
| pauseGame(); | pauseGame(); | ||||
| ▲ Show 20 Lines • Show All 748 Lines • Show Last 20 Lines | |||||
(since it's convention to have operators at the end of the previous line rather than at the start of the next line, I was doing the same with code comments and their conjunction and. After all we put the comma there for the same reason)