Fix negotiaton between attack chains causing issues with borgs (#27571)

* Attack chain: borg interactions and janicart migration

* redundant check

code/_onclick/item_attack.dm

@@ -35,11 +35,14 @@
 	// At this point it means the attack was "successful", or at least
 	// handled, in some way. This can mean nothing happened, this can mean the
 	// target took damage, etc.
-	if(target.new_attack_chain)
-		target.attacked_by(src, user)
-		after_attack(target, user, proximity_flag, params)
-	else
-		afterattack__legacy__attackchain(target, user, proximity_flag, params)
+
+	// TODO: `target` here should probably be another `!QDELETED` check.
+	// Preserved for backwards compatibility, may be fixed post-migration.
+	if(target && !QDELETED(src))
+		if(new_attack_chain)
+			after_attack(target, user, proximity_flag, params)
+		else
+			afterattack__legacy__attackchain(target, user, proximity_flag, params)
 
 /// Called when the item is in the active hand, and clicked; alternately, there
 /// is an 'activate held object' verb or you can hit pagedown.
@@ -101,7 +104,7 @@
 		return FINISH_ATTACK
 
 	if(!can_be_hit)
-		return FINISH_ATTACK
+		return
 
 	return attacking.attack_obj(src, user, params)
 
@@ -178,7 +181,9 @@
 	user.changeNext_move(CLICK_CD_MELEE)
 	user.do_attack_animation(attacked_obj)
 
-	if(!attacked_obj.new_attack_chain)
+	if(attacked_obj.new_attack_chain)
+		attacked_obj.attacked_by(src, user)
+	else
 		attacked_obj.attacked_by__legacy__attackchain(src, user)
 
 /**

code/modules/vehicle/janivehicle.dm

@@ -90,36 +90,36 @@
 	if(buffer_installed)
 		. += "It has been upgraded with a floor buffer."
 
-/obj/vehicle/janicart/attack_by(obj/item/attacking, mob/user, params)
-	if(..())
-		return FINISH_ATTACK
-
+/obj/vehicle/janicart/item_interaction(mob/living/user, obj/item/used, list/modifiers)
 	var/fail_msg = "<span class='notice'>There is already one of those in [src].</span>"
 
-	if(istype(attacking, /obj/item/storage/bag/trash))
+	if(istype(used, /obj/item/storage/bag/trash))
 		if(mybag)
 			to_chat(user, fail_msg)
-			return FINISH_ATTACK
+			return ITEM_INTERACT_BLOCKING
 		if(!user.drop_item())
-			return FINISH_ATTACK
-		to_chat(user, "<span class='notice'>You hook [attacking] onto [src].</span>")
-		attacking.forceMove(src)
-		mybag = attacking
+			return ITEM_INTERACT_BLOCKING
+		to_chat(user, "<span class='notice'>You hook [used] onto [src].</span>")
+		used.forceMove(src)
+		mybag = used
 		update_icon(UPDATE_OVERLAYS)
-		return FINISH_ATTACK
-	if(istype(attacking, /obj/item/borg/upgrade/floorbuffer))
+		return ITEM_INTERACT_SUCCESS
+
+	if(istype(used, /obj/item/borg/upgrade/floorbuffer))
 		if(buffer_installed)
 			to_chat(user, fail_msg)
-			return FINISH_ATTACK
+			return ITEM_INTERACT_BLOCKING
 		buffer_installed = TRUE
-		qdel(attacking)
-		to_chat(user,"<span class='notice'>You upgrade [src] with [attacking].</span>")
+		qdel(used)
+		to_chat(user,"<span class='notice'>You upgrade [src] with [used].</span>")
 		update_icon(UPDATE_OVERLAYS)
-		return FINISH_ATTACK
-	if(mybag && user.a_intent == INTENT_HELP && !is_key(attacking))
-		mybag.attackby__legacy__attackchain(attacking, user)
-	else
-		return FINISH_ATTACK
+		return ITEM_INTERACT_SUCCESS
+
+	if(mybag && user.a_intent == INTENT_HELP && !is_key(used))
+		mybag.attackby__legacy__attackchain(used, user)
+		return ITEM_INTERACT_ANY_BLOCKER
+
+	return ..()
 
 /obj/vehicle/janicart/install_vtec(obj/item/borg/upgrade/vtec/vtec, mob/user)
 	if(..() && floorbuffer)

code/modules/vehicle/vehicle.dm

@@ -68,22 +68,20 @@
 
 		return TRUE
 
-/obj/vehicle/attack_by(obj/item/attacking, mob/user, params)
-	if(..())
-		return FINISH_ATTACK
-
-	if(key_type && !is_key(inserted_key) && is_key(attacking))
+/obj/vehicle/item_interaction(mob/living/user, obj/item/used, list/modifiers)
+	if(key_type && !is_key(inserted_key) && is_key(used))
 		if(user.drop_item())
-			attacking.forceMove(src)
-			to_chat(user, "<span class='notice'>You insert [attacking] into [src].</span>")
+			used.forceMove(src)
+			to_chat(user, "<span class='notice'>You insert [used] into [src].</span>")
 			if(inserted_key)	//just in case there's an invalid key
 				inserted_key.forceMove(drop_location())
-			inserted_key = attacking
+			inserted_key = used
 		else
-			to_chat(user, "<span class='warning'>[attacking] seems to be stuck to your hand!</span>")
-		return FINISH_ATTACK
-	if(istype(attacking, /obj/item/borg/upgrade/vtec) && install_vtec(attacking, user))
-		return FINISH_ATTACK
+			to_chat(user, "<span class='warning'>[used] seems to be stuck to your hand!</span>")
+		return ITEM_INTERACT_ANY_BLOCKER
+
+	if(istype(used, /obj/item/borg/upgrade/vtec) && install_vtec(used, user))
+		return ITEM_INTERACT_ANY_BLOCKER
 
 /obj/vehicle/AltClick(mob/user)
 	if(inserted_key && user.Adjacent(user))

docs/references/attack_chain.md

@@ -155,6 +155,11 @@ have already been performed.
 used on it. These can be fairly straightforward if the type tree is shallow and
 the number of interactions is small.
 
+Something to note is that `attackby`, despite its name, rarely has behavior that
+is designed to respond to combat attacks. Most `attackby` methods you will find
+are simple item interactions; specific behavior the objects want to intercept
+before allowing the attack phase to begin.
+
 Our example migration is `/obj/vehicle`. This type tree only requires migrating:
 
 - `/obj/vehicle/proc/attackby__legacy__attackchain`
@@ -230,45 +235,78 @@ return control to the parent for installing the VTEC. If there's a bag and the
 user is clicking on it with anything else with help intent, attempt to put it in
 the bag. Otherwise, return control to the parent.
 
-Most of this logic will work just fine as is, with the exception that we will
-need to return `FINISH_ATTACK` in the appropriate places when we want the attack
-chain to stop.
+Most of this logic will work just fine as is. However, none of this is
+combat-related, so we should pull it out of `attack_by` and substitute in
+`item_interaction`. This ensures that all the code involving specific behavior
+when clicking on the janicart will run before the attack phase, and not get
+in its way.
 
-We also need to refactor the code regarding VTEC installation: because one
-subtype does something different in reaction to the installation, we will pull
-that into its own proc, rather than try to rely on a back-and-forth between
-parent and child `attack_by`.
+Note that while `item_interaction` does not require a parent call, in this case
+it is useful to us because we want to handle the janicart-specific interactions
+before handling the vehicle-specific interactions.
 
-At the end of `/obj/vehicle/attack_by`:
+We change all the return statements to return one of the `ITEM_INTERACT_` flags
+at each junction whenever we have handled the item interaction.
 
 ```diff
-						to_chat(user, "<span class='warning'>[I] seems to be stuck to your hand!</span>")
-				return
--		if(istype(I, /obj/item/borg/upgrade/vtec) && vehicle_move_delay > 1)
--			vehicle_move_delay = 1
--			qdel(I)
--			to_chat(user, "<span class='notice'>You upgrade [src] with [I].</span>")
+-/obj/vehicle/janicart/attackby(obj/item/I, mob/user, params)
++/obj/vehicle/janicart/item_interaction(mob/living/user, obj/item/I, list/modifiers)
+ 	var/fail_msg = "<span class='notice'>There is already one of those in [src].</span>"
+
+ 	if(istype(I, /obj/item/storage/bag/trash))
+ 		if(mybag)
+ 			to_chat(user, fail_msg)
 -			return
--		return ..()
-+		if(istype(I, /obj/item/borg/upgrade/vtec) && install_vtec(I, user))
-+			return FINISH_ATTACK
++			return ITEM_INTERACT_BLOCKING
+ 		if(!user.drop_item())
+-			return
++			return ITEM_INTERACT_BLOCKING
+ 		to_chat(user, "<span class='notice'>You hook [I] onto [src].</span>")
+ 		I.forceMove(src)
+ 		mybag = I
+ 		update_icon(UPDATE_OVERLAYS)
+-		return
++		return ITEM_INTERACT_SUCCESS
++
+ 	if(istype(I, /obj/item/borg/upgrade/floorbuffer))
+ 		if(buffer_installed)
+ 			to_chat(user, fail_msg)
+-			return
++			return ITEM_INTERACT_BLOCKING
+ 		buffer_installed = TRUE
+ 		qdel(I)
+ 		to_chat(user,"<span class='notice'>You upgrade [src] with [I].</span>")
+ 		update_icon(UPDATE_OVERLAYS)
+-		return
+-	if(istype(I, /obj/item/borg/upgrade/vtec) && floorbuffer)
++		return ITEM_INTERACT_SUCCESS
++
++	if(mybag && user.a_intent == INTENT_HELP && !is_key(I))
++		mybag.attackby__legacy__attackchain(I, user)
++		return ITEM_INTERACT_ANY_BLOCKER
++
++	return ..()
 ```
 
-And then:
+We also refactor the code regarding VTEC installation: because one subtype does
+something different in reaction to the installation, we will pull that into its
+own proc, so that the parent interaction can handle that behavior.
 
 ```diff
-+/obj/vehicle/proc/install_vtec(obj/item/borg/upgrade/vtec/vtec, mob/user)
-+	if(vehicle_move_delay > 1)
-+		vehicle_move_delay = 1
-+		qdel(vtec)
-+		to_chat(user, "<span class='notice'>You upgrade [src] with [vtec].</span>")
++/obj/vehicle/janicart/install_vtec(obj/item/borg/upgrade/vtec/vtec, mob/user)
++	if(..() && floorbuffer)
+ 		floorbuffer = FALSE
+ 		vehicle_move_delay -= buffer_delay
+-		return ..() //VTEC installation is handled in parent attackby, so we're returning to it early
+-	if(mybag && user.a_intent == INTENT_HELP && !is_key(I))
+-		mybag.attackby(I, user)
+-	else
+-		return ..()
 +
-+		return TRUE
++	return TRUE
 ```
 
-Now, since we know the parent `attack_by` will get called first, we don't need
-to check for an attempted VTEC installation in the janicart at all. We just need
-to react to it properly:
+This allows us to keep the VTEC-specific behavior separate.
 
 ```diff
 +/obj/vehicle/janicart/install_vtec(obj/item/borg/upgrade/vtec/vtec, mob/user)
@@ -279,20 +317,22 @@ to react to it properly:
 +	return TRUE
 ```
 
-That is: if the VTEC installation was successful, we remove the floorbuffer and
+That is: if the VTEC installation was successful, we disable the floorbuffer and
 its delay. We want to return `TRUE` at the end no matter what, because this is
 the indication not that the VTEC installation was succesful, but that it was
 attempted, and thus the rest of the attack chain is not necessary.
 
-We make sure to include:
+We'll take the opportunity to rename the passed in argument from `I` to `used`
+to make the code clearer, as well:
 
 ```diff
-+	if(..())
-+		return FINISH_ATTACK
+-/obj/vehicle/janicart/item_interaction(mob/living/user, obj/item/I, list/modifiers)
++/obj/vehicle/janicart/item_interaction(mob/living/user, obj/item/used, list/modifiers)
+
+// etc...
 ```
 
-At the top of each `attack_by` proc, and set `new_attack_chain = TRUE` on
-`/obj/vehicle.`
+Finally, set `new_attack_chain = TRUE` on `/obj/vehicle.`
 
 > [!NOTE]
 >
@@ -613,8 +653,8 @@ chain from running.
 For `attack_by` prevention, this proc is [/datum/proc/signal_cancel_attack_by][]. For
 `activate_self` prevention, this proc is [/datum/proc/signal_cancel_activate_self][].
 
-[/datum/proc/signal/cancel_attack_by]: https://codedocs.paradisestation.org/datum.html#proc/signal_cancel_attack_by
-[/datum/proc/signal/cancel_activate_self]: https://codedocs.paradisestation.org/datum.html#proc/signal_cancel_activate_self
+[/datum/proc/signal_cancel_attack_by]: https://codedocs.paradisestation.org/datum.html#proc/signal_cancel_attack_by
+[/datum/proc/signal_cancel_activate_self]: https://codedocs.paradisestation.org/datum.html#proc/signal_cancel_activate_self
 
 For example, when we migrated the airlock electronics above, we neglected to
 handle the `/destroyed` subtype, which prevents any interaction via