Codechange: improve performance for complex vehicle chains by resolving sprites less often

2 files changed, 74 insertions, 4 deletions

diff --git a/src/vehicle.cpp b/src/vehicle.cpp
index e014944a9..8dead6cee 100644
--- a/src/vehicle.cpp
+++ b/src/vehicle.cpp
@@ -1091,6 +1091,23 @@ 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.
+	 *
+	 * I'm not keen on casting to mutable - it's the approach JGR uses in JGRPP but I
+	 * wonder if there's a cleaner option (even though we can only take the decision
+	 * whether to update once we already know the vehicle is going to appear in a
+	 * viewport)
+	 */
+	if (v->rstate.sprite_has_viewport_changes) {
+		Vehicle* v_mutable = const_cast<Vehicle*>(v);
+		VehicleSpriteSeq seq;
+		v_mutable->GetImage(v_mutable->direction, EIT_ON_MAP, &seq);
+		v_mutable->sprite_seq = seq;
+		v_mutable->rstate.sprite_has_viewport_changes = false;
+	}
+
 	StartSpriteCombine();
 	for (uint i = 0; i < v->sprite_seq.count; ++i) {
 		PaletteID pal2 = v->sprite_seq.seq[i].pal;
@@ -1139,6 +1156,7 @@ void ViewportAddVehicles(DrawPixelInfo *dpi)
 			const Vehicle *v = _vehicle_viewport_hash[x + y]; // already masked & 0xFFF
 
 			while (v != nullptr) {
+
 				if (!(v->vehstatus & VS_HIDDEN) &&
 						l <= v->coord.right &&
 						t <= v->coord.bottom &&
@@ -1146,6 +1164,23 @@ void ViewportAddVehicles(DrawPixelInfo *dpi)
 						b >= v->coord.top) {
 					DoDrawVehicle(v);
 				}
+				else {
+					/*
+					 * Indicate that this vehicle was considered for rendering in a viewport,
+					 * 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.
+					 *
+					 * TODO: is there a cleaner solution than casting to a mutable type?
+					 */
+					Vehicle* v_mutable = const_cast<Vehicle*>(v);
+					v_mutable->rstate.is_viewport_candidate = true;
+				}
+
 				v = v->hash_viewport_next;
 			}
 
diff --git a/src/vehicle_base.h b/src/vehicle_base.h
index 019c94a29..8ef316df1 100644
--- a/src/vehicle_base.h
+++ b/src/vehicle_base.h
@@ -124,6 +124,13 @@ struct VehicleCache {
 	byte cached_vis_effect;  ///< Visual effect to show (see #VisualEffect)
 };
 
+/** Values for controlling how a vehicle's sprites are refreshed */
+struct VehicleSpriteRefreshState {
+	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
+};
+
 /** Sprite sequence for a vehicle part. */
 struct VehicleSpriteSeq {
 	PalSpriteID seq[4];
@@ -327,6 +334,8 @@ public:
 	NewGRFCache grf_cache;              ///< Cache of often used calculated NewGRF values
 	VehicleCache vcache;                ///< Cache of often used vehicle values.
 
+	VehicleSpriteRefreshState rstate;   ///< Values relating to whether sprites should be refreshed, see #VehicleSpriteRefreshState
+
 	Vehicle(VehicleType type = VEH_INVALID);
 
 	void PreDestructor();
@@ -1169,16 +1178,42 @@ struct SpecializedVehicle : public Vehicle {
 	 */
 	inline void UpdateViewport(bool force_update, bool update_delta)
 	{
+		bool sprite_has_changed = false;
+
 		/* Skip updating sprites on dedicated servers without screen */
 		if (_network_dedicated) return;
 
 		/* Explicitly choose method to call to prevent vtable dereference -
 		 * it gives ~3% runtime improvements in games with many vehicles */
 		if (update_delta) ((T *)this)->T::UpdateDeltaXY();
-		VehicleSpriteSeq seq;
-		((T *)this)->T::GetImage(this->direction, EIT_ON_MAP, &seq);
-		if (force_update || this->sprite_seq != seq) {
-			this->sprite_seq = seq;
+
+		/*
+		 * Only check for a new sprite sequence if the vehicle direction
+		 * has changed since we last checked it, assuming that otherwise
+		 * there won't be enough change in bounding box or offsets to need
+		 * to resolve a new sprite.
+		 */
+		if (this->direction != this->rstate.last_direction || this->rstate.is_viewport_candidate) {
+			VehicleSpriteSeq seq;
+
+			((T*)this)->T::GetImage(this->direction, EIT_ON_MAP, &seq);
+			if (this->sprite_seq != seq) {
+				sprite_has_changed = true;
+				this->sprite_seq = seq;
+			}
+
+			this->rstate.last_direction = this->direction;
+			this->rstate.is_viewport_candidate = false;
+			this->rstate.sprite_has_viewport_changes = false;
+		} else {
+			/*
+			 * Changes could still be relevant when we render the vehicle even if
+			 * they don't alter the bounding box
+			 */
+			this->rstate.sprite_has_viewport_changes = true;
+		}
+
+		if (force_update || sprite_has_changed) {
 			this->Vehicle::UpdateViewport(true);
 		}
 	}