sergiou87 / open-supaplex

Reverse engineering Supaplex
245 stars 39 forks source link

VSync timing issues #19

Open NY00123 opened 3 years ago

NY00123 commented 3 years ago

Before getting into the issues themselves, it's clear that there's been a great deal of work done while disassembling the game. Porting the code was also no little work.

Generally speaking, in terms of gameplay, it feels quite accurate to me.

Regarding VSync:

  1. The game may be a bit slower than expected while VSync is enabled. In that case, it appears to run more-or-less the same with the game speeds of 9 and 10, while 10 is a bit faster without VSync. This is probably a side-effect of the app trying to display more than 60 distinct frames per second while using a 60Hz monitor. I'm not familiar with the details, but the solution for this should probably involve decoupling in-game timing from display updates, while skipping frames if required.

  2. Every once in a while, there might be a problem of frame pacing, like the game suddenly goes from 60fps to a rate which feels closer to 30fps, at least for a second or two. This might naturally be more difficult to solve, since reproducing the problem itself can also be difficult. Maybe if the first problem will be resolved, the changed code will also take care of the second one.

For now, I simply removed the addition of the flag SDL_RENDERER_PRESENTVSYNC for my build. Still, this is again great work.

sergiou87 commented 3 years ago

Hello @NY00123!! πŸ‘‹

Thank you for your kind words! I definitely did my best to get it to this point, so I'm glad you like it!! ❀️

Regarding the issue you report, I think you're right it's a bit weird. From my understanding, the original game was written with 70Hz monitors in mind (that'd be the game speed 10). Disabling VSync will let you get to 70fps, but you would get tearing in result, unless you play in a >60Hz screen able to refresh at 70Hz.

https://github.com/sergiou87/open-supaplex/blob/1f8989e2ba3882758ef2585600ff7560af42a109/src/graphics.c#L1307-L1328

I don't know what the best solution would be... Maybe disabling Vsync for game speed 10? Or adding Vsync as a completely separate setting?

but the solution for this should probably involve decoupling in-game timing from display updates, while skipping frames if required.

This is something I considered, but the logic of the game in many parts is already coupled to the display updates like you describe, which means a huge refactor of the game and potentially diverge from the original implementation, behavior and feeling 😒

In the end, the game was written to be rendered at 70fps, so anything different than that will be "worse". Of course, that also depends on the machine where you originally played the game πŸ˜„ If your computer was much slower and you were used to play it at 35fps, that's what will feel right to you πŸ˜†

NY00123 commented 3 years ago

Thanks for your feedback!

The game was indeed written to work in specific aways. Changes are still required from the original DOS versions in a port, though, so these can be done appropriately.

For the sake of simplicity, I'll concentrate just at the game loop. I think that the code in question involves the loop under runLevel, as well as the calls to the functions handleGameIterationStarted, handleGameIterationFinished and videoLoop (at least the first call to it).

While this might not exactly be how is the port's loop working like, it can be imagined that the significant part looks like this:

while (1)
{
  runGameIter();
  checkInput();
  render();
  waitForNextIter();
}

In order to make this work as expected with VSync, you can take advantage of the way timing is done, as used for waitForNextIter, in order to change the loop to look more like this:

while (1)
{
  while (pendingItersLeft())
  {
    runGameIter();
    checkInput();
  }
  render();
  waitForNextIter();
}

The idea here is simple. If the call to render took more than one game iteration, it's possible that you may want to run two such iterations before rendering the next frame. waitForNextIter will return immediately in such a case.

It's still true that without a 70Hz+ monitor, you can't truly display 70fps or more, but changing the code as described here should at least make the game's effective tick rate more consistent.

sergiou87 commented 3 years ago

In that pseudocode, how would you calculate how many interations are left? (pendingItersLeft())

NY00123 commented 3 years ago

It can basically work like this:

For technical reasons, there may be some complications related to the accuracy of results, as well as handling wrap-arounds after (very) long runs (in case it's relevant), but this is the basic idea.

sergiou87 commented 3 years ago

But you also need to know how many iterations fit between vsyncs, no? And that's what you don't know (unless you also measure that).

You can measure how long a game iteration takes, but you don't know if you can fit another one before the next vsync. And even if you know, for whatever reason the iteration could take longer and you'd skip a frame… although that shouldn't be a big issue if we're talking about more fps than the display can handle, since it will skip "frames" anyway πŸ˜†

Am I missing something here? πŸ€”

NY00123 commented 3 years ago

The thing here is that there isn't necessarily a need to measure how much time passed between vsyncs, if there's any at all.

When it's decided that it's time to wait for the next game tick, you try waiting for the desired amount of time, while possibly also taking advantage of this opportunity for updating the window. After doing both, you check how much time passed in practice. If it was in-between 1-2 game ticks, you execute one more iteration. If it was more than 1 game tick, you repeatedly execute 2 or more iterations before waiting and/or drawing again.

vanfanel commented 3 years ago

Whatever you end up doing, please leave the current behaviour as an option because the game movement is PERFECT. On GNU/Linux, SDL2 on KMSDRM, the game is absolutely smooth as intended. If you sync on internal timers, smoothness will go to hell. So, if you do, please, please, do it optionally and let us wait on vsync as you currently do, don't break the engine speed just to mimic the strange framerates of DOS (35FPS or 70FPS are just absurd on today's displays).

Waiting on internal timers is what OpenTyrian does, just to match the strange DOS framerates, and look how UGLY it's scrolling movement is on any modern display...