Changeset View
Standalone View
binaries/data/mods/public/gui/gamesettings/attributes/MapExploration.js
GameSettings.prototype.Attributes.MapExploration = class MapExploration extends GameSetting | GameSettings.prototype.Attributes.MapExploration = class MapExploration extends GameSetting | |||||||||
{ | { | |||||||||
init() | init() | |||||||||
{ | { | |||||||||
this.explored = false; | this.explored = false; | |||||||||
this.revealed = false; | this.revealed = false; | |||||||||
Silier: no need for empty line | ||||||||||
this.allied = false; | this.allied = false; | |||||||||
this.settings.map.watch(() => this.onMapChange(), ["map"]); | this.settings.map.watch(() => this.onMapChange(), ["map"]); | |||||||||
} | } | |||||||||
toInitAttributes(attribs) | toInitAttributes(attribs) | |||||||||
{ | { | |||||||||
attribs.settings.RevealMap = this.revealed; | attribs.settings.RevealMap = this.revealed; | |||||||||
attribs.settings.ExploreMap = this.explored; | attribs.settings.ExploreMap = this.explored; | |||||||||
attribs.settings.AllyView = this.allied; | attribs.settings.AllyView = this.allied; | |||||||||
} | } | |||||||||
fromInitAttributes(attribs) | fromInitAttributes(attribs) | |||||||||
{ | { | |||||||||
this.explored = !!this.getLegacySetting(attribs, "ExploreMap"); | this.explored = !!this.getLegacySetting(attribs, "ExploreMap"); | |||||||||
this.revealed = !!this.getLegacySetting(attribs, "RevealMap"); | this.revealed = !!this.getLegacySetting(attribs, "RevealMap"); | |||||||||
this.allied = !!this.getLegacySetting(attribs, "AllyView"); | this.allied = !!this.getLegacySetting(attribs, "AllyView"); | |||||||||
} | } | |||||||||
onMapChange(mapData) | onMapChange(mapData) | |||||||||
{ | { | |||||||||
if (this.settings.map.type != "scenario") | if (this.settings.map.type != "scenario") | |||||||||
Done Inline ActionsSo this is sus, Silier: So this is sus,
but probably out of scope | ||||||||||
return; | return; | |||||||||
this.setExplored(this.getMapSetting("ExploreMap")); | this.setExplored(this.getMapSetting("ExploreMap")); | |||||||||
this.setRevealed(this.getMapSetting("RevealMap")); | this.setRevealed(this.getMapSetting("RevealMap")); | |||||||||
this.setAllied(this.getMapSetting("AllyView")); | this.setAllied(this.getMapSetting("AllyView")); | |||||||||
} | } | |||||||||
Not Done Inline ActionsWhy the order does matter? vladislavbelov: Why the order does matter? | ||||||||||
Done Inline ActionsIf a map e.g. only defines RevealMap as true, then if you call setRevealed(), the values explored, alliedView and revealed all get set to true. But ExploreMap and AllyView are undefined in the map js file. There's no explicit logic for dealing with undefined, so they'll get coalesced to false. Therefore, if you call setExplored() and/ or setAllied() after setRevealed(), explored/ alliedView will get set to false and revealed will get set back to false again. It may be better if we handled the undefined/ null case explicitly. Then order wouldn't matter. There's also questions about handling cases where a map developer uses conflicting settings e.g. RevealMap = true and ExploreMap = false. Currently, RevealMap overrides. (Because its setter is called last.) Jammyjamjamman: If a map e.g. only defines RevealMap as true, then if you call setRevealed(), the values… | ||||||||||
Not Done Inline ActionsThe reason should be mentioned at least in a comment. Having the implicit relation isn't a good thing. Also maybe there should be some checks/tests for that cases. vladislavbelov: The reason should be mentioned at least in a comment. Having the implicit relation isn't a good… | ||||||||||
setExplored(enabled) | setExplored(enabled) | |||||||||
{ | { | |||||||||
// Check if enabled is defined in map settings - don't change any values if it isn't. | ||||||||||
Done Inline Actions
same in other cases Silier: same in other cases | ||||||||||
if(enabled !== undefined) | ||||||||||
SilierUnsubmitted Done Inline ActionsI would use early return, but looks ok Silier: I would use early return, but looks ok | ||||||||||
SilierUnsubmitted Done Inline ActionsAlso, probably comments not needed Silier: Also, probably comments not needed | ||||||||||
{ | ||||||||||
this.explored = enabled; | this.explored = enabled; | |||||||||
this.revealed = this.revealed && this.explored; | this.revealed = this.revealed && this.explored; | |||||||||
} | } | |||||||||
} | ||||||||||
setRevealed(enabled) | setRevealed(enabled) | |||||||||
{ | { | |||||||||
// Check if enabled is defined in map settings - don't change any values if it isn't. | ||||||||||
if(enabled !== undefined) | ||||||||||
{ | ||||||||||
this.explored = this.explored || enabled; | this.explored = this.explored || enabled; | |||||||||
this.revealed = enabled; | this.revealed = enabled; | |||||||||
this.allied = this.allied || enabled; | this.allied = this.allied || enabled; | |||||||||
} | } | |||||||||
} | ||||||||||
setAllied(enabled) | setAllied(enabled) | |||||||||
{ | { | |||||||||
// Check if enabled is defined in map settings - don't change any values if it isn't. | ||||||||||
if(enabled !== undefined) | ||||||||||
{ | ||||||||||
this.allied = enabled; | this.allied = enabled; | |||||||||
this.revealed = this.revealed && this.allied; | this.revealed = this.revealed && this.allied; | |||||||||
} | } | |||||||||
} | ||||||||||
}; | }; |
no need for empty line