Remove savegame hack from rP13579
ClosedPublic
Actions

Authored by elexis on Sep 17 2019, 9:58 PM.

Details

Reviewers: None
Commits: rP22922: Happy savegame hack removal following rP13579, refs #2030.
Trac Tickets: #2030

Summary

In save.js there is a comment about a function being a hack.
And indeed a workaround it is, because the GUI can just pass the JS Value storing the GUI data to be added to the savegame when calling the JS function,
rather than the savegame code calling the topmost GUI page to obtain that.

It seems like the save.js function was not called since rP14496 anymore, but not sure how JS worked back then.

Test Plan

Make sure that the GUI data is still stored for autoloading by having unit controlgroups (ctrl+1 to assign etc.) and that those are loaded correctly, for both quicksave and regular save.
Ensure that the PersistentRootedValue is correctly rooted.
Be glad about the GUI manager having that code removed.
Notice that quickload in mp mode is not implemented, currently ends the game with a UI error.

Diff Detail

Repository

rP 0 A.D. Public Repository

Lint

Automatic diff as part of commit; lint not applicable.

Unit

Automatic diff as part of commit; unit tests not applicable.

Event Timeline

elexis created this revision.Sep 17 2019, 9:58 PM

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/225/display/redirect

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...

source/gui/GUIManager.h
|  46| class·CGUIManager
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCGUIManager{' is invalid C code. Use --std or --language to configure the language.

source/simulation2/system/TurnManager.h
|  57| class·CTurnManager
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCTurnManager{' is invalid C code. Use --std or --language to configure the language.

source/ps/scripting/JSInterface_SavedGame.h
|  23| namespace·JSI_SavedGame
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'namespaceJSI_SavedGame{' is invalid C code. Use --std or --language to configure the language.
Executing section JS...
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/736/display/redirect

Harbormaster completed remote builds in B9435: Diff 9825.Sep 17 2019, 10:08 PM

elexis added inline comments.Sep 18 2019, 1:51 AM

source/simulation2/system/TurnManager.cpp
47 ↗	(On Diff #9825)	Current `~CGame()` deletes TurnManager prior to Simulation2, so the context isnt nuked while this is still persistently rooted.
306 ↗	(On Diff #9825)	Needs a structured clone for quicksave (because it stores the reference to the value), but not for regular save (where it is serialized to json)
311 ↗	(On Diff #9825)	Thought about passing the ScriptInterface from the JSInterface here, but seems like no gain since we already rely on the sim2 SI and still need to test for `g_GUI`.

elexis edited the test plan for this revision. (Show Details)Sep 18 2019, 1:54 AM

elexis added inline comments.Sep 18 2019, 2:10 AM

source/simulation2/system/TurnManager.cpp
306 ↗	(On Diff #9825)	A deepfreeze would also prevent a case where the JS functions modifies the JS object. For example if the JS function reading this JS object would modify the object (thinking that it receives a copy), it would change the quicksave state. Then when quickloading a second time without saving, it will load the modified data, not the data at the time the quicksave was stored. Freezing prevents that. (Tested: Yes)