Camera movement when walking is jittering

[HybridDog]

SuperTux version: current master and older System information: lubuntu 18.04

Expected behavior

Walking should not be jittery.

Actual behavior

I noticed that supertux does not run as liquidly as the very old supertux-stable (I think it was version 1.3). When walking it looks like in intervals of a few hundred milliseconds frames are skipped. I could record the behaviour with simplescreenrecorder, it worked surprisingly well: mp4 video You can download the file, remove the .png extension (github doesn't allow uploading videos directly) and watch the video with mpv. When the camera moves to the right in mpv, you can press . repeatedly to view the single frames. About every 6, 8, 9 or 10 frames the camera moves a bit more between two frames, variable camera speeds make supertux look jittery.

Steps to reproduce actual behavior

Give attention to how frames are displayed when walking with the camera moving with constant speed.

[tobbi]

Does this also exist in the current nightly? http://download.supertux.org

[HybridDog]

I compiled the newest version and it still happens.

If I change MAX_FRAME_SKIP to 1 and do not disable vsync with vblank_mode=0, the movement looks looks liquid. I can even see ghosting more easily (I think my screen is of low quality). https://github.com/SuperTux/supertux/blob/96ab21a56d2fc77d87a5d9380eb1edd9c064a3b2/src/supertux/screen_manager.cpp#L41

[HybridDog]

I found a fix:

diff --git a/src/supertux/screen_manager.cpp b/src/supertux/screen_manager.cpp
index 098b926..6bc6e42 100644
--- a/src/supertux/screen_manager.cpp
+++ b/src/supertux/screen_manager.cpp
@@ -36,8 +36,8 @@
 
 #include <stdio.h>
 
-/** don't skip more than every 2nd frame */
-static const int MAX_FRAME_SKIP = 2;
+/** wait at least MIN_TICKS for every frame */
+static const int MIN_TICKS = 2;
 
 ScreenManager::ScreenManager(VideoSystem& video_system) :
   m_waiting_threads(),
@@ -412,27 +412,24 @@ ScreenManager::run()
       elapsed_ticks = 0;
     }
 
-    if (elapsed_ticks < ticks_per_frame)
+    if (elapsed_ticks == 0)
     {
-      Uint32 delay_ticks = ticks_per_frame - elapsed_ticks;
+      Uint32 delay_ticks = MIN_TICKS;
       SDL_Delay(delay_ticks);
       last_ticks += delay_ticks;
       elapsed_ticks += delay_ticks;
     }
 
-    int frames = 0;
-
-    while (elapsed_ticks >= ticks_per_frame && frames < MAX_FRAME_SKIP)
+    if (elapsed_ticks >= MIN_TICKS)
     {
-      elapsed_ticks -= ticks_per_frame;
-      float timestep = 1.0f / m_target_framerate;
+      float timestep = elapsed_ticks / (1000.0 * g_game_speed);
+      elapsed_ticks = 0;
       g_real_time += timestep;
       timestep *= m_speed;
       g_game_time += timestep;
 
       process_events();
       update_gamelogic(timestep);
-      frames += 1;
     }
 
     if (!m_screen_stack.empty())

Instead of calling update_gamelogic always with the same timestep, timestep is variable now. This disables the jerking with en- and disabled vsync.

[tobbi]

@Grumbel Does this have any adverse effects on anything?

[Grumbel]

There are some enemies (e.g. icecrusher) that can't deal with timesteps other than the default. Those need to be fixed.

[HybridDog]

I noticed that Tux is jerking a bit with my change when vsync is enabled, without vsync Tux doesn't jitter.

[tobbi]

@HybridDog Can you submit a PR so that we can easily merge this?

[HybridDog]

Sorry, pulling and pushing takes a long time, so I can't easily create a PR now. But you can download the patch and apply it with git am: 0001-screen_manager-Use-variable-timestep.patch.txt

[HybridDog]

I managed to push now. Here's the PR: #1055

[HybridDog]

This is still not fixed; my change was bad, e.g. since it made the game non-deterministic.