Saves the mod versions in savegames and replays.
Checks for the right version when loading and replaying.
Currently does some ugly thing to go around scriptinterface constness. (I'm open to suggestions)
Details
- Reviewers
elexis - Commits
- rP21239: Check for and save mod version for savegames and replays
- Trac Tickets
- #4887
Test with different versions of some mods but the same engine version.
Diff Detail
- Repository
- rP 0 A.D. Public Repository
- Branch
- /ps/trunk
- Lint
Lint OK - Unit
No Unit Test Coverage - Build Status
Buildable 4407 Build 7733: Vulcan Build (Windows) Jenkins Build 7732: Vulcan Build Jenkins Build 7731: arc lint + arc unit
Event Timeline
Successful build - Chance fights ever on the side of the prudent.
Updating workspaces... Build (release)... Build (debug)... Running release tests... Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK! Running debug tests... Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK! Checking XML files...
binaries/data/mods/mod/gui/common/mod.js | ||
---|---|---|
9 | I see, one would have to change SavedGames::GetEngineInfo. Seems reasonable to me actually. The savegame version shouldn't exist, the mod version should come from JSInterface_Mods.cpp not SavedGame, and the engine version should come from Main or something, not SavedGame since its also going to be reused from things other than savedgames. Maybe it doesn't improve too much to nuke that, dunno. | |
22 | probably yes. Unfortunately there is more symmetry in the logic (should have the same properties in the same order) than reflected by the code (the first object has a different role than the second obejct since it's the subject of the array function). But I don't see a nicer way to code it sadly. | |
35 | Then your code should use === instead of == so that we don't require the reader to know the subtle details of the ES Abstract Equality Comparison Algorithm? | |
39 | ) + "\n" + ` the line above. I believe JSHint was the one complaining about operators not being at the end of a line (http://jshint.com/docs/options/#laxbreak) | |
42 | Ah, reading can perform wonders for those exercising it. | |
source/ps/Mod.cpp | ||
111 | remove commented code | |
source/ps/Mod.h | ||
25 | Looks like its only used in Gamesetup.cpp, perhaps GetMods in Gamesetup.cpp can not use this vector and the global can either be deleted or if it's considered useful (probably is), then it's content could be GetLoadedModsWithVersions. Maybe. | |
31 | Should ideally use the same phrasing globally. The mod selection page uses enabled. Somewhere else I read "launched mods". Dunno what's most common. But then again we have g_modsLoaded so I guess its ok. |
Successful build - Chance fights ever on the side of the prudent.
Updating workspaces... Build (release)... Build (debug)... Running release tests... Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK! Running debug tests... Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK! Checking XML files...
The mod mod should be filtered too when displaying mod versions in the savegame / lobby, because noone except devs knows what that is and why it doesn't have a version.
({engine_version:"0.0.23", mods:[["mod", ""], ["public", "0.0.23"]]})
binaries/data/mods/mod/gui/common/mod.js | ||
---|---|---|
23 | comment seems pointless since it doesnt explain whats in it. The function is obvious enough IMO | |
binaries/data/mods/public/gui/loadgame/load.js | ||
117 | This doesn't work properly anymore since there is a space missing after the comma that separates the mod version from the mod name. Even better would be more than only translating the comma between the mod name and mod version, maybe "Mod Name (Mod Version)".join(", "), or even join with newline. Also we should split the public mod into a 0ad mod and a pyrogenesis mod, can you add it to this patch? | |
source/ps/Replay.cpp | ||
1 | ^ |
translate("You don't have the same mods active as the replay does.") -> -does
That has the same issue with regards to mod name + version string concatentation and "mod" filtering.
I don't get your point.
binaries/data/mods/public/gui/loadgame/load.js | ||
---|---|---|
117 |
Sure xD |
Seems to work, but I wouldn't reject some string and naming improvements.
Good that it only excludes the mod mod for displaying.
Eventually (ticket?) the load dialog should be rearranged to first display the mods, then the game details. Not some game details, then the mods, then the other game details. Also the GUI space likely won't be sufficient for3 mods enabled.
Same goes for the replay menu (ticket!) - there should be a list of mods shown, at least if it's different from the current sequence of mods.
The mods must also be displayed in the lobby, but theres a patch for that.
binaries/data/mods/mod/gui/common/mod.js | ||
---|---|---|
9 | D1131 for the first part. | |
30 | Likely already a tumult on transifex about that comma. Someone should check | |
36 | I assume there will be the one guy who has all the right mods but the wrong order and doesn't comprehend what's going on. Maybe we should write a ticket to catch that case. | |
39 | Now what's the difference between Active, Loaded and Enabled? :-) | |
42 | ); after the } | |
43 | Don't forget eol-style native and to end the file with exactly one newline | |
binaries/data/mods/public/gui/loadgame/load.js | ||
146 | There's at least a newline missing here, but the string seems pointless / redundant with the stings following that. | |
162–163 | since the order matters, set is wrong. Sequence? | |
163 | Seems more readable with a blank line before and after the mod list, but I suspect the message box doesn't autoresize. (Also the left-alignment would fit better for the modlist) | |
binaries/data/mods/public/gui/replaymenu/replay_actions.js | ||
92 ↗ | (On Diff #5451) | Should use the same phrasing as the other loadgame / lobby |
93 ↗ | (On Diff #5451) | Can become one statement on two lines |
source/ps/Mod.h | ||
31 | You mean the difference between enabled and saved to the config and enabled while not saved to the config? |
binaries/data/mods/public/gui/loadgame/load.js | ||
---|---|---|
146 | Still don't see the sense as all three following strings imply the string above. |
Successful build - Chance fights ever on the side of the prudent.
Updating workspaces... Build (release)... Build (debug)... Running release tests... Running cxxtest tests (309 tests).....................................................................................................................................................................................................................................................................................................................OK! Running debug tests... Running cxxtest tests (309 tests).....................................................................................................................................................................................................................................................................................................................OK! Checking XML files...
Executing section Default... Executing section Source... Executing section JS... | | [NORMAL] ESLintBear (space-in-parens): | | There should be no spaces inside this paren. |----| | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/mod/gui/common/mod.js | |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/mod/gui/common/mod.js | 22| 22| */ | 23| 23| function modsToString(mods) | 24| 24| { | 25| |- return mods.filter(mod => mod[0] != "mod" && mod[0] != "user" ).map(mod => | | 25|+ return mods.filter(mod => mod[0] != "mod" && mod[0] != "user").map(mod => | 26| 26| sprintf(translateWithContext("Mod comparison", "%(mod)s (%(version)s)"), { | 27| 27| "mod": mod[0], | 28| 28| "version": mod[1] | | [NORMAL] ESLintBear (indent): | | Expected indentation of 2 tabs but found 3. |----| | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/mod/gui/common/mod.js | |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/mod/gui/common/mod.js | 35| 35| function comparedModsString(required, active) | 36| 36| { | 37| 37| return sprintf(translateWithContext("Mod comparison", "Required: %(mods)s"), | 38| |- { "mods": modsToString(required) }) + "\n" + | | 38|+ { "mods": modsToString(required) }) + "\n" + | 39| 39| sprintf(translateWithContext("Mod comparison", "Active: %(mods)s"), | 40| 40| { "mods": modsToString(active) }); | 41| 41| } binaries/data/mods/public/gui/loadgame/load.js | 34| » » switch·(sortKey) | | [NORMAL] ESLintBear (default-case): | | Expected a default case.
Ok if the reference error is fixed, if it uses JS_NewArrayObject and if the user and mod mod are filtered in GetLoadedModsWithVersions or if you give me a good reason why these two should be in there.
After several attempts of testing I noticed that this patch doesn't implement the lobby part.
It works as advertized with the Savegame and Replay feature.
The replay menu only has an "Ok" button, but the savegame menu has a Yes/No choice allowing to load a game with wrong mods.
This should be corrected in the replay menu. Can be done separately.
binaries/data/mods/mod/gui/common/mod.js | ||
---|---|---|
2 | I don't know if the author of this page endorses this file, but there was sufficient possibility to give feedback. | |
18 | Reading only this function, I still think it could become ["modname", "modversion"].every(name, i => ). But since the next function can't do that too, it's ok. | |
26 | 1 excess space | |
binaries/data/mods/public/gui/loadgame/load.js | ||
145–146 | It wouldn't hurt to define the variable we reference below. | |
155 | I wonder if we shouldn't kill this string now. Do we care about people with that old savegame versions? | |
166 | The question is ok if there are different mods, since some of them might not change the simstate, but for the engine version we might not ask this. Anyway out of scope. | |
binaries/data/mods/public/gui/replaymenu/replay_actions.js | ||
99 ↗ | (On Diff #5722) | guess the replay and savegame engine check can be abstracted too eventually |
source/ps/Mod.cpp | ||
111 | Those Evals were ugly. What happened to JS_NewArrayObject? | |
113 | Wondering if we shouldn't ignore the user-mod here. Useless bytes saved to the disk and sent across the network. | |
source/ps/Replay.cpp | ||
58 | AutoRequest probably not needed, but should be kept for consistency and good practice. | |
65 | Calling a function seems nicer than a global indeed. If something needs that very frequently, it can cache that on it's own, or we should have a getter for the cache. | |
source/ps/SavedGame.cpp | ||
291 | This function should be moved to Mods since SaveGames were only the original of multiple use cases. |
binaries/data/mods/public/gui/loadgame/load.js | ||
---|---|---|
145–146 | What do you mean? | |
145–146 | I think I got it .\ | |
155 | I think explicitly warning the cause shouldn't hurt. | |
binaries/data/mods/public/gui/replaymenu/replay_actions.js | ||
99 ↗ | (On Diff #5722) |
Probably, but not in this diff I'd say. |