Page MenuHomeWildfire Games

Check for and save mod version for savegames and replays
ClosedPublic

Authored by Imarok on Oct 9 2017, 8:34 PM.

Details

Summary

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)

Test Plan

Test with different versions of some mods but the same engine version.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Style fixes and translation comments

Executing section Default...
Executing section Source...
Executing section JS...

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...
elexis added inline comments.Dec 28 2017, 5:53 PM
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.

Imarok marked 7 inline comments as done.Jan 9 2018, 11:05 PM
Imarok added inline comments.
binaries/data/mods/mod/gui/common/mod.js
9

Sorry, what do you mean?

source/ps/Mod.h
25

and in SetMods.
This variable is definitely needed, as it defines witch mods are loaded.

31

Enabled != loaded

Imarok updated this revision to Diff 5201.Jan 9 2018, 11:10 PM
Imarok marked 2 inline comments as done.

Fix some things

Executing section Default...
Executing section Source...
Executing section JS...

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...
elexis requested changes to this revision.Jan 22 2018, 3:55 PM

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
24

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.
Also that occulted comma should be translated.

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

^

This revision now requires changes to proceed.Jan 22 2018, 3:55 PM
Imarok updated this revision to Diff 5421.Jan 22 2018, 4:59 PM
Imarok marked 3 inline comments as done.

Fixed.

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.

In D955#50473, @elexis wrote:

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

Also we should split the public mod into a 0ad mod and a pyrogenesis mod, can you add it to this patch?

Sure xD

Imarok updated this revision to Diff 5451.Jan 23 2018, 7:00 PM

Fix the oversight in replay_actions

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.
Secondly SavedGames::GetEngineInfo should be moved to JSInterface_Mods.cpp because it's used for the replay menu and lobby too, not only savegames. Can be done separately.

29

Likely already a tumult on transifex about that comma. Someone should check

35

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? :-)

41

); after the }

42

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

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

Should use the same phrasing as the other loadgame / lobby

93

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?
In that case both are enabled, because if it were not enabled it were an unenabled mod and an unenabled mod is synonymous with a disabled mod and disabled mods can't be loaded by definition. Would be good to use precise language, but I'm not having any ideas currently.
Notice in alpha 22 we also have the "Enable" button that doesn't write the state to the user config.
But as long as we have g_modsLoaded, the current name doesnt make things worse.

Imarok marked 13 inline comments as done.Jan 25 2018, 11:52 PM
Imarok added inline comments.
binaries/data/mods/mod/gui/common/mod.js
35

I guess he'll see, that the only difference between the required and active strings is the order...

binaries/data/mods/public/gui/loadgame/load.js
146

Makes sense when both occurs (wrong engine version and wrong mods.

Imarok updated this revision to Diff 5499.Jan 25 2018, 11:52 PM
Imarok marked an inline comment as done.

Some fixes

elexis edited reviewers, added: leper; removed: elexis.Feb 8 2018, 8:00 PM
elexis added inline comments.
binaries/data/mods/public/gui/loadgame/load.js
146

Still don't see the sense as all three following strings imply the string above.

Imarok updated this revision to Diff 5722.Feb 8 2018, 8:15 PM

Remove the one string.

Vulcan added a comment.Feb 8 2018, 9:38 PM

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...
Vulcan added a comment.Feb 8 2018, 9:40 PM
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.
elexis accepted this revision.Feb 17 2018, 11:37 AM
elexis added a subscriber: elexis.

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?
We are happily throwing errors if they contain some other broken data, like the missing AIBehavior property will bug upon selection of a22 savegames in a23.

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

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?
XmppClient::GuiPollHistoricMessages is specifically easy.

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.
IMO can do this without review afterwards.

This revision is now accepted and ready to land.Feb 17 2018, 11:37 AM
Imarok marked 4 inline comments as done.Feb 17 2018, 3:45 PM
Imarok added inline comments.
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

guess the replay and savegame engine check can be abstracted too eventually

Probably, but not in this diff I'd say.

Imarok marked 3 inline comments as done.Feb 17 2018, 4:11 PM
Imarok added inline comments.
source/ps/Replay.cpp
58

I think it is needed for L65

source/ps/SavedGame.cpp
291

It also contains the engine_version, but I still agree.

This revision was automatically updated to reflect the committed changes.
Imarok marked an inline comment as done.
Imarok marked 2 inline comments as done.Feb 17 2018, 6:42 PM
Imarok added inline comments.
source/ps/SavedGame.cpp
291
This comment was removed by Imarok.