Replay Control for STK

[schrowd]

Issue #5197 mentions adding the ability to pause, seek, move back and forth by individual frames and change
the speed of playback when watching replays. To this, Alayan responded "Pause, rewind and speed change are
all things I would like to have as well. This won't be in 1.5 and maybe not in 2.0, but it's definitely on the roadmap."

I have built the engine side of this and in accordance with the "Communicating with the team" section of the
contributing code guidelines I am making a forum post about it.

I have implemented this by means of a ReplayControl class that owns the replay clock. It can pause (setPlaying), move 
the head of the playback to a desired time (seek) and change the rate of playback (setRate). The ability to move back and 
forth by individual frames naturally falls out of this; you would just seek by a frame-sized delta (or move to an adjacent time
via m_all_times, both can work). This functionality is currently enabled via a --replay-control flag and without it it is inert.
This doesn't change the normal race clock: everything outside of watch replay mode remains untouched.

I have deliberately NOT added the UI. I've built the mechanism, however as it stands there is no user interface that allows
the user to control it. This is on purpose and has been done for two main reasons.

   1. Building UI blind without knowing if it is wanted or how the structure should be set out puts my time and effort at risk of being wasted.

   2. Where the UI lives, how it looks, where the buttons are, etc. are design decisions that are not my call to make.

This feature has a dependency on the fix outlined in pull request #5837 - without it, seeking backwards would cause the ghost
to freeze in place, which is far from ideal.

The diff can be found here: https://github.com/schrowd/stk-code/compare/fix-ghost-controller-seekable...feature-replay-control

(Note to reviewer: the three "removed" lines in world_status.cpp are still there, they're just indented one level deeper and so
 GitHub shows them as red. None of the original behaviour has been changed or deleted. You can see that exact same code in
 lines 479-481.)

To wrap this up, I'd like to ask a few questions:

• Is this something you'd want in the main game?
  
• Should I build the UI or is that something you would be doing?
  
• Is driving the world clock the correct approach?
  
• Where should this live? It currently lives in src/replay: does it belong somewhere else?
  
• Whose job should it be to handle frame-stepping - ReplayControl's or the UI's?
  

Those are the main questions I'd like answers for however if you have any other questions or concerns I'd be happy to answer them.

[Alayan]

Hello!

It is quite nice to see a code contributor willing to tackle an important feature and taking the time to check the contribution guide as well as properly thinking about architectural design.

So here are my answers to your ideas and questions.

I have implemented this by means of a ReplayControl class that owns the replay clock. It can pause (setPlaying), move 
the head of the playback to a desired time (seek) and change the rate of playback (setRate). The ability to move back and 
forth by individual frames naturally falls out of this; you would just seek by a frame-sized delta (or move to an adjacent time
via m_all_times, both can work). This functionality is currently enabled via a --replay-control flag and without it it is inert.
This doesn't change the normal race clock: everything outside of watch replay mode remains untouched.

I got quite confused when reading your message and looking at your code on whether or not your change is driving the main world clock.

Since tracks can have various dynamic elements that change over time, getting them to behave correctly when the replay is sped up, rewound, etc. is important to avoid things looking very odd (such as the ghost kart being hit when nothing seems to be there, or the ghost kart going through an obstacle it looks like it should be touching).

There are already several such obstacles in STK, and since dynamic track elements are quite fun, we plan to add more of them for Evolution, so handling them correctly is a requirement. This means that we need the replay controls to drive the main world clock and the events tied to it.

But looking at your code, I am pretty sure it is not going to work as needed in some respects:
- The game logic runs in ticks. There is conversion between time in floats and time in ticks (check the "setTime" function) and it seems to me that it messes with how your code tries to do things.
- It's unclear to me without digging deeper how the logic will run exactly, but it also seems to me that you will get desynchronization between what the state of the world ought to be at a given clock time (location of animated elements for example) and what it will actually be.

What we essentially need is to change the speed at which the game go through ticks (or the direction we go through them), without changing the changes that happen within one tick.

For pausing, slowing down, and speeding up, controlling the rate at which events are played ought to be sufficient (note that when slowing down, the same physics frame may be displayed for many graphics frame, that'd be the correct behavior rather than inventing intermediary states that didn't exist). There is already the pause menu of course, and during the start sequence, the game world is effectively paused, so that's an example of it already existing in practice (I think that came about because of different players having different track loading times online so world events couldn't start running early without causing synchronization issues).

As for rewinding, I could see that causing issues because you can't necessarily predict the past state based on the present state.

However, while it's trickier, technically rewinding is already supported because it is an essential feature of online play. It's just that as soon as the game rewinds back to an earlier state based on server-data, it immediately plays back the required frame to go back to present time, and the player never sees the game state going backwards.

So a potentially viable method here would be to build on the existing rewind code, with whatever extra is needed to make it work for replays (presumably creating rewind state info when the replay is played forward), and to have it happen progressively with the player seeing the world state change backward.

As a side note, at the current time, ghost replays don't store any data about how they influence the world (e.g. moved objects, nitro cans taken, etc.) which is the proper behavior when racing against a replay, or when comparing two replays (I don't think the limit is actually enforced, but comparison should only really be used when comparing recordings that have a single kart only, as with multiple karts in the same race influencing each other it just gets confusing).

But when purely watching a single replay, it'd actually be better if the effect on the world was also shown. Some of it could perhaps be stored in the replay, but having some of the events using the normal world loop and clock to infer what happens could avoid having to store exact data for every frame.

I have deliberately NOT added the UI. I've built the mechanism, however as it stands there is no user interface that allows
the user to control it. This is on purpose and has been done for two main reasons.

   1. Building UI blind without knowing if it is wanted or how the structure should be set out puts my time and effort at risk of being wasted.

   2. Where the UI lives, how it looks, where the buttons are, etc. are design decisions that are not my call to make.

That's very sensible.

I think there are three main phases around such a project:
- Establishing requirements and coming up with an initial architectural solution,
- Getting the underlying logic to work reliably in all situations,
- Fine-tuning how the feature is exposed to the user : UI design (buttons, locations, etc.), what options are exposed (code-wise it would be possible to allow arbitrary replay speeds, but that is likely unnecessary complexity for most users and purposes)

We are somewhere between the first and second phase here, since you already have some code done but some fundamental details are still to be decided.

All this to say that we actually need some kind of UI or keyboard controls or a mix to properly test things during the second phase. They don't need sophistication, beauty or thought about UX, but without a key or button to get the playback speed to change, I don't see how the correctness of replay control can be tested and validated in-game.

This feature has a dependency on the fix outlined in pull request #5837 - without it, seeking backwards would cause the ghost
to freeze in place, which is far from ideal.

I have taken a look, this seems fairly simple and sensible. The while loop to seek the correct index is not exactly an efficient algorithm, but this is entirely negligible in practical situations. There is a tiny indentation issue that I pointed out.

The diff can be found here: https://github.com/schrowd/stk-code/compare/fix-ghost-controller-seekable...feature-replay-control

(Note to reviewer: the three "removed" lines in world_status.cpp are still there, they're just indented one level deeper and so
 GitHub shows them as red. None of the original behaviour has been changed or deleted. You can see that exact same code in
 lines 479-481.)

The code is quite well-written and follows the typical style conventions found in STK code files, so that's great.

Now, based on what I wrote above, we'd need some basic controls (the simplest is probably to do it through keybinds, with perhaps a bit of UI display to show the current replay speed) to test it.

And there are of course the issues I mentioned, so there will be changes needed in relation to that. But the basic principle of dedicated code files for ReplayControl logic is sensible.

To wrap this up, I'd like to ask a few questions:

Is this something you'd want in the main game?

Yes, this is definitely something we want to be part of the main game. While people who seriously do time-trials are a small minority, they often are quite engaged with the game, and slowing down, pausing, rewinding, and speeding up would all be extremely useful features to analyze replays.

Furthermore, for Evolution we'd also like to allow proper recording of normal races, chiefly so that online multiplayer races can be recorded and rewatched. Controls on playback speed and the possibility to rewind would also be useful there.

I wouldn't add this feature for 1.5.1, though, as it's a complex feature that will require validation, and we don't have the freedom to update the replay format either (while not strictly required, it will likely come useful).

I would suggest using the Evolution (BalanceSTK2) branch as a base.

Should I build the UI or is that something you would be doing?

I wouldn't bother with making anything pretty or complex at this stage.

But a little extra code in the race UI to display the current replay speed factor (when in replay mode), and some basic keybinds to control what is happening (for example, by adding extra combinations to the Artist Debug Mode keybinds) would allow proper testing, so I would suggest doing that.

Is driving the world clock the correct approach?

Without context, I would say it is the correct approach. But I suppose you mean what your code does with setTime(), and I don't think it will work as we need it to for the reasons I explained earlier in this post.

Where should this live? It currently lives in src/replay: does it belong somewhere else?

Whose job should it be to handle frame-stepping - ReplayControl's or the UI's?

Although the ultimate aim is to control the World's behavior by controlling how it moves, I think that src/replay is the most sensible location to handle the core logic that determines at what speed things should be played. The UI ought to give the player a way to give commands to ReplayControl, but should then only pass them along, having the entire logic for managing the World handled in UI code would be messy.

I think additional code in src/modes to make World behave properly based on ReplayControl's settings will also be needed, but it wouldn't be a good place to track state.

Those are the main questions I'd like answers for however if you have any other questions or concerns I'd be happy to answer them.

I already mentioned my main thoughts regarding the feature and the code, so now I'll wait for your answer on them.

I guess I'm also interested on your thoughts about the code contribution guide. It's nice to see a new code contributor taking time to try to do a quality contribution, and we certainly want to make the experience of contributors nicer (selfishly, this also means more contributions to the project).

[schrowd]

Hi!

To preface this I would like to offer an explanation of what ReplayControl actually does and what it does not do.

I understand that some of what I wrote may not be easily understood from the outside (even from the inside it's a little confusing!) so let me explain. The advance() function is ReplayControl's own accumulator. One tick (in seconds) is passed into the function. From there, after checking if ReplayControl's pause is active and a check to see if the replay is finished, it adds the product of 1 tick and the rate multiplier to the head of the playback. This function's caller is WorldStatus::updateTime(). advance() runs and it's return value is given to setTime(), thus making m_head the source of truth when ReplayControl is enabled.

Essentially, when ReplayControl is active, instead of incrementing the clock normally, it goes through advance() instead. The way setTime() works is that it truncates to ticks and then feeds that same truncated tick count back to m_time. This is a non-issue under normal circumstances but under rate multipliers this inaccuracy quickly adds up, hence the need to keep m_head as an exact value and the need for ReplayControl to have it's own accumulator.

To tie it back to the world clock, moving m_head also moves the clock and changing the rate changes how much time is added to the world clock by means of multiplying the time delta. It writes to the same m_time and m_time_ticks that get called by anything that needs the world's time, such as a ghost kart. It does not change the world's stepping, 120 physics ticks pass
no matter what ReplayControl does, leading the desync with the world that you mentioned when the rate changes or when ReplayControl enables it's pause.

I got quite confused when reading your message and looking at your code on whether or not your change is driving the main world clock.

To answer your question: it does, however that's only part of the picture. It drives the clock, not the world, and I think that's where the discrepancy between what I've built and the future vision of the feature lies.

You outlined that pausing and changing the rate of playback via the world would be simple and I agree, the only slight challenge would be rewiring what I already have but even that's simple enough. Reversal of playback direction (rewinding) is another beast entirely though and it's the biggest task here by far.

Sadly there's no easy approach to it. Smooth and continuous rewind would most likely involve restoring to a snapshot every single frame and a rewind system like the one in multiplayer faces the risk of incurring noticable memory/performance problems in relation to snapshot frequency. Too often and your memory usage explodes, too sparse (say 2 seconds) and you cram 240 ticks worth of physics calculations into one frame which, as you can imagine, would cause a stutter. Perhaps an edit to the replay format would help a bit but that's beyond my scope and certainly a decision that cannot be made by myself (and, as you pointed out, on 1.5 we don't have the freedom to edit it). I could go on about the difficulties this poses but I think you get the gist.

Fortunately for us both, while I was waiting for a reply I built a prototype control interface for ReplayControl. As of writing it's only been a couple of hours since I finished it. No doubt there are a few style nits in the code itself but it's pretty clean and it works as intended so I think for a demo it'll be fine. If you want to give it a shot, here's the link:

https://github.com/schrowd/stk-code/tree...control-ui

For convenience, the commit message is here:

https://github.com/schrowd/stk-code/comm...223803aa68

Build it and then to enable it go to Singleplayer -> Ghost Replay Race -> [any replay] and then tick "Watch replay only" and then the "Enable replay control" box that appears. Then just start the race. The overlay then lists out the control scheme (This uses your normal race binds, not any artist debug binds. At the time of writing I elected to go with the normal ones for simplicity's sake), the current rate, the head of playback and the replay's duration.

This whole thing was built on the 1.5 branch and not the Evolution one as I wasn't aware that that was the ideal branch to work on. I'll be working on Evolution from now on.

On PR #5837, I fixed the indentation plus some tab indenting that was still there.

In regards to the code contribution guide, I think it covers pretty much everything you'd need to know in regards to contributing. Perhaps for the "Coding Style" section you could give a side-by-side example of code that fits the style and code that doesn't? We see code snippets in the "Layout" section yet the one titled "Coding" is a big text wall. I think some examples might help new contributors better understand the STK conventions.

[schrowd]

I will note here that I have reworked the replay control system to drive the world's tick rate as opposed to the clock. Under this new system, there only exists functionality for pausing and changing rate; rewinds are not implemented. The desync between the ghost kart and the world is gone as a result of the rework, tested against Fort Magma's cannonballs. I have also forward-ported it to the BalanceSTK2 branch. See it here:

https://github.com/schrowd/stk-code/tree...ontrol-evo

For the diff against Evolution:

https://github.com/supertuxkart/stk-code...ontrol-evo

As a side-note, PR #5837 isn't in BalanceSTK2. Ultimately, what you want to do with it is up to you.