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.setAllied(this.getMapSetting("AllyView")); | this.setAllied(this.getMapSetting("AllyView")); | |||||||||
| this.setRevealed(this.getMapSetting("RevealMap")); | ||||||||||
vladislavbelovUnsubmitted Not Done Inline ActionsWhy the order does matter? vladislavbelov: Why the order does matter? | ||||||||||
JammyjamjammanAuthorUnsubmitted 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… | ||||||||||
vladislavbelovUnsubmitted 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) | |||||||||
| { | { | |||||||||
| this.explored = enabled; | this.explored = enabled; | |||||||||
Done Inline Actions
same in other cases Silier: same in other cases | ||||||||||
| this.revealed = this.revealed && this.explored; | this.revealed = this.revealed && this.explored; | |||||||||
| } | } | |||||||||
Done Inline ActionsI would use early return, but looks ok Silier: I would use early return, but looks ok | ||||||||||
Done Inline ActionsAlso, probably comments not needed Silier: Also, probably comments not needed | ||||||||||
| setRevealed(enabled) | setRevealed(enabled) | |||||||||
| { | { | |||||||||
| 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) | |||||||||
| { | { | |||||||||
| this.allied = enabled; | this.allied = enabled; | |||||||||
| this.revealed = this.revealed && this.allied; | this.revealed = this.revealed && this.allied; | |||||||||
| } | } | |||||||||
| }; | }; | |||||||||
no need for empty line