Fix eeb88e8: Trains reversed while paused do not correctly update sprite bounds (#8540)

6 files changed, 105 insertions, 68 deletions

diff --git a/src/saveload/vehicle_sl.cpp b/src/saveload/vehicle_sl.cpp
index 6715eb97d..94bc37fb8 100644
--- a/src/saveload/vehicle_sl.cpp
+++ b/src/saveload/vehicle_sl.cpp
@@ -470,6 +470,7 @@ void AfterLoadVehicles(bool part_of_load)
 
 		v->UpdateDeltaXY();
 		v->coord.left = INVALID_COORD;
+		v->sprite_cache.old_coord.left = INVALID_COORD;
 		v->UpdatePosition();
 		v->UpdateViewport(false);
 	}
diff --git a/src/ship_cmd.cpp b/src/ship_cmd.cpp
index e839941cf..825321e10 100644
--- a/src/ship_cmd.cpp
+++ b/src/ship_cmd.cpp
@@ -646,6 +646,8 @@ static void ShipController(Ship *v)
 		if ((v->tick_counter & 7) == 0) {
 			DirDiff diff = DirDifference(v->direction, v->rotation);
 			v->rotation = ChangeDir(v->rotation, diff > DIRDIFF_REVERSE ? DIRDIFF_45LEFT : DIRDIFF_45RIGHT);
+			/* Invalidate the sprite cache direction to force recalculation of viewport */
+			v->sprite_cache.last_direction = INVALID_DIR;
 			v->UpdateViewport(true, true);
 		}
 		return;
diff --git a/src/vehicle.cpp b/src/vehicle.cpp
index 17e843255..21d02f181 100644
--- a/src/vehicle.cpp
+++ b/src/vehicle.cpp
@@ -346,6 +346,7 @@ Vehicle::Vehicle(VehicleType type)
 {
 	this->type               = type;
 	this->coord.left         = INVALID_COORD;
+	this->sprite_cache.old_coord.left = INVALID_COORD;
 	this->group_id           = DEFAULT_GROUP;
 	this->fill_percent_te_id = INVALID_TE_ID;
 	this->first              = this;
@@ -643,11 +644,9 @@ static void UpdateVehicleTileHash(Vehicle *v, bool remove)
 
 static Vehicle *_vehicle_viewport_hash[1 << (GEN_HASHX_BITS + GEN_HASHY_BITS)];
 
-static void UpdateVehicleViewportHash(Vehicle *v, int x, int y)
+static void UpdateVehicleViewportHash(Vehicle *v, int x, int y, int old_x, int old_y)
 {
 	Vehicle **old_hash, **new_hash;
-	int old_x = v->coord.left;
-	int old_y = v->coord.top;
 
 	new_hash = (x == INVALID_COORD) ? nullptr : &_vehicle_viewport_hash[GEN_HASH(x, y)];
 	old_hash = (old_x == INVALID_COORD) ? nullptr : &_vehicle_viewport_hash[GEN_HASH(old_x, old_y)];
@@ -881,7 +880,7 @@ Vehicle::~Vehicle()
 	delete v;
 
 	UpdateVehicleTileHash(this, true);
-	UpdateVehicleViewportHash(this, INVALID_COORD, 0);
+	UpdateVehicleViewportHash(this, INVALID_COORD, 0, this->sprite_cache.old_coord.left, this->sprite_cache.old_coord.top);
 	DeleteVehicleNews(this->index, INVALID_STRING_ID);
 	DeleteNewGRFInspectWindow(GetGrfSpecFeature(this->type), this->index);
 }
@@ -1091,17 +1090,6 @@ static void DoDrawVehicle(const Vehicle *v)
 		if (to != TO_INVALID && (IsTransparencySet(to) || IsInvisibilitySet(to))) return;
 	}
 
-	/*
-	 * If the vehicle sprite was not updated despite further viewport changes, we need
-	 * to update it before drawing.
-	 */
-	if (v->sprite_cache.sprite_has_viewport_changes) {
-		VehicleSpriteSeq seq;
-		v->GetImage(v->direction, EIT_ON_MAP, &seq);
-		v->sprite_cache.sprite_seq = seq;
-		v->sprite_cache.sprite_has_viewport_changes = false;
-	}
-
 	StartSpriteCombine();
 	for (uint i = 0; i < v->sprite_cache.sprite_seq.count; ++i) {
 		PaletteID pal2 = v->sprite_cache.sprite_seq.seq[i].pal;
@@ -1156,30 +1144,42 @@ void ViewportAddVehicles(DrawPixelInfo *dpi)
 			while (v != nullptr) {
 
 				if (!(v->vehstatus & VS_HIDDEN) &&
-						l <= v->coord.right &&
-						t <= v->coord.bottom &&
-						r >= v->coord.left &&
-						b >= v->coord.top) {
-					DoDrawVehicle(v);
-				}
-				else if (l <= v->coord.right + xb &&
+					l <= v->coord.right + xb &&
 					t <= v->coord.bottom + yb &&
 					r >= v->coord.left - xb &&
 					b >= v->coord.top - yb)
 				{
 					/*
-					 * Indicate that this vehicle was considered for rendering in a viewport,
-					 * is within the bounds where a sprite could be valid for rendering
-					 * and we therefore need to update sprites more frequently in case a callback
-					 * will change the bounding box to one which will cause the sprite to be
-					 * displayed.
-					 *
-					 * This reduces the chances of flicker when sprites enter the screen, if they
-					 * are part of a newgrf vehicle set which changes bounding boxes within a
-					 * single vehicle direction.
+					 * This vehicle can potentially be drawn as part of this viewport and
+					 * needs to be revalidated, as the sprite may not be correct.
 					 */
-					v->sprite_cache.is_viewport_candidate = true;
-				}
+					if (v->sprite_cache.revalidate_before_draw) {
+						VehicleSpriteSeq seq;
+						v->GetImage(v->direction, EIT_ON_MAP, &seq);
+
+						if (seq.IsValid() && v->sprite_cache.sprite_seq != seq) {
+							v->sprite_cache.sprite_seq = seq;
+							/*
+							 * A sprite change may also result in a bounding box change,
+							 * so we need to update the bounding box again before we
+							 * check to see if the vehicle should be drawn. Note that
+							 * we can't interfere with the viewport hash at this point,
+							 * so we keep the original hash on the assumption there will
+							 * not be a significant change in the top and left coordinates
+							 * of the vehicle.
+							 */
+							v->UpdateBoundingBoxCoordinates(false);
+
+						}
+
+						v->sprite_cache.revalidate_before_draw = false;
+					}
+
+					if (l <= v->coord.right &&
+						t <= v->coord.bottom &&
+						r >= v->coord.left &&
+						b >= v->coord.top) DoDrawVehicle(v);
+					}
 
 				v = v->hash_viewport_next;
 			}
@@ -1598,11 +1598,10 @@ void Vehicle::UpdatePosition()
 }
 
 /**
- * Update the vehicle on the viewport, updating the right hash and setting the
- *  new coordinates.
- * @param dirty Mark the (new and old) coordinates of the vehicle as dirty.
- */
-void Vehicle::UpdateViewport(bool dirty)
+ * Update the bounding box co-ordinates of the vehicle
+ * @param update_cache Update the cached values for previous co-ordinate values
+*/
+void Vehicle::UpdateBoundingBoxCoordinates(bool update_cache) const
 {
 	Rect new_coord;
 	this->sprite_cache.sprite_seq.GetBounds(&new_coord);
@@ -1613,20 +1612,44 @@ void Vehicle::UpdateViewport(bool dirty)
 	new_coord.right  += pt.x + 2 * ZOOM_LVL_BASE;
 	new_coord.bottom += pt.y + 2 * ZOOM_LVL_BASE;
 
-	UpdateVehicleViewportHash(this, new_coord.left, new_coord.top);
+	if (update_cache) {
+		/*
+		 * If the old coordinates are invalid, set the cache to the new coordinates for correct
+		 * behaviour the next time the coordinate cache is checked.
+		 */
+		this->sprite_cache.old_coord = this->coord.left == INVALID_COORD ? new_coord : this->coord;
+	}
+	else {
+		/* Extend the bounds of the existing cached bounding box so the next dirty window is correct */
+		this->sprite_cache.old_coord.left   = std::min(this->sprite_cache.old_coord.left,   this->coord.left);
+		this->sprite_cache.old_coord.top    = std::min(this->sprite_cache.old_coord.top,    this->coord.top);
+		this->sprite_cache.old_coord.right  = std::max(this->sprite_cache.old_coord.right,  this->coord.right);
+		this->sprite_cache.old_coord.bottom = std::max(this->sprite_cache.old_coord.bottom, this->coord.bottom);
+	}
 
-	Rect old_coord = this->coord;
 	this->coord = new_coord;
+}
+
+/**
+ * Update the vehicle on the viewport, updating the right hash and setting the new coordinates.
+ * @param dirty Mark the (new and old) coordinates of the vehicle as dirty.
+ */
+void Vehicle::UpdateViewport(bool dirty)
+{
+	Rect old_coord = this->sprite_cache.old_coord;
+
+	this->UpdateBoundingBoxCoordinates(true);
+	UpdateVehicleViewportHash(this, this->coord.left, this->coord.top, old_coord.left, old_coord.top);
 
 	if (dirty) {
 		if (old_coord.left == INVALID_COORD) {
-			this->MarkAllViewportsDirty();
+			this->sprite_cache.is_viewport_candidate = this->MarkAllViewportsDirty();
 		} else {
-			::MarkAllViewportsDirty(
-					std::min(old_coord.left,   this->coord.left),
-					std::min(old_coord.top,    this->coord.top),
-					std::max(old_coord.right,  this->coord.right),
-					std::max(old_coord.bottom, this->coord.bottom));
+			this->sprite_cache.is_viewport_candidate = ::MarkAllViewportsDirty(
+				std::min(this->sprite_cache.old_coord.left, this->coord.left),
+				std::min(this->sprite_cache.old_coord.top, this->coord.top),
+				std::max(this->sprite_cache.old_coord.right, this->coord.right),
+				std::max(this->sprite_cache.old_coord.bottom, this->coord.bottom));
 		}
 	}
 }
@@ -1642,10 +1665,11 @@ void Vehicle::UpdatePositionAndViewport()
 
 /**
  * Marks viewports dirty where the vehicle's image is.
+ * @return true if at least one viewport has a dirty block
  */
-void Vehicle::MarkAllViewportsDirty() const
+bool Vehicle::MarkAllViewportsDirty() const
 {
-	::MarkAllViewportsDirty(this->coord.left, this->coord.top, this->coord.right, this->coord.bottom);
+	return ::MarkAllViewportsDirty(this->coord.left, this->coord.top, this->coord.right, this->coord.bottom);
 }
 
 /**
diff --git a/src/vehicle_base.h b/src/vehicle_base.h
index 24facfb3e..f80faf1e3 100644
--- a/src/vehicle_base.h
+++ b/src/vehicle_base.h
@@ -186,10 +186,11 @@ struct VehicleSpriteSeq {
  * or calculating the viewport.
  */
 struct MutableSpriteCache {
-	Direction last_direction;         ///< Last direction we obtained sprites for
-	bool is_viewport_candidate;       ///< The vehicle has been in the hash for a shown viewport recently
-	bool sprite_has_viewport_changes; ///< There have been viewport changes since the sprite was last updated
-	VehicleSpriteSeq sprite_seq;      ///< Vehicle appearance.
+	Direction last_direction;     ///< Last direction we obtained sprites for
+	bool revalidate_before_draw;  ///< We need to do a GetImage() and check bounds before drawing this sprite
+	Rect old_coord;               ///< Co-ordinates from the last valid bounding box
+	bool is_viewport_candidate;   ///< This vehicle can potentially be drawn on a viewport
+	VehicleSpriteSeq sprite_seq;  ///< Vehicle appearance.
 };
 
 /** A vehicle pool for a little over 1 million vehicles. */
@@ -251,7 +252,7 @@ public:
 
 	CargoPayment *cargo_payment;        ///< The cargo payment we're currently in
 
-	Rect coord;                         ///< NOSAVE: Graphical bounding box of the vehicle, i.e. what to redraw on moves.
+	mutable Rect coord;                 ///< NOSAVE: Graphical bounding box of the vehicle, i.e. what to redraw on moves.
 
 	Vehicle *hash_viewport_next;        ///< NOSAVE: Next vehicle in the visual location hash.
 	Vehicle **hash_viewport_prev;       ///< NOSAVE: Previous vehicle in the visual location hash.
@@ -768,8 +769,9 @@ public:
 
 	void UpdatePosition();
 	void UpdateViewport(bool dirty);
+	void UpdateBoundingBoxCoordinates(bool update_cache) const;
 	void UpdatePositionAndViewport();
-	void MarkAllViewportsDirty() const;
+	bool MarkAllViewportsDirty() const;
 
 	inline uint16 GetServiceInterval() const { return this->service_interval; }
 
@@ -1206,14 +1208,14 @@ struct SpecializedVehicle : public Vehicle {
 			}
 
 			this->sprite_cache.last_direction = this->direction;
-			this->sprite_cache.is_viewport_candidate = false;
-			this->sprite_cache.sprite_has_viewport_changes = false;
+			this->sprite_cache.revalidate_before_draw = false;
 		} else {
 			/*
-			 * Changes could still be relevant when we render the vehicle even if
-			 * they don't alter the bounding box
+			 * A change that could potentially invalidate the sprite has been
+			 * made, signal that we should still resolve it before drawing on a
+			 * viewport.
 			 */
-			this->sprite_cache.sprite_has_viewport_changes = true;
+			this->sprite_cache.revalidate_before_draw = true;
 		}
 
 		if (force_update || sprite_has_changed) {
diff --git a/src/viewport.cpp b/src/viewport.cpp
index 8fd6f373f..44ff37c74 100644
--- a/src/viewport.cpp
+++ b/src/viewport.cpp
@@ -179,7 +179,7 @@ struct ViewportDrawer {
 	Point foundation_offset[FOUNDATION_PART_END];    ///< Pixel offset for ground sprites on the foundations.
 };
 
-static void MarkViewportDirty(const Viewport *vp, int left, int top, int right, int bottom);
+static bool MarkViewportDirty(const Viewport *vp, int left, int top, int right, int bottom);
 
 static ViewportDrawer _vd;
 
@@ -1903,27 +1903,28 @@ void UpdateViewportPosition(Window *w)
  * @param top    Top edge of area to repaint
  * @param right  Right edge of area to repaint
  * @param bottom Bottom edge of area to repaint
+ * @return true if the viewport contains a dirty block
  * @ingroup dirty
  */
-static void MarkViewportDirty(const Viewport *vp, int left, int top, int right, int bottom)
+static bool MarkViewportDirty(const Viewport *vp, int left, int top, int right, int bottom)
 {
 	/* Rounding wrt. zoom-out level */
 	right  += (1 << vp->zoom) - 1;
 	bottom += (1 << vp->zoom) - 1;
 
 	right -= vp->virtual_left;
-	if (right <= 0) return;
+	if (right <= 0) return false;
 
 	bottom -= vp->virtual_top;
-	if (bottom <= 0) return;
+	if (bottom <= 0) return false;
 
 	left = std::max(0, left - vp->virtual_left);
 
-	if (left >= vp->virtual_width) return;
+	if (left >= vp->virtual_width) return false;
 
 	top = std::max(0, top - vp->virtual_top);
 
-	if (top >= vp->virtual_height) return;
+	if (top >= vp->virtual_height) return false;
 
 	AddDirtyBlock(
 		UnScaleByZoomLower(left, vp->zoom) + vp->left,
@@ -1931,6 +1932,8 @@ static void MarkViewportDirty(const Viewport *vp, int left, int top, int right,
 		UnScaleByZoom(right, vp->zoom) + vp->left + 1,
 		UnScaleByZoom(bottom, vp->zoom) + vp->top + 1
 	);
+
+	return true;
 }
 
 /**
@@ -1939,18 +1942,23 @@ static void MarkViewportDirty(const Viewport *vp, int left, int top, int right,
  * @param top    Top    edge of area to repaint. (viewport coordinates, that is wrt. #ZOOM_LVL_NORMAL)
  * @param right  Right  edge of area to repaint. (viewport coordinates, that is wrt. #ZOOM_LVL_NORMAL)
  * @param bottom Bottom edge of area to repaint. (viewport coordinates, that is wrt. #ZOOM_LVL_NORMAL)
+ * @return true if at least one viewport has a dirty block
  * @ingroup dirty
  */
-void MarkAllViewportsDirty(int left, int top, int right, int bottom)
+bool MarkAllViewportsDirty(int left, int top, int right, int bottom)
 {
+	bool dirty = false;
+
 	Window *w;
 	FOR_ALL_WINDOWS_FROM_BACK(w) {
 		Viewport *vp = w->viewport;
 		if (vp != nullptr) {
 			assert(vp->width != 0);
-			MarkViewportDirty(vp, left, top, right, bottom);
+			if (MarkViewportDirty(vp, left, top, right, bottom)) dirty = true;
 		}
 	}
+
+	return dirty;
 }
 
 void ConstrainAllViewportsZoom()
diff --git a/src/viewport_func.h b/src/viewport_func.h
index 466a24a3b..9461f3df5 100644
--- a/src/viewport_func.h
+++ b/src/viewport_func.h
@@ -27,7 +27,7 @@ Point TranslateXYToTileCoord(const Viewport *vp, int x, int y, bool clamp_to_map
 Point GetTileBelowCursor();
 void UpdateViewportPosition(Window *w);
 
-void MarkAllViewportsDirty(int left, int top, int right, int bottom);
+bool MarkAllViewportsDirty(int left, int top, int right, int bottom);
 
 bool DoZoomInOutWindow(ZoomStateChange how, Window *w);
 void ZoomInOrOutToCursorWindow(bool in, Window * w);