Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement game save plumbing and enable for missions #16411

Merged
merged 10 commits into from Apr 20, 2019

Conversation

@pchote
Copy link
Member

commented Apr 13, 2019

This PR implements game saving and loading using order resimulation.

This implements a couple of tricks to speed the process up vs replays:

  • World rendering is disabled, and UI rendering capped at 5 FPS
  • Sync calculations are disabled until the very end

Also note that chat and other immediate orders are not saved (to save confusion when clients change in MP saves).

On my machine I see a 30-40x real time restoration for one of the TD campaign missions.

Another big difference from replays is that traits can store arbitrary data that is returned on load - this is used to restore the viewport and selection groups. In the future we could expand on this if we want to implement a proper save/load using serialization.

Saves are enabled for campaign missions, and compatible with (but disabled for) skirmishes. A followup PR will enable skirmish savegames after implementing IGameSaveTraitData on the bot modules to properly restore their state.

This also lays the groundwork for multiplayer saves, with several TODOs noting the remaining work to implement these.

Closes #2743.
Supersedes #6287.

// Enable game saves for singleplayer missions only
// TODO: Enable for skirmish once the AI supports state-restoration
// TODO: Enable for multiplayer (non-dedicated servers only) once the lobby UI has been created
LobbyInfo.GlobalSettings.GameSavesEnabled = !Dedicated && Map.Visibility == MapVisibility.MissionSelector &&

This comment has been minimized.

Copy link
@pchote

pchote Apr 13, 2019

Author Member

Remove the Map.Visibility == MapVisibility.MissionSelector here to enable saving in skirmish.

@pchote pchote added this to the Next Release milestone Apr 13, 2019

@pchote pchote force-pushed the pchote:serverside-savegames branch 3 times, most recently from 4ea96ec to 1949dea Apr 13, 2019

@pchote pchote force-pushed the pchote:serverside-savegames branch from 1949dea to 6bb1075 Apr 13, 2019

@pchote

This comment has been minimized.

Copy link
Member Author

commented Apr 13, 2019

Fixed.

@reaperrr

This comment has been minimized.

Copy link
Contributor

commented Apr 13, 2019

Can't say much about the code, other than that it seems to work (loading a skirmish game seemingly doesn't work even without bots, but I assume that's to be expected and will be fixed with the next PR either way).
Saving and loading missions seems to work flawlessly, didn't see any UI regressions either.
Cautious 👍 from me, though ideally I'd like to see @obrakmann, @chrisforbes or @RoosterDragon to check and approve the code before it's merged.
Some more testing from others wouldn't hurt, either.

@pchote

This comment has been minimized.

Copy link
Member Author

commented Apr 13, 2019

The first pushed version had issues with skirmish games, but these should now be fixed (provided you comment out the check that disables them).

@matjaeck

This comment has been minimized.

Copy link
Contributor

commented Apr 13, 2019

Don't know why but I can't load into these replays.zip

Unlike the filenames might tell, it's not caused by saving during demolition, this works as expected.

@matjaeck

This comment has been minimized.

Copy link
Contributor

commented Apr 14, 2019

Aborting loading of a saved game reliably breaks any ingame sounds when I start a mission after it.

Got this exception while trying to load into one of the replays posted above (unfortunately don't know which one), after extracting again from the zip archive.

OpenRA engine version {DEV_VERSION}
Red Alert mod version {DEV_VERSION}
Date: 2019-04-13 23:48:30Z
Operating System: Linux (Unix 4.15.0.47)
Runtime Version: Mono 5.18.1.0 (tarball Fri Mar 15 20:41:32 UTC 2019) CLR 4.0.30319.42000
Exception of type `System.InvalidOperationException`: Out of sync in frame 177.
 Compare syncreport.log with other players.
  at OpenRA.Network.OrderManager.OutOfSync (System.Int32 frame) [0x00037] in <9c96625fb1da40059cf161732c28f14c>:0 
  at OpenRA.Network.OrderManager.CheckSync (System.Byte[] packet) [0x00034] in <9c96625fb1da40059cf161732c28f14c>:0 
  at OpenRA.Network.OrderManager+<>c__DisplayClass40_0.<TickImmediate>b__3 (System.Int32 clientId, System.Byte[] packet) [0x00038] in <9c96625fb1da40059cf161732c28f14c>:0 
  at OpenRA.Network.ReplayConnection.Receive (System.Action`2[T1,T2] packetFn) [0x00052] in <9c96625fb1da40059cf161732c28f14c>:0 
  at OpenRA.Network.OrderManager.TickImmediate () [0x000cb] in <9c96625fb1da40059cf161732c28f14c>:0 
  at OpenRA.Sync+<>c__DisplayClass13_0.<RunUnsynced>b__0 () [0x00000] in <9c96625fb1da40059cf161732c28f14c>:0 
  at OpenRA.Sync.RunUnsynced[T] (System.Boolean checkSyncHash, OpenRA.World world, System.Func`1[TResult] fn) [0x00023] in <9c96625fb1da40059cf161732c28f14c>:0 
  at OpenRA.Sync.RunUnsynced (System.Boolean checkSyncHash, OpenRA.World world, System.Action fn) [0x0000d] in <9c96625fb1da40059cf161732c28f14c>:0 
  at OpenRA.Game.InnerLogicTick (OpenRA.Network.OrderManager orderManager) [0x000f6] in <9c96625fb1da40059cf161732c28f14c>:0 
  at OpenRA.Game.LogicTick () [0x00071] in <9c96625fb1da40059cf161732c28f14c>:0 
  at OpenRA.Game.Loop () [0x000e0] in <9c96625fb1da40059cf161732c28f14c>:0 
  at OpenRA.Game.Run () [0x0003c] in <9c96625fb1da40059cf161732c28f14c>:0 
  at OpenRA.Game.InitializeAndRun (System.String[] args) [0x00010] in <9c96625fb1da40059cf161732c28f14c>:0 
  at OpenRA.Program.Main (System.String[] args) [0x00044] in <9c96625fb1da40059cf161732c28f14c>:0

Might not be related but my hotkey setup (game, unit, unit stances) has partly been reset after starting a mission. The viewport and production hotkeys however have not been reset.

@pchote

This comment has been minimized.

Copy link
Member Author

commented Apr 14, 2019

@matjaeck did you try to reload a save from an earlier version of the PR using the latest version? I believe I rebased, which may well have introduced sync-affecting changes from bleed.

@matjaeck

This comment has been minimized.

Copy link
Contributor

commented Apr 14, 2019

These have been saved using your remote branch. But I'll fetch the PR like I would do usually and try to reproduce to make sure.

@matjaeck

This comment has been minimized.

Copy link
Contributor

commented Apr 14, 2019

@pchote I can not guarantee that these are from the the updated branch so better consider the comment as false positive for now. But I can reproduce the mentioned sound issue.

@pchote pchote force-pushed the pchote:serverside-savegames branch from 6bb1075 to cc75b73 Apr 14, 2019

@pchote

This comment has been minimized.

Copy link
Member Author

commented Apr 14, 2019

Fixed the sound issue.

OpenRA.Mods.Common/Widgets/Logic/MainMenuLogic.cs Outdated Show resolved Hide resolved
OpenRA.Game/Network/GameSave.cs Show resolved Hide resolved
OpenRA.Game/Network/GameSave.cs Outdated Show resolved Hide resolved
@@ -105,7 +110,7 @@ void CacheChatLine(Color color, string name, string text)
public void TickImmediate()
{
var immediateOrders = localOrders.Where(o => o.IsImmediate).ToList();
if (immediateOrders.Count != 0)
if (immediateOrders.Count != 0 && GameSaveLastFrame < NetFrameNumber + FramesAhead)

This comment has been minimized.

Copy link
@teinarss

teinarss Apr 14, 2019

Contributor

NetFrameNumber + FramesAhead is used in several places, could be made to its own property

This comment has been minimized.

Copy link
@pchote

pchote Apr 14, 2019

Author Member

Can you suggest a name for this?

@reaperrr

This comment has been minimized.

Copy link
Contributor

commented Apr 14, 2019

Can confirm that both saving and loading bot-less skirmishes now works.

@pchote pchote force-pushed the pchote:serverside-savegames branch from cc75b73 to b6ad591 Apr 14, 2019

@pchote

This comment has been minimized.

Copy link
Member Author

commented Apr 14, 2019

Updated and rebased (so any existing saves will now be invalid).

OpenRA.Game/Network/GameSave.cs Outdated Show resolved Hide resolved
@reaperrr

This comment has been minimized.

Copy link
Contributor

commented Apr 18, 2019

Loading skirmishes works and couldn't reproduce that sound issue after last updates.

One thing, though:
In my opinion, the GameSaveViewportManager commit should get at least a notification update rule that tells modders to manually add the trait, and ideally refer them to a wiki entry detailing required UI changes, too.
UI changes have by now become the biggest showstopper for easy transitions to newer engine versions, as they (usually) don't get update rules and cannot be caught by the linter.

@pchote pchote force-pushed the pchote:serverside-savegames branch from 62cb4ab to aea4d90 Apr 19, 2019

@pchote

This comment has been minimized.

Copy link
Member Author

commented Apr 19, 2019

I accidentally commited my test change to enable skirmish saves - fixed.

I have also created https://github.com/OpenRA/OpenRAModSDK/wiki/Update-notes:-release-20190314-to-next#game-saves as mentioned above.

@matjaeck
Copy link
Contributor

left a comment

I give this a 👍 because creating, loading, renaming and deleting save points works without issues for me. Save point replays work too. This is a big improvement when playing long or very hard missions, great job!
However, I'd suggest to consider the following changes for future PR's:

  • Don't show the save button when a mission is won/lost. The player does not need to save anything here.
  • Don't show the save button when watching replays. It won't create a save point but this button should not appear in a replay context. Perhaps remove the load button from replays too to make it clear to the player that he is in "replay mode".
  • If possible, don't start the replay with the opened briefing/options screen, "normal" replays don't do this.
  • If possible, don't play the "mission saved/loaded" notification in replays. I'm concerned that it will confuse players.

@pchote pchote force-pushed the pchote:serverside-savegames branch from aea4d90 to d8e524a Apr 20, 2019

@pchote

This comment has been minimized.

Copy link
Member Author

commented Apr 20, 2019

Updated:

  • Fixed all of @matjaeck's requests.
  • Added a new commit working around an ugly issue with replays ignoring state.
  • Split the cursor hiding into its own commit, as this is now shared with #16414.
  • Moved some fixups that were accidentally in the wrong commits where they belong.

@pchote pchote force-pushed the pchote:serverside-savegames branch from d8e524a to 3c21e46 Apr 20, 2019

@pchote

This comment has been minimized.

Copy link
Member Author

commented Apr 20, 2019

I suspect I have a better solution for the replay issue - to be investigated.

@pchote pchote force-pushed the pchote:serverside-savegames branch from 3c21e46 to 5ede77c Apr 20, 2019

@pchote

This comment has been minimized.

Copy link
Member Author

commented Apr 20, 2019

Fixed the replay issue properly by sending the trait data before the orders stream and dropped the redundant workaround commit.

@matjaeck

This comment has been minimized.

Copy link
Contributor

commented Apr 20, 2019

Thanks for fixing this here, I've tested it again and everything seems to work so consider my 👍 still valid.

@reaperrr
Copy link
Contributor

left a comment

Looks good as far as I can tell, SP savegames still work, and I trust @matjaeck's testing more than my own anyway so LGTM.

@reaperrr reaperrr merged commit d5588c5 into OpenRA:bleed Apr 20, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@pchote pchote deleted the pchote:serverside-savegames branch Aug 26, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.