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.
Details
Details
- Reviewers
wraitii irishninja - Commits
- rP22991: Save replay metadata for non-visual games too, refs #4577, #5548, fixes #5565.
- Trac Tickets
- #5565
#4020
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
Diff Detail
- Repository
- rP 0 A.D. Public Repository
- Lint
Lint Skipped - Unit
Unit Tests Skipped
Event Timeline
There are a very large number of changes, so older changes are hidden. Show Older Changes
Comment Actions
I have a vague memory of @vladislavbelov doing something similar, I'll try and find out.
Comment Actions
| 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. (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) |
| source/ps/Game.cpp | ||
|---|---|---|
| 78 | 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. |
Comment Actions
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 | 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. |
| source/ps/Game.cpp | ||
|---|---|---|
| 78 | Yeah :) In the end there should be no NULL in the codebase so we just replace them when we can. | |
Comment Actions
I added GetReplayMetadata to the GuiInterface component to reduce code duplication introduced by the original diff.
Comment Actions
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). /** * 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. |
| binaries/data/mods/public/simulation/components/GuiInterface.js | ||
| 200 ↗ | (On Diff #9422) | Is Engine.GetInitAttributes() actually defined in the simulation context? 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(). |
| 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. |
Comment Actions
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. |
Comment Actions
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
Comment Actions
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 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? 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:
(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). 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. |
| 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. |
Comment Actions
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... |
Comment Actions
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.
Comment Actions
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 | https://en.cppreference.com/w/cpp/language/initializer_list#Initialization_order
? | |
| 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 :/ |
Comment Actions
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?
Comment Actions
Build failure - The Moirai have given mortals hearts that can endure.
Link to build: https://jenkins.wildfiregames.com/job/docker-differential/508/display/redirect
Comment Actions
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.
Comment Actions
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
| 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... |
| source/ps/GameSetup/GameSetup.cpp | ||
|---|---|---|
| 699 ↗ | (On Diff #9528) |
To achieve the purpose of the diff D2197 without making it worse. Precisely https://code.wildfiregames.com/D2197#91967
Since when are alt+tab crashes related to saving replay metadata? I see r22314 r21476, there it is about window fullscreen, renderer... |
Comment Actions
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())).
Comment Actions
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. |