Page MenuHomeWildfire Games

Add support for recording replay metadata when in nonvisual mode
ClosedPublic

Authored by elexis on Aug 20 2019, 2:15 AM.
Tags
None
Subscribers
Restricted Owners Package, Restricted Owners Package, Angen and 5 others
Tokens
"Like" token, awarded by Imarok."Like" token, awarded by irishninja."Like" token, awarded by elexis.

Details

Summary

Currently, the recording of replays relies on a method in the GUI JS script interface. This revision adds the getReplayMetadata function to the simulation script interface enabling replays to be recorded while in nonvisual mode.

Test Plan

I have mostly been testing this alongside the support for the RL interface as the RL interface exposes an option to record the replay when resetting the scenario. There may be a better way to test this independently.

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

There are a very large number of changes, so older changes are hidden. Show Older Changes
wraitii added subscribers: vladislavbelov, wraitii.

I have a vague memory of @vladislavbelov doing something similar, I'll try and find out.

elexis added a subscriber: elexis.Aug 20 2019, 12:05 PM
binaries/data/mods/public/simulation/helpers/Replay.js
14 ↗(On Diff #9410)

This is the metadata.json replacement? the code ought to be unified from GUI and simulation, instead of having two copies to be kept in sync.
So it could be GUIInterface.GetReplayMetadata that is called from both places, and the GUI just adds its GUI data separately (the separation is already the case as an anticipation for such things).

(Now where I look, it's a nearly exact copy of a functions_global_object.js function)

source/ps/Game.h
159 ↗(On Diff #9410)

The only effect of the inline keyword I could find is that gcc has an option to throw compiler warnings if it decides that inlining would be worse than not inlining.

source/ps/GameSetup/GameSetup.cpp
705 ↗(On Diff #9410)

(The VisualReplay namespace might be organized better in the future, as we see its not only about visual replays but also replay file management)

elexis retitled this revision from Add support for recording replays when in nonvisual mode to Add support for recording replay metadata when in nonvisual mode.Aug 20 2019, 12:06 PM
Stan added a subscriber: Stan.Aug 20 2019, 12:58 PM
Stan added inline comments.
source/ps/Game.cpp
78 ↗(On Diff #9410)

nullptr while at it :)

source/ps/Game.h
159 ↗(On Diff #9410)

I'm not sure it can be inlined at all, since it's called out of the class, and you can't call a private member from there. I wonder what happens in this case ?

source/ps/GameSetup/GameSetup.cpp
705 ↗(On Diff #9410)

Well it's not visual, but's that's irrelevant for this patch.

Thanks for the feedback - I have added my responses inline!

binaries/data/mods/public/simulation/helpers/Replay.js
14 ↗(On Diff #9410)

Yeah, the code duplication is not ideal. I confess this was added relatively quick as I was focusing on the RL interface. I will refactor this to unify the main logic and simply invoke the GUIInterface from this file and functions_global_object.js!

source/ps/Game.cpp
78 ↗(On Diff #9410)

So, I only changed this line by adding a comma. Would you still like me to change NULL to nullptr?

source/ps/Game.h
159 ↗(On Diff #9410)

I used the inline keyword to follow the same convention used by the similar functions in Game.h (IsGraphicsDisabled, IsGameStarted, etc) but I am happy to change it if preferred.

Stan added inline comments.Aug 20 2019, 7:15 PM
source/ps/Game.cpp
78 ↗(On Diff #9410)

Yeah :) In the end there should be no NULL in the codebase so we just replace them when we can.

irishninja updated this revision to Diff 9422.Aug 20 2019, 10:38 PM

I added GetReplayMetadata to the GuiInterface component to reduce code duplication introduced by the original diff.

irishninja updated this revision to Diff 9423.Aug 20 2019, 10:41 PM

Changed NULL to nullptr

The patch is looking like it's going into a healthy direction.

Also please add yourself to programming.json, with or without realname as you wish.

binaries/data/mods/public/gui/common/functions_global_object.js
90 ↗(On Diff #9422)

Also it would be good to add more context to the code (99999 lines if possible).
This would allow us to see the comment above this function:

/**
 * Also called from the C++ side when ending the game.
 * The current page can be the summary screen or a message box, so it can't be moved to session/.
 */
function getReplayMetadata()

So a really good improvement would be to completely delete this function and call the GUIInterface directly from C++ instead of going through the GUI to grab something from the simulation and get back to the GUI to get back to C++ :P

The function is called from VisualReplay::SaveReplayMetadata.
That already requires g_Game, which has the m_Simulation2 member.
Searching for GUIInterface in C++, I find JSI_Simulation::GuiInterfaceCall which should be a good example for the few lines needed to make that happen.

binaries/data/mods/public/simulation/components/GuiInterface.js
200 ↗(On Diff #9422)

Is Engine.GetInitAttributes() actually defined in the simulation context?
It's registered via JSInterface_Simulation, which is only registered in gui/scripting/ScriptFunctions.cpp, so I assume it's not available here.

But you have it in the previous patch too, so that is actually working? Why is it working? :s

If its not, there should definitely be a somewhat easy way to obtain it, since that is availble in simulation.GetInitAttributes().
The CComponentManager::CComponentManager has the RegisterFunction calls for the Simulation context, so if its not mysteriously working already, it should be possible to add the function there.

binaries/data/mods/public/simulation/helpers/Replay.js
14 ↗(On Diff #9422)

Please upload the patches relative to the current svn trunk or github mirror, it looks like this file was already committed to svn.

irishninja marked an inline comment as done.Aug 21 2019, 9:45 PM

Thanks for the feedback! I added some responses inline and will be updating the diff (hopefully addressing all the remaining issues)!

binaries/data/mods/public/simulation/components/GuiInterface.js
200 ↗(On Diff #9422)

Yeah, it seems I accidently included this change in the diff for the RPC interface (I missed it when pulling this diff out from it) and have been testing the headless replay using the RPC interface.

I had registered GetInitAttributes in CComponentManager as you suggested. I will get this added to the diff.

binaries/data/mods/public/simulation/helpers/Replay.js
14 ↗(On Diff #9422)

This was a file that I added myself so it shouldn't be committed to svn... However, the function appears to only be called in one spot so it will probably be easier to just use Engine.GuiInterfaceCall there.

irishninja updated this revision to Diff 9433.Aug 21 2019, 9:55 PM

Updates:

  • Removed duplicate functions for getReplayMetadata to more directly call the fn from the GuiInterface.
    • Updated VisualReplay::SaveReplayMetadata to call GuiInterface directly from the C++. After the code change, I removed the (now unused) ScriptInterface argument from the method signature.
    • Replaced the single invocation of getReplayMetadata with Engine.GuiInterfaceCall("GetReplayMetadata")
  • Added the registration of GetInitAttributes which should have been included in the original diff.
  • Added myself to the credits
elexis updated the Trac tickets for this revision.Aug 22 2019, 12:15 AM
elexis accepted this revision.Aug 22 2019, 3:54 AM
elexis added a subscriber: Imarok.

Ah, you copied that hunk directly from JSI_Simulation::GuiInterfaceCall, that's why there is an ENSURE and the name cxSim etc.

Really, for the future you need to upload patches in such a way that it contains all changes. One should only look at the most recent version to get all changes. Then if one wants to see what changed relative to the last patch, one can compare the versions in the web UI.
This is even more important for the Jenkins / Vulcan bot which applies the patch and tests it for compiling or linting errors.

Also notice that GUIInterface needs to have exposedFunctions inserted, since it's called from the JS GUI.

I can't tell you how glad I am that this gui/common/ hackery is removed, it was always a thorn in my eye (rP18613), not only because it shouldn't go through the GUI, but also because it didn't thank you irishninja!

@Imarok we require another thumbs up please

I click on accept since I don't have any further requests to the patch.

Here my revised version of the patch:

I will commit it, but I need the post-sleep state, not the pre-sleep one.

source/ps/VisualReplay.cpp
25 ↗(On Diff #9433)

alphabetic order

480 ↗(On Diff #9433)

sim -> simulation
Also we can derefence here so we can avoid it later.

So after all it seems nicer to pass CSimulation2& simulation as an argument, then one doesn't have the ugly g_Game global here, nor does one have to check whether it could be null or not - whereas the check already exists in the caller.

481 ↗(On Diff #9433)

I know this was copied, but in general I'm not a fan of ENSURE, because if it is triggered for a player, the player will click on "continue" and then the code flow continues like nothing happened.

Also this doesn't check whether g_Game is null.

487 ↗(On Diff #9433)

This can be done after the early-return to save few CPU cycles in case of error :P also groups the code a bit more.

495 ↗(On Diff #9433)

Hold on, why GetViewedPlayerID?
That should be wrong, because the data stored to metadata.json should not depend on the player assignments or selection of the observer, and I expect it to be not positive (as in -1) for non-visual replay.

Also avoiding this has the benefit of avoiding another global.

Looking at GetReplayMetadata in GUIInterface.js (which I can only find in the second version of the patch, but not in the first or later one), the function indeed doesn't depend on the viewed player, so I suggest to pass a fixed default:

helpers/Player.h:static const player_id_t INVALID_PLAYER = -1;

(That's also the ID for observers)

source/simulation2/system/ComponentManager.cpp
34 ↗(On Diff #9433)

I'm not happy about this inclusion, because the JSInterface_Simulation.h are functions meant to be called by the GUI, not from the simulation, and there is a strict separation by design between GUI and simulation (SimulationArchitecture).

If we look at that function, we see even more g_Game globals that should ultimately be removed.

It's again going from the simulation to the GUI code (at least intended for the GUI) back to the simulation, a detour. This may even be more problematic in the future if GUI and simulation run in different threads.

So the alternative is making a short function here.

But looking at how this would be achieved, I see no Simulation2 reference in this file at all, so I wonder if it's not even cleaner to Register this in Simulation2.cpp. This way the ComponentManager doesn't receive information about things that aren't components.

I see CSimulation2Impl::InitRNGSeedSimulation, CSimulation2Impl::InitRNGSeedAI are mutating the simulation ScriptInterface and located in Simulation2, so it's not without precedent. LoadTriggerScripts too kind of (its calling into the componentmanager again to load the script).

Since m_InitAttributes is stored in Simulation2.cpp and operated on in a number of places, it seems good to have the Getter function there.

Notice that we can even register the global variable directly instead of calling a function, making it a bit neater (m_ScriptInterface->SetGlobal).
But we can't use that without doubt as long as InitAttributes has a setter that can hypothetically be called at a later time.

If I look into JSI_Simulation::GetInitAttributes, I see it also does a CloneValueFromOtherContext, but the source and destination context are the same, so it seems this can be avoided.

So after removing the Clone call, it's exactly CSimulation2::GetInitAttributes(). I wanted to overload that, but it's the same, so yay.

So we can only register static functions and we can only get one pointer from the JScontext. JSI_Simulation::GetInitAttributes cheats by using globals. Examples that use the pointer (pCBData) instead of some global are the CComponentManager registered functions.

Now the problem is that we get a pointer to the ComponentManager, but the data is stored in CSimulation2. CSimulation2 owns the ComponentManager, but we have no reference going the other way.

So there needs to be some philosophy done here.

So due to the fact that we can only register static functions and that we dont have a link from ComponentManager to CSimulation2, and that adding this link seems like adding something out of place, and the fact that RegisterGlobal function only requires one line, that seems to be the least harmful way forward.

Also the existing GetInitSettings used from the GUI can also be removed and replaced by a GUIInterface call.

So it actually works with the global, and the code is much shorter, and we can kick out the old JSI_Simulation::GetInitAttributes!

Now before it can be considered safe, we need to consider that the InitAttributes variable is exposed to the entire simulation (the GUI to GUIInterface calls do cloning so those are safe), so it's better to freeze the object to ensure nothing is messing with it by chance.
The previous code also cloned in the JSInterface.
Freezing means adding a ScriptInterface FreezeObject call after the JS value was set.
That is actually after SetInitAttributes, as the simulation/helpers/Player.js script is ran over this JS value.
So I went to bed.

73 ↗(On Diff #9433)

Adding this at the bottom rather than at the top, just becaues it seems less important than the other functions which are the core of the simulation engine.

This revision is now accepted and ready to land.Aug 22 2019, 3:54 AM
irishninja marked 3 inline comments as done.Aug 22 2019, 7:49 AM

I will be updating the diff with the most recent fixes. Dumb question: when I update the diff, does it add the changes to the existing diff or overwrite it with the most recent diff? I usually use git and GitHub and am still getting used to Phabricator...

source/ps/VisualReplay.cpp
480 ↗(On Diff #9433)

Sounds good. Consider it updated. It is too bad that g_Game will still be used later to get the replay logger but it makes sense to minimize the number of occurrences of it.

481 ↗(On Diff #9433)

Gotcha. I will update this.

495 ↗(On Diff #9433)

Makes sense! I didn't really like using GetViewedPlayerID but hadn't found the viewed player constant...

irishninja updated this revision to Diff 9437.Aug 22 2019, 7:51 AM

Cleaned up the updates to the SaveReplayMetadata function (mentioned in the prior reviews). I also made the argument for the fn const since it is read-only.

I will be updating the diff with the most recent fixes.

It seems you didn't see the inline comments and the patch?
JSI_Simulation::GetInitAttributes is out of place in ComponentManager, and furthermore it can be deleted :-)

source/ps/Game.cpp
78 ↗(On Diff #9437)

I suppose the order isn't correct, some compilers may complain that m_IsVisualReplay should be initialised after m_IsSavingReplay.

source/simulation2/system/ComponentManager.cpp
34 ↗(On Diff #9433)

Probably we can separate JSInterface_Component and make it friend.

elexis commandeered this revision.Aug 22 2019, 12:58 PM
elexis edited reviewers, added: irishninja; removed: elexis.
elexis added inline comments.
source/ps/Game.cpp
78 ↗(On Diff #9437)

https://en.cppreference.com/w/cpp/language/initializer_list#Initialization_order

The order of member initializers in the list is irrelevant

?

source/ps/Game.h
161 ↗(On Diff #9437)

Moving the SaveReplayMetadata function to the CReplayLogger means that one can avoid VisualReplay including simulation code, and the replay writing function are grouped, even if the metadata.json relating files arent grouped anymore (same as with commands.txt)

source/simulation2/system/ComponentManager.cpp
34 ↗(On Diff #9433)

You didn't see my patch either :/

This revision now requires review to proceed.Aug 22 2019, 12:58 PM
vladislavbelov added inline comments.Aug 22 2019, 1:23 PM
source/ps/Game.cpp
78 ↗(On Diff #9437)

Yes, it's not an error, but I recommend to group similar properties in order.

source/simulation2/system/ComponentManager.cpp
34 ↗(On Diff #9433)

It wasn't a suggestion, just a possible solution.

So the important part is this, this way the ugly monster condition that copies the CGame constructor argument logic doesn't have to be replicated, kept in sync, and then bugfixes multiple times without ever becoming evidently correct, unless doing it this way:

Index: source/ps/Game.cpp
===================================================================
--- source/ps/Game.cpp	(revision 22752)
+++ source/ps/Game.cpp	(working copy)
@@ -100,10 +100,13 @@ CGame::~CGame()
 {
 	// Again, the in-game call tree is going to be different to the main menu one.
 	if (CProfileManager::IsInitialised())
 		g_Profiler.StructuralReset();
 
+	if (m_ReplayLogger)
+		m_ReplayLogger->SaveMetadata(*m_Simulation2);
+
 	delete m_TurnManager;
 	delete m_GameView;
 	delete m_Simulation2;
 	delete m_World;
 	delete m_ReplayLogger;
Index: source/ps/GameSetup/GameSetup.cpp
===================================================================
--- source/ps/GameSetup/GameSetup.cpp	(revision 22752)
+++ source/ps/GameSetup/GameSetup.cpp	(working copy)
@@ -697,14 +697,10 @@ static void ShutdownSDL()
 
 void EndGame()
 {
 	const bool nonVisual = g_Game && g_Game->IsGraphicsDisabled();
 
-	if (g_Game && g_Game->IsGameStarted() && !g_Game->IsVisualReplay() &&
-	    g_AtlasGameLoop && !g_AtlasGameLoop->running && !nonVisual)
-		VisualReplay::SaveReplayMetadata(g_GUI->GetActiveGUI()->GetScriptInterface().get());
-
 	SAFE_DELETE(g_NetClient);
 	SAFE_DELETE(g_NetServer);
 	SAFE_DELETE(g_Game);
 
 	if (!nonVisual)

Am I wrong?

elexis updated this revision to Diff 9528.Aug 27 2019, 8:43 PM

Actually upload the version of the patch where the ReplayLogger is moved.

Build failure - The Moirai have given mortals hearts that can endure.

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

With this version of the patch, there is a crash when cancelling the game during loading screen stage of a rmgen match:

WARNING: JavaScript warning: simulation/components/EndGameManager.js line 36
reference to undefined property this.gameSettings.victoryConditions
terminate called after throwing an instance of 'std::out_of_range'
  what():  vector::_M_range_check: __n (which is 1) >= this->size() (which is 0)

It seems this might be the same as the sim JS constant:

EndGameManager.prototype.GetGameSettings = function()
{
	return this.gameSettings;
};

and could perhaps be removed.

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

Linter detected issues:

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

Stan added a subscriber: Angen.Aug 28 2019, 12:40 PM
Stan added inline comments.
source/ps/GameSetup/GameSetup.cpp
699 ↗(On Diff #9528)

Why must this be deleted ? Wonder if that might affect the alt+tab crash fixed by @Angen...

elexis added inline comments.Aug 28 2019, 12:51 PM
source/ps/GameSetup/GameSetup.cpp
699 ↗(On Diff #9528)

Why must this be deleted ?

To achieve the purpose of the diff D2197 without making it worse.

Precisely https://code.wildfiregames.com/D2197#91967

Wonder if that might affect the alt+tab crash fixed by @Angen...

Since when are alt+tab crashes related to saving replay metadata?
In case you refer to this one https://code.wildfiregames.com/D2197#92719, no, this is local to this version of the patch, and it relates to ending the game before a variable in this patch is set, not performing alt+tab.

I see r22314 r21476, there it is about window fullscreen, renderer...

Stan added inline comments.Aug 28 2019, 3:04 PM
source/ps/GameSetup/GameSetup.cpp
699 ↗(On Diff #9528)

Sorry remember incorrectly D1212 and thought the code was analogous.

Stan added a comment.Sep 10 2019, 10:25 AM

@wraitii @elexis any changes you'd like to be made for that patch ?

This patch should be good to go, but the problem is that it depends on Freezing that initattributes, at least it sounds very fragile if we already had many previous code that changed the simulation state unintentionally which was fixed by freezing.
So the patch depends on D2213, and for that one I dont know yet (A) if its a good idea (and even correct) to remove the second persistentrooted value, and (B) at which place to call the freeze function.
(I'm not so familiar with that code either, so it's something new to learn for any of us looking at that.)
It's actually quite annoying to not have this patch committed, because it fixes an important defect (removing g_Game && g_Game->IsGameStarted() && !g_Game->IsVisualReplay() && g_AtlasGameLoop && !g_AtlasGameLoop->running && CRenderer::IsInitialised())).

When terminating the program in nonvisual nonreplay mode, it will exit immediately, without the Game destructor or anything else called.
So "Add support for recording replay metadata when in nonvisual mode" only applies to teh case where the game ended with a winner.
Thanks for the patch template!

binaries/data/mods/public/gui/session/session.js
45 ↗(On Diff #9528)

(without .settings)

source/simulation2/Simulation2.cpp
859 ↗(On Diff #9528)

Freezing should take place in this class, because its a member variable of this class.
It cant take place sooner, since LoadMapSettings modifies it.
Im not convinced that it should be frozen later, because the random map and trigger scripts shouldn't modify the init attributes but work with them and possibly store data elsewhere.
So perhaps one can find a cleaner place to Set and freeze, or perhaps something will be redesigned in the future. Until then it's the best place I could locate.

This revision was not accepted when it landed; it landed in state Needs Review.Sep 25 2019, 12:06 PM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Owners added subscribers: Restricted Owners Package, Restricted Owners Package.Sep 25 2019, 12:06 PM