diff --git a/code/_onclick/item_attack.dm b/code/_onclick/item_attack.dm index 7dd43c5d0d8..1427ba83299 100644 --- a/code/_onclick/item_attack.dm +++ b/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) /** diff --git a/code/modules/vehicle/janivehicle.dm b/code/modules/vehicle/janivehicle.dm index a116fad9de3..34ced584b80 100644 --- a/code/modules/vehicle/janivehicle.dm +++ b/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 = "There is already one of those in [src]." - 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, "You hook [attacking] onto [src].") - attacking.forceMove(src) - mybag = attacking + return ITEM_INTERACT_BLOCKING + to_chat(user, "You hook [used] onto [src].") + 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,"You upgrade [src] with [attacking].") + qdel(used) + to_chat(user,"You upgrade [src] with [used].") 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) diff --git a/code/modules/vehicle/vehicle.dm b/code/modules/vehicle/vehicle.dm index 4b0c49da3c1..3cfdb02ac5d 100644 --- a/code/modules/vehicle/vehicle.dm +++ b/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, "You insert [attacking] into [src].") + used.forceMove(src) + to_chat(user, "You insert [used] into [src].") 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, "[attacking] seems to be stuck to your hand!") - return FINISH_ATTACK - if(istype(attacking, /obj/item/borg/upgrade/vtec) && install_vtec(attacking, user)) - return FINISH_ATTACK + to_chat(user, "[used] seems to be stuck to your hand!") + 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)) diff --git a/docs/references/attack_chain.md b/docs/references/attack_chain.md index 5a159c2a00d..894810b607b 100644 --- a/docs/references/attack_chain.md +++ b/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, "[I] seems to be stuck to your hand!") - return -- if(istype(I, /obj/item/borg/upgrade/vtec) && vehicle_move_delay > 1) -- vehicle_move_delay = 1 -- qdel(I) -- to_chat(user, "You upgrade [src] with [I].") +-/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 = "There is already one of those in [src]." + + 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, "You hook [I] onto [src].") + 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,"You upgrade [src] with [I].") + 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, "You upgrade [src] with [vtec].") ++/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