[PATCH] Save replay metadata on Alt+F4

[elexis]

Currently the metadata.json file, which contains the summary screen data is not saved if the game is quit using Alt+F4.

However, if it's not a forced shutdown, it should be possible to detect that using SDL_QuitRequested() (f.e. r17852) somewhere and quickly write the file before destroying g_Game and the other components.

[Imarok]

Should probably done in EndGame() (GameSetup.cpp) or maybe in the destructor of CReplayLogger(Replay.cpp)

[Imarok]

fixed with the help of elexis

[elexis]

• Thanks for the patch. It works essentially as advertized. Some code style issues and a missing page check should be addressed before the commit.
• Often the LOGERROR("Could not save replay metadata"); is triggered. This is because there might have been other pages opened. For example a message box (player has lost), the summary screen or civ info page. functions_global_object.js is included in all gui pages.
• Non-minimalistic (ugly) call stack / code flow / labyrinth:

  	EndGame()
  	g_Game->SaveReplayMetadata
  	m_ReplayLogger->SaveReplayMetadata
  	scriptInterface->CallFunction(global, "saveReplayMetadata"
  	Engine.SaveReplayMetadata
  	JSI_VisualReplay::SaveReplayMetadata
  	VisualReplay::SaveReplayMetadata
  

  instead it should be:

  	EndGame
  	VisualReplay::SaveReplayMetadata
  	scriptInterface->CallFunction(global, "getReplayMetadata"
  

  All methods and functions present in (1) but not (2) should be removed. Thus avoid the noisy, pointless proxies in Game.cpp/.h (the code doesn't use anything from Game) and the replay logger won't be cluttered with unrelated metadata managing (as for now it was only used to write the commands.txt). It should be GetReplayMetadata in session.js returning an object instead of calling yet another unneeded proxy.
• Don't upload patches with compile warnings: the arguments of the new dummy logger method would have to become UNUSED
• Since VisualReplay::SaveReplayMetadata is the function that should contain the new logic, there is no reason why this shouldn't be fixed when changing the affected lines:

  	// TODO: enhancement: use JS::HandleValue similar to SaveGame
  

i.e. use the StringifyJSON in c++, similar to the CGUIManager::GetSavedGameData() approach.

• Nuke this TODO you're fixing:

  // TODO: enhancement: how to save the data if the process is killed? (case SDL_QUIT in main.cpp)
  

• Add a comment to the new session.js / functions_global_object.js function that it's called from EndGame()
• Developer eye candy: Those debug_printf calls when writing a screenshot, saving a game or writing a replay have been very useful for debugging and managing these files to me. Should we have one for the metadata too?

whitespace:

• ScriptInterface *scriptInterface should be ScriptInterface* scriptInterface, see 4th paragraph of wiki:Coding_Conventions#Misc
• missing spaces at {} of CDummyReplayLogger
• missing newline in Game.cpp

[Imarok]

Made all changes proposed in the ticket above

[elexis]

• Error prone duplication of the sim-data object. Anyone updating that will likely forget to update the globalscripts one. Better call the globalfunction instead. The session function won't need the extended simstate anymore and can use g_Players, or even isPlayerObserver instead.
• Missing c_str() in debug_printf("Saved replay metadata to %s\n", fileName.string8());
• Unneeded variable ok
• Perhaps VisualReplay can access g_GUI directly instead of passing those 2 arguments? Perhaps only passing the scriptinterface and using GetGlobalObject on that?

[Imarok]

Applied elexis' remarks

[elexis]

• Unneeded variable simData
• Isn't isPlayerObserver easier to read? The simstate is cached anyway.
• Ambiguous name simData, GetReplayMetadata was fine. (GetSummaryScreen would have been a worse alternative as the function might return some data used in the replay menu but not in the summary screen.)

Looks nice otherwise :)

• Does EndGame() break in non-visual replay mode? Doesn't look like it actually. If the saving would be done in the destructor of Game, it might have though (since the GUI context wouldn't exist probably).

[Imarok]

again

[elexis]

In 18613:

Always save the replay metadata (summary screen info) when ending the application. Patch by Imarok, fixes #4020.

[elexis]

Thanks, for the great patch!

The code looks much better now and the metadata is always saved when ending the game (SIGTERM), unless sending a KILL signal.

Had to add and g_AtlasGameLoop and IsGameStarted to avoid writing the metadata when it's not wanted or unfeasible. Also removed a duplicate g_Game check and changed 3 individual character for styling reasons.

[elexis]

In 22991:

Save replay metadata for non-visual games too, refs #4577, #5548, fixes #5565.

Remove gui/common/ hack by going through the GUIInterface and fragile EndGame() hardcoding by saving if and only if a ReplayLogger exists following rP18613, refs #4020.
Refs D2211, D2213.

Differential Revision: ​https://code.wildfiregames.com/D2197
Based on patch by: irishninja
Tested on: clang 8.0.1, Jenkins