Opened 7 years ago

Closed 7 years ago

Last modified 4 years ago

#4020 closed enhancement (fixed)

[PATCH] Save replay metadata on Alt+F4

Reported by: elexis Owned by: elexis
Priority: Should Have Milestone: Alpha 21
Component: Core engine Keywords: patch
Cc: Patch:


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.

Attachments (4)

4020_altf4_v1.patch (5.5 KB ) - added by Imarok 7 years ago.
fixed with the help of elexis
4020_altf4_v2.patch (7.5 KB ) - added by Imarok 7 years ago.
Made all changes proposed in the ticket above
4020_altf4_v2.1.patch (7.2 KB ) - added by Imarok 7 years ago.
Applied elexis' remarks
4020_altf4_v2.2.patch (7.1 KB ) - added by Imarok 7 years ago.

Download all attachments as: .zip

Change History (14)

comment:1 by Imarok, 7 years ago

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

Last edited 7 years ago by Imarok (previous) (diff)

by Imarok, 7 years ago

Attachment: 4020_altf4_v1.patch added

fixed with the help of elexis

comment:2 by Imarok, 7 years ago

Keywords: rfc patch added
Milestone: BacklogAlpha 21
Summary: Save replay metadata on Alt+F4[PATCH] Save replay metadata on Alt+F4

comment:3 by elexis, 7 years ago

Component: UI & SimulationCore engine
Keywords: rfc removed
Type: defectenhancement
  • 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:
    	scriptInterface->CallFunction(global, "saveReplayMetadata"
    instead it should be:
    	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?


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

comment:4 by elexis, 7 years ago

Priority: Nice to HaveShould Have

by Imarok, 7 years ago

Attachment: 4020_altf4_v2.patch added

Made all changes proposed in the ticket above

comment:5 by Imarok, 7 years ago

Keywords: rfc added

comment:6 by elexis, 7 years ago

  • 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?

by Imarok, 7 years ago

Attachment: 4020_altf4_v2.1.patch added

Applied elexis' remarks

comment:7 by elexis, 7 years ago

  • 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).

by Imarok, 7 years ago

Attachment: 4020_altf4_v2.2.patch added


comment:8 by elexis, 7 years ago

Owner: set to elexis
Resolution: fixed
Status: newclosed

In 18613:

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

comment:9 by elexis, 7 years ago

Keywords: rfc removed

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.

comment:10 by elexis, 4 years ago

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:
Based on patch by: irishninja
Tested on: clang 8.0.1, Jenkins

Note: See TracTickets for help on using tickets.