Merge lp:~widelands-dev/widelands/lock_game_logic into lp:widelands

[SirVer]

Decouples UI update frequency from game update frequency (which is now 15 times per second).

This is a hacky-hack. The game logic should not be driven by the UI, but that is such a fundamental design in Widelands that I am not sure if we can ever pull that apart.

For now, this should fix desyncs, I could not reproduce them on my system anymore.

[GunChleoc (gunchleoc)]

Not tested, but code LGTM.

Maybe add a comment to the code to explain why we have the 2 separate intervals?

review: Approve (code)

[kaputtnik (franku)]

Is there something specific to look at? I tested a short 2Player game against AI and found nothing strange. Does this affect calculation of statistics f.e. the productivity shown on each building? Is a game with 8 players better to test this changes?

[Klaus Halfmann (klaus-halfmann)]

Hello Kaputtnick:

I just review that code, you wrote:

> Is there something specific to look at?

I think we should do tests in the „real world“ with different FPS seconds and a real
Network jitter. SirVers approach feels correct to me, but I am not sure is this
may have adverse effects, as the game logic now will run at 15 thinks()/s compared
to 25/s (with default settings) before.

Worst secario would we a slow computer in australia on ad 1024k Modem (Windows)
vs. a Workstation on Linux/OSX on 1 fibre net. But I tend to exaggerate such issues :-)

> I tested a short 2Player game against AI and found nothing strange.

This should not affect any local game, AFAI can imagine

> Does this affect calculation of statistics f.e. the productivity shown on each building?

No idea, good question,

> Is a game with 8 players better to test this changes?

Yes as many players as spoonable with different FPS.
But not in a LAN-Party :-)

[GunChleoc (gunchleoc)]

This is how SirVer triggered the bug:

run widelands with --maxfps=10 and host a game, run a second one with --maxfps=60 and join that game -> immediate desyncs.

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 685. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/109052981.
Appveyor build 537. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_lock_game_logic-537.

[Klaus Halfmann (klaus-halfmann)]

Hello SirVer:

Id propose a small change, mostly comments only

=== modified file 'src/ui_basic/panel.cc'
--- src/ui_basic/panel.cc 2016-02-13 19:17:06 +0000
+++ src/ui_basic/panel.cc 2016-02-14 11:20:39 +0000
@@ -148,9 +148,10 @@
  // Panel-specific startup code. This might call end_modal()!
  start();

- // think() is called at most 15 times per second.
+ // think() is called at most 15 times per second, thats every 66ms
  const uint32_t kGameLogicDelay = 1000 / 15;

+ // with a default of 30 FPS the gemay will be drawn every 33ms
  const uint32_t draw_delay =
     1000 / std::max(5, g_options.pull_section("global").get_int("maxfps", 30));

@@ -163,8 +164,9 @@
   Panel::ui_mousewheel
  };

- uint32_t next_think_time = SDL_GetTicks() + kGameLogicDelay;
- uint32_t next_draw_time = SDL_GetTicks() + draw_delay;
+ uint32_t sdl_ticks = SDL_GetTicks();
+ uint32_t next_think_time = sdl_ticks + kGameLogicDelay;
+ uint32_t next_draw_time = sdl_ticks + draw_delay;
  while (_running) {
   const uint32_t start_time = SDL_GetTicks();

I would like to test this „in the wild“ for a while.

[Klaus Halfmann (klaus-halfmann)]

A test with TinoM palying on Impact with an AI, failed, going to reprodcu this

[SirVer (sirver)]

I applied all code review comments now.

Apparently, my analyze of the problem was either flawed or incomplete, since this has been reproduced again even with this patch.

However, I still think it is useful to get this in, because the game logic should not depend on the refresh rate of the graphics. Next steps for the sync bug is to enable syncstream dumps through a option (instead of for debug or release builds) so that we can get debug data more easily. I'll prepare a branch for that soonish.

[GunChleoc (gunchleoc)]

Sounds good to me :)

@bunnybot merge

=== modified file 'src/economy/request.cc'
--- src/economy/request.cc	2016-02-07 06:10:47 +0000
+++ src/economy/request.cc	2016-02-14 19:35:24 +0000
@@ -351,7 +351,7 @@
 			economy_->remove_request(*this);
 		economy_ = e;
 		if (economy_ && is_open())
-			economy_->   add_request(*this);
+			economy_->add_request(*this);
 	}
 }
 

=== modified file 'src/ui_basic/panel.cc'
--- src/ui_basic/panel.cc	2016-02-07 16:31:06 +0000
+++ src/ui_basic/panel.cc	2016-02-14 19:35:24 +0000
@@ -148,41 +148,55 @@
 	// Panel-specific startup code. This might call end_modal()!
 	start();
 
-	const uint32_t minimum_frame_time =
+	// think() is called at most 15 times per second, that is roughly ever 66ms.
+	const uint32_t kGameLogicDelay = 1000 / 15;
+
+	// With the default of 33FPS, the game will be drawn every 33ms.
+	const uint32_t draw_delay =
 	   1000 / std::max(5, g_options.pull_section("global").get_int("maxfps", 30));
 
+	static InputCallback input_callback = {
+		Panel::ui_mousepress,
+		Panel::ui_mouserelease,
+		Panel::ui_mousemove,
+		Panel::ui_key,
+		Panel::ui_textinput,
+		Panel::ui_mousewheel
+	};
+
+	const uint32_t initial_ticks = SDL_GetTicks();
+	uint32_t next_think_time = initial_ticks + kGameLogicDelay;
+	uint32_t next_draw_time = initial_ticks + draw_delay;
 	while (_running) {
-		const uint32_t startTime = SDL_GetTicks();
-
-		static InputCallback icb = {
-			Panel::ui_mousepress,
-			Panel::ui_mouserelease,
-			Panel::ui_mousemove,
-			Panel::ui_key,
-			Panel::ui_textinput,
-			Panel::ui_mousewheel
-		};
-
-		app->handle_input(&icb);
-		if (app->should_die())
-			end_modal<Returncodes>(Returncodes::kBack);
-
-		do_think();
-
-		RenderTarget& rt = *g_gr->get_render_target();
-		forefather->do_draw(rt);
-		rt.blit(app->get_mouse_position() - Point(3, 7),
-		        WLApplication::get()->is_mouse_pressed() ? s_default_cursor_click : s_default_cursor);
-		forefather->do_tooltip();
-		g_gr->refresh();
-
-		if (_flags & pf_child_die)
-			check_child_death();
-
-		//  Wait until 1second/maxfps are over.
-		const uint32_t frame_time = SDL_GetTicks() - startTime;
-		if (frame_time < minimum_frame_time) {
-			SDL_Delay(minimum_frame_time - frame_time);
+		const uint32_t start_time = SDL_GetTicks();
+
+		app->handle_input(&input_callback);
+
+		if (start_time >= next_think_time) {
+			if (app->should_die())
+				end_modal<Returncodes>(Returncodes::kBack);
+
+			do_think();
+
+			if (_flags & pf_child_die)
+				check_child_death();
+			next_think_time = start_time + kGameLogicDelay;
+		}
+
+		if (start_time >= next_draw_time) {
+			RenderTarget& rt = *g_gr->get_render_target();
+			forefather->do_draw(rt);
+			rt.blit(app->get_mouse_position() - Point(3, 7), WLApplication::get()->is_mouse_pressed() ?
+			                                                    s_default_cursor_click :
+			                                                    s_default_cursor);
+			forefather->do_tooltip();
+			g_gr->refresh();
+			next_draw_time = start_time + draw_delay;
+		}
+
+		int32_t delay = std::min<int32_t>(next_draw_time, next_think_time) - SDL_GetTicks();
+		if (delay > 0) {
+			SDL_Delay(delay);
 		}
 	}
 	end();

=== modified file 'src/wui/interactive_base.cc'
--- src/wui/interactive_base.cc	2016-02-01 11:46:58 +0000
+++ src/wui/interactive_base.cc	2016-02-14 19:35:24 +0000
@@ -307,13 +307,6 @@
 */
 void InteractiveBase::think()
 {
-	// Timing
-	uint32_t curframe = SDL_GetTicks();
-
-	frametime_ = curframe - lastframe_;
-	avg_usframetime_ = ((avg_usframetime_  * 15) + (frametime_  * 1000)) / 16;
-	lastframe_ = curframe;
-
 	// If one of the arrow keys is pressed, scroll here
 	const uint32_t scrollval = 10;
 
@@ -347,6 +340,13 @@
 ===============
 */
 void InteractiveBase::draw_overlay(RenderTarget& dst) {
+	// Timing
+	uint32_t curframe = SDL_GetTicks();
+
+	frametime_ = curframe - lastframe_;
+	avg_usframetime_ = ((avg_usframetime_  * 15) + (frametime_  * 1000)) / 16;
+	lastframe_ = curframe;
+
 
 	const Map & map = egbase().map();
 	const bool is_game = dynamic_cast<const Game*>(&egbase());