Merge lp:~gunchleoc/widelands/bug-1818494-ingame-zoom-freezes into lp:widelands

[GunChleoc]

Skip calculating changes to the map view's viewport when nothing would change. This prevents the map view from being busy when the user leans on the CTRL+/- keys.

I think this should go into Build 20, because it can make the player think that Widelands has crashed, while it's just the mapview being busy.

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 4610. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/508054518.
Appveyor build 4397. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_gunchleoc_widelands_bug_1818494_ingame_zoom_freezes-4397.

[GunChleoc (gunchleoc)]

Just bumping this review. I'd like this branch in Build 20 to avoid getting bug reports when Widelands seems to freeze.

[Benedikt Straub (nordfriese)]

Code LGTM, works as expected.
Thanks for fixing this nasty bug :)

review: Approve

[GunChleoc (gunchleoc)]

Thanks for the review :)

@bunnybot merge

=== modified file 'src/wui/mapview.cc'
--- src/wui/mapview.cc	2019-02-23 11:00:49 +0000
+++ src/wui/mapview.cc	2019-03-20 12:59:24 +0000
@@ -387,6 +387,10 @@
 	const Transition transition = animate_map_panning_ ? passed_transition : Transition::Jump;
 	switch (transition) {
 	case Transition::Jump: {
+		if (view_ == target_view) {
+			// We're already there
+			return;
+		}
 		view_ = target_view;
 		MapviewPixelFunctions::normalize_pix(map_, &view_.viewpoint);
 		changeview();
@@ -394,6 +398,10 @@
 	}
 
 	case Transition::Smooth: {
+		if (!view_plans_.empty() && view_plans_.back().back().view == target_view) {
+			// We're already there
+			return;
+		}
 		const TimestampedView current = animation_target_view();
 		const auto plan =
 		   plan_map_transition(current.t, map_, current.view, target_view, get_w(), get_h());
@@ -509,6 +517,10 @@
 	const TimestampedView current = animation_target_view();
 	switch (transition) {
 	case Transition::Jump: {
+		if (view_.zoom == new_zoom) {
+			// We're already there
+			return;
+		}
 		// Zoom around the current mouse position. See
 		// http://stackoverflow.com/questions/2916081/zoom-in-on-a-point-using-scale-and-translate
 		// for a good explanation of this math.
@@ -518,6 +530,10 @@
 	}
 
 	case Transition::Smooth: {
+		if (!view_plans_.empty() && view_plans_.back().back().view.zoom == new_zoom) {
+			// We're already there
+			return;
+		}
 		const int w = get_w();
 		const int h = get_h();
 		const auto plan = plan_zoom_transition(

=== modified file 'src/wui/mapview.h'
--- src/wui/mapview.h	2019-02-23 11:00:49 +0000
+++ src/wui/mapview.h	2019-03-20 12:59:24 +0000
@@ -75,6 +75,10 @@
 		View() : View(Vector2f::zero(), 1.0f) {
 		}
 
+		bool operator==(const View& other) const {
+			return (zoom == other.zoom) && (viewpoint == other.viewpoint);
+		}
+
 		// Mappixel of top-left pixel of this MapView.
 		Vector2f viewpoint;