From b11ef2d3306ddecba0f55576922dd27801e10e03 Mon Sep 17 00:00:00 2001
From: bones_was_here <bones_was_here@xonotic.au>
Date: Fri, 21 Jul 2023 18:57:27 +1000
Subject: [PATCH] items: only draw the removal effect when appropriate

Sends an extra update that triggers the effect prior to removal.
Currently only used for loot items when picked up or touching void.

Fixes #2849
without reverting to the old way of sending the effect in SVQC which
caused every player to receive it even if they couldn't see the item,
and without the expense of additional culling.

Changes the pipeline hash because g_powerups_drop_ondeath is enabled by
default and bots remove an item from their goals immediately if it's
deleted, but if it still exists (for 1-2 frames in this case) they only
remove it if it's in their PVS.
---
 .gitlab-ci.yml              |  2 +-
 qcsrc/client/items/items.qc | 15 +++++++++++----
 qcsrc/common/items/item.qh  |  1 +
 qcsrc/server/items/items.qc | 15 +++++++++++++--
 4 files changed, 26 insertions(+), 7 deletions(-)

diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index 48fa0d93f..c6adaed4d 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -75,7 +75,7 @@ test_sv_game:
     - wget -nv -O data/maps/stormkeep.waypoints https://gitlab.com/xonotic/xonotic-maps.pk3dir/raw/master/maps/stormkeep.waypoints
     - wget -nv -O data/maps/stormkeep.waypoints.cache https://gitlab.com/xonotic/xonotic-maps.pk3dir/raw/master/maps/stormkeep.waypoints.cache
 
-    - EXPECT=9fb6b4ba8c0f8b04995c123e3801b32d
+    - EXPECT=f68507df6290e8a8bd8b2a383913184f
     - HASH=$(${ENGINE} +exec serverbench.cfg
       | tee /dev/stderr
       | grep '^:'
diff --git a/qcsrc/client/items/items.qc b/qcsrc/client/items/items.qc
index 755f0f9cd..1b03ba8fc 100644
--- a/qcsrc/client/items/items.qc
+++ b/qcsrc/client/items/items.qc
@@ -215,9 +215,6 @@ void ItemDraw(entity this)
 
 void ItemRemove(entity this)
 {
-	if(this.alpha)
-	if(!this.wait || time < this.wait - ticrate) // despawning loot items have their own effects
-		pointparticles(EFFECT_ITEM_PICKUP, (this.absmin + this.absmax) * 0.5, '0 0 0', 1);
 	strfree(this.mdl);
 }
 
@@ -290,6 +287,7 @@ NET_HANDLE(ENT_CLIENT_ITEM, bool isnew)
 			IL_PUSH(g_drawables, this);
 			this.draw = ItemDraw;
 			this.flags |= FL_ITEM;
+			this.entremove = ItemRemove;
 		}
 
 		this.fade_end = ReadShort();
@@ -325,7 +323,16 @@ NET_HANDLE(ENT_CLIENT_ITEM, bool isnew)
 		SET_ONGROUND(this); // extra overkill
 	}
 
-	this.entremove = ItemRemove;
+	if(sf & ISF_REMOVEFX && !(sf & ISF_SIZE) && !(sf & ISF_MODEL)) // TODO !isnew isn't reliable for this... are we double sending initialisations?
+	{
+		// no longer available to pick up, about to be removed
+		if (this.drawmask) // this.alpha > 0
+			pointparticles(EFFECT_ITEM_PICKUP, (this.absmin + this.absmax) * 0.5, '0 0 0', 1);
+		// removing now causes CSQC_Ent_Remove() to spam
+		this.drawmask = 0;
+		IL_REMOVE(g_drawables, this);
+		this.solid = SOLID_NOT;
+	}
 
 	return true;
 }
diff --git a/qcsrc/common/items/item.qh b/qcsrc/common/items/item.qh
index 038566784..4beeda849 100644
--- a/qcsrc/common/items/item.qh
+++ b/qcsrc/common/items/item.qh
@@ -38,6 +38,7 @@ const int IT_SPEED						= BIT(13);
 const int IT_PICKUPMASK                	= IT_UNLIMITED_AMMO | IT_UNLIMITED_SUPERWEAPONS | IT_JETPACK | IT_FUEL_REGEN; // strength and invincible are handled separately
 
 // item networking
+const int ISF_REMOVEFX          = BIT(0); // technically unnecessary (after the kludge in Item_Think() is reverted), but cheaper and cleaner than using ITS_AVAILABLE
 const int ISF_LOCATION          = BIT(1);
 const int ISF_MODEL             = BIT(2);
 const int ISF_STATUS            = BIT(3);
diff --git a/qcsrc/server/items/items.qc b/qcsrc/server/items/items.qc
index e0013b5ce..829418497 100644
--- a/qcsrc/server/items/items.qc
+++ b/qcsrc/server/items/items.qc
@@ -679,6 +679,7 @@ void Item_Touch(entity this, entity toucher)
 	{
 		if (ITEM_TOUCH_NEEDKILL())
 		{
+			this.SendFlags |= ISF_REMOVEFX;
 			RemoveItem(this);
 			return;
 		}
@@ -742,7 +743,8 @@ LABEL(pickup)
 
 	if (ITEM_IS_LOOT(this))
 	{
-		delete(this);
+		this.SendFlags |= ISF_REMOVEFX;
+		RemoveItem(this);
 		return;
 	}
 	if (!this.spawnshieldtime)
@@ -847,7 +849,16 @@ void RemoveItem(entity this)
 	if(wasfreed(this) || !this) { return; }
 	if(this.waypointsprite_attached)
 		WaypointSprite_Kill(this.waypointsprite_attached);
-	delete(this);
+
+	if (this.SendFlags & ISF_REMOVEFX)
+	{
+		// delay removal until ISF_REMOVEFX has been sent
+		setthink(this, RemoveItem);
+		this.nextthink = time + 2 * autocvar_sys_ticrate; // micro optimisation: next frame will be too soon
+		this.solid = SOLID_NOT; // untouchable
+	}
+	else
+		delete(this);
 }
 
 // pickup evaluation functions
-- 
2.39.2