widelands-dev team mailing list archive

[SirVer <SirVer@xxxxxx>]

SirVer has proposed merging lp:~widelands-dev/widelands/ignore_too_many_z_valuees into lp:widelands.

Commit message:
Limit zoom to avoid having too many objects on the screen as of not running out of z-values and do not crash when we run out of z-layers.

Requested reviews:
  Widelands Developers (widelands-dev)
Related bugs:
  Bug #1658593 in widelands: "Click on minimap zooming on the Nile can crash the game"
  https://bugs.launchpad.net/widelands/+bug/1658593

For more details, see:
https://code.launchpad.net/~widelands-dev/widelands/ignore_too_many_z_valuees/+merge/316498
-- 
Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/ignore_too_many_z_valuees into lp:widelands.

=== modified file 'src/graphic/render_queue.cc'
--- src/graphic/render_queue.cc	2017-01-25 18:55:59 +0000
+++ src/graphic/render_queue.cc	2017-02-06 20:01:36 +0000
@@ -187,9 +187,13 @@
 }
 
 void RenderQueue::draw(const int screen_width, const int screen_height) {
-	if (next_z_ >= kMaximumZValue) {
-		throw wexception("Too many drawn layers. Ran out of z-values.");
-	}
+	// TODO(sirver): If next_z >= kMaximumZValue here, we ran out of z-layers to
+	// correctly order the drawing of our objects (see
+	// https://bugs.launchpad.net/widelands/+bug/1658593). This is non-critical,
+	// but will look strange. We used to crash here in this case, but since it
+	// can happen on large zoom and huge screen resolution (> 3440 x 1400), we
+	// do not crash anymore. The linked bug contains a discussion how to fix the
+	// issue properly, but it was too much work to address at the time.
 
 	Gl::State::instance().bind_framebuffer(0, 0);
 	glViewport(0, 0, screen_width, screen_height);

=== modified file 'src/wui/mapview.cc'
--- src/wui/mapview.cc	2017-01-25 18:55:59 +0000
+++ src/wui/mapview.cc	2017-02-06 20:01:36 +0000
@@ -41,8 +41,10 @@
 // it also means more computational work to plan the animation.
 constexpr int kNumKeyFrames = 102;
 
-// The maximum zoom to use in moving animations.
-constexpr float kMaxAnimationZoom = 8.f;
+// Somewhat arbitrarily we limit the zoom to a reasonable value. This is for
+// performance and to avoid numeric glitches with more extreme values. This
+// value is used for automatic movements and for user controled zoom.
+constexpr float kMaxZoom = 4.f;
 
 // The time used for panning automated map movement only.
 constexpr float kShortAnimationMs = 500.f;
@@ -175,8 +177,8 @@
 
 	if (jumping_animation) {
 		// We jump higher if the distance is farther - but we never zoom in (i.e.
-		// negative jump) or jump higher than 'kMaxAnimationZoom'.
-		const float target_zoom = math::clamp(num_screens + start.zoom, end.zoom, kMaxAnimationZoom);
+		// negative jump) or jump higher than 'kMaxZoom'.
+		const float target_zoom = math::clamp(num_screens + start.zoom, end.zoom, kMaxZoom);
 		do_plan_map_transition(
 		   start_time, duration_ms, center_point_t,
 		   DoubleSmoothstepInterpolator<float>(start.zoom, target_zoom, end.zoom, duration_ms), width,
@@ -524,9 +526,6 @@
 void MapView::zoom_around(float new_zoom,
                           const Vector2f& panel_pixel,
                           const Transition& transition) {
-	// Somewhat arbitrarily we limit the zoom to a reasonable value. This is for
-	// performance and to avoid numeric glitches with more extreme values.
-	constexpr float kMaxZoom = 4.f;
 	new_zoom = math::clamp(new_zoom, 1.f / kMaxZoom, kMaxZoom);
 
 	const TimestampedView current = animation_target_view();