[MIRROR] Rack sounds pr turned rack code refactor #2563

Steals-The-PRs · 2024-03-25T14:53:28Z

Mirrored on Nova: NovaSector/NovaSector#1622
Original PR: tgstation/tgstation#81973

About The Pull Request

Alternate title: "Label Hoarding PR"

You ever just want to add sounds to a thing and then end up just refactoring half of the damn thing?
Yeah.
Anyhow.

So in rough chronological order!

Putting items in racks actually plays the sounds

Picking things up from racks plays their pickup sound, but putting them on them doesn't. Just dropping it or putting it on tables does make the right sounds. This seems to be because of a silent parameter in transferItemToLoc that's set by tables but not racks.

(/code/game/objects/structures/tables_racks.dm, line 273)
if(user.transferItemToLoc(I, drop_location(), silent = FALSE))

(/code/game/objects/structures/tables_racks.dm, line 867)
if(user.transferItemToLoc(W, drop_location()))

Adding this makes it work just fine.

(/code/game/objects/structures/tables_racks.dm, line 867)
if(user.transferItemToLoc(W, drop_location(), silent = FALSE))

Attackby single letter parameters, 1 instead of TRUE

Then, I noticed attackby just returns 1 to mean true after calling transferItemToLoc, when we just have the more readable TRUE.
Similarly it uses a single letter parameter W, which on its own is already unreadable, but is also mismatched with the parent proc using attacking_item.

(/code/game/objects/structures/tables_racks.dm, line 859-868)
/obj/structure/rack/attackby(obj/item/W, mob/living/user, params)
	var/list/modifiers = params2list(params)
	if (W.tool_behaviour == TOOL_WRENCH && !(obj_flags & NO_DECONSTRUCTION) && LAZYACCESS(modifiers, RIGHT_CLICK))
		W.play_tool_sound(src)
		deconstruct(TRUE)
		return
	if(user.combat_mode)
		return ..()
	if(user.transferItemToLoc(W, drop_location()))
		return 1

(/code/_onclick/item_attack.dm, line 133-136)
/atom/proc/attackby(obj/item/attacking_item, mob/user, params)
	if(SEND_SIGNAL(src, COMSIG_ATOM_ATTACKBY, attacking_item, user, params) & COMPONENT_NO_AFTERATTACK)
		return TRUE
	return FALSE

So we change it 1 to TRUE and W to attacking_item to make it more readable!

Don't let them try to put Abstract items

While looking over the table code, though, I also noticed it has a check for the ABSTRACT item flag when placing items. Having tested this, we add this too for consistency and to avoid awkward situations down the line.

(/code/game/objects/structures/tables_racks.dm, line 272-273)
if(!user.combat_mode && !(I.item_flags & ABSTRACT))
	if(user.transferItemToLoc(I, drop_location(), silent = FALSE))

(/code/game/objects/structures/tables_racks.dm, line 859-868)
/obj/structure/rack/attackby(obj/item/attacking_item mob/living/user, params)
	var/list/modifiers = params2list(params)
	if (attacking_item.tool_behaviour == TOOL_WRENCH && !(obj_flags & NO_DECONSTRUCTION) && LAZYACCESS(modifiers, RIGHT_CLICK))
		attacking_item.play_tool_sound(src)
		deconstruct(TRUE)
		return
	if(user.combat_mode || attacking_item.item_flags & ABSTRACT)
		return ..()
	if(user.transferItemToLoc(W, drop_location()))
		return TRUE

Split off rack structure attackby wrenching into wrench_act procs

But that's still kind of bad! But wait, we have procs for wrenching actions, so it should really be in there.
So we move this to its own proc.

(/code/game/objects/structures/tables_racks.dm, line 850-855)
/obj/structure/rack/wrench_act_secondary(mob/living/user, obj/item/tool)
	if(obj_flags & NO_DECONSTRUCTION)
		return FALSE
	tool.play_tool_sound(src)
	deconstruct(TRUE)
	return ITEM_INTERACT_SUCCESS

(/code/game/objects/structures/tables_racks.dm, line 857-861)
/obj/structure/rack/attackby(obj/item/attacking_item mob/living/user, params)
	if(user.combat_mode || attacking_item.item_flags & ABSTRACT)
		return ..()
	if(user.transferItemToLoc(W, drop_location()))
		return TRUE

Split off rack item attackby wrenching into wrench_act procs, include sounds

But the item can also be deconstructed, and sure enough, it does the same thing.

(/code/game/objects/structures/tables_racks.dm, line 920-925)
/obj/item/rack_parts/attackby(obj/item/W, mob/user, params)
	if (W.tool_behaviour == TOOL_WRENCH)
		new /obj/item/stack/sheet/iron(user.loc)
		qdel(src)
	else
		. = ..()

So we give this the same treatment, and include a deconstruct method rather than just having it be separate. We also play the tool sound for consistency with deconstructing the rack structure.

(/code/game/objects/structures/tables_racks.dm, line 948-953)
/obj/item/rack_parts/wrench_act(mob/living/user, obj/item/tool)
	if(obj_flags & NO_DECONSTRUCTION)
		return FALSE
	tool.play_tool_sound(src)
	deconstruct(TRUE)
	return ITEM_INTERACT_SUCCESS

(/code/game/objects/structures/tables_racks.dm, line 955-958)
/obj/item/rack_parts/deconstruct(disassembled = TRUE)
	if(!(obj_flags & NO_DECONSTRUCTION))
		new /obj/item/stack/sheet/iron(drop_location())
	return ..()

Note: this makes it so it only deconstructs rack items on left click. I think that's perfectly fine.

Ancient code removal

Now we get to the fun part! Ancient code.
When removing the single letter parameters from attackby previously, I thought I might as well remove other such instances while we're at it.
This gets us to MouseDrop_T.

(/code/game/objects/structures/tables_racks.dm, line 850-857)
/obj/structure/rack/MouseDrop_T(obj/O, mob/user)
	. = ..()
	if ((!( isitem(O) ) || user.get_active_held_item() != O))
		return
	if(!user.dropItemToGround(O))
		return
	if(O.loc != src.loc)
		step(O, get_dir(O, src))

What the fuck?
Right so, this lets us click-drag-drop an item onto the rack, but only for our active item. And it just drops it and steps it in the right direction.
So we just, we just kill it. We just kill it.
You can just click on the rack to do functionally the exact same thing, so we just kill it.
It's blocking us from dumping storage item contents, so just.
We Just Kill It.
We Just Fucking Kill It.

Combat mode kicking

Anyhow! With that out of the way, we move to the finishing touches: usage context and kicking!
While writing up context I was reminded that currently clicking on a rack with an empty hand would just, kick it, regardless of combat mode.

(/code/game/objects/structures/tables_racks.dm, line 873-882)
/obj/structure/rack/attack_hand(mob/living/user, list/modifiers)
	. = ..()
	if(.)
		return
	if(user.body_position == LYING_DOWN || user.usable_legs < 2)
		return
	user.changeNext_move(CLICK_CD_MELEE)
	user.do_attack_animation(src, ATTACK_EFFECT_KICK)
	user.visible_message(span_danger("[user] kicks [src]."), null, null, COMBAT_MESSAGE_RANGE)
	take_damage(rand(4,8), BRUTE, MELEE, 1)

This is awkward, misclick a few times and you've kicked it back into item form.
So we make it only kick it while in combat mode to avoid this.

(/code/game/objects/structures/tables_racks.dm, line 880-889)
/obj/structure/rack/attack_hand(mob/living/user, list/modifiers)
	(...)
	if(!user.combat_mode || user.body_position == LYING_DOWN || user.usable_legs < 2)
		return
	(...)

Usage context

Then finally! Usage context!
This part's easy, just copying over the logic from tables and making it show deconstruct context for the rack structure and item and construction context for the item.

(/code/game/objects/structures/tables_racks.dm, line 840-851)
/obj/structure/rack/add_context(atom/source, list/context, obj/item/held_item, mob/living/user)
	. = ..()

	if(isnull(held_item))
		return NONE

	if(!(obj_flags & NO_DECONSTRUCTION))
		if(held_item.tool_behaviour == TOOL_WRENCH)
			context[SCREENTIP_CONTEXT_RMB] = "Deconstruct"
			. = CONTEXTUAL_SCREENTIP_SET

	return . || NONE

(/code/game/objects/structures/tables_racks.dm, line 931-946)
/obj/item/rack_parts/add_context(atom/source, list/context, obj/item/held_item, mob/living/user)
	. = ..()

	if(isnull(held_item))
		return NONE

	if(held_item == src)
		context[SCREENTIP_CONTEXT_LMB] = "Construct Rack"
		. = CONTEXTUAL_SCREENTIP_SET

	if(!(obj_flags & NO_DECONSTRUCTION))
		if(held_item.tool_behaviour == TOOL_WRENCH)
			context[SCREENTIP_CONTEXT_LMB] = "Deconstruct"
			. = CONTEXTUAL_SCREENTIP_SET

	return . || NONE

And then don't forget to register said context.

(/code/game/objects/structures/tables_racks.dm, line 834-838)
/obj/structure/rack/Initialize(mapload)
	. = ..()
	AddElement(/datum/element/climbable)
	AddElement(/datum/element/elevation, pixel_shift = 12)
	register_context()

(/code/game/objects/structures/tables_racks.dm, line 927-929)
/obj/item/rack_parts/Initialize(mapload)
	. = ..()
	register_context()

And that's all!

Now please remind me not to be this comprehensive again.
Not for something that's less than thirty lines, at least.
Thanks.

Why It's Good For The Game

Doing this in chronological order.
First off, it's wonky that putting say a toolbox on a table or the ground makes sound but putting it in a rack is perfectly silent, especially when taking it from the rack isn't. This makes it consistent with tables.

Returning TRUE is more readable than returning 1. Single letter parameters are awful for readability, especially when inconsistent with the parent parameters. This resolves those.

It shouldn't let you attempt to place items tagged with ABSTRACT, as you shouldn't be able to place those. This makes it consistent with tables.

We have procs for wrench actions, better to use those than implementing your own wrench checks in attackby. This makes it do so.

It's just nice to have a deconstruction sound, so this makes deconstructing the item also play the sound of the used tool.

The ancient rack code we're removing is entirely just a more awkward way of doing what we can already do without it, it only let you drag your active item onto them, but you can just click. It was awkwardly implemented, and blocked anything from doing its own click-drag onto surface thing like say storage dumping contents.

Usage context is just nice to have. This adds context for deconstructing to the rack structure and for deconstructing and constructing to the rack item.

Changelog

🆑 00-Steven
refactor: Touched most of the code for racks. It should function almost entirely the same save for what's noted here, please report any issues.
code: Wrenching moved to wrenching procs. Side-effect, rack items are only deconstructed on left-click
sound: Items that have sounds make them when placed on racks, much like when placed on tables.
sound: Rack items now make a sound upon deconstructing them.
fix: Racks no longer let you attempt to place abstract items like the slap hand or water tank spray nozzles on them.
qol: Clicking on a rack no longer kicks the shit out of it if you don't actually have combat mode on.
qol: Racks and rack items have hover tooltip usage context.
del: Killed ancient rack code for dragging your active item onto a rack. Just click it does the same thing. This allows you to actually dump items onto racks, though currently disorderly.
/:cl:

* Rack sounds pr turned rack code refactor * Fix CI, Remove old old edit That edit is no longer needed --------- Co-authored-by: _0Steven <[email protected]> Co-authored-by: SomeRandomOwl <[email protected]>

* Cargo ripley starts at full charge. (#83287) ## About The Pull Request Cargo ripley starts at 100% charge instead of 25% charge. ## Why It's Good For The Game Because charging a cell actually requires a significant amount of energy now, the roundstart ripley starting almost uncharged causes quite a significant roundstart load since it's next to the recharger. This makes it start fully charged so the roundstart power rush is less extreme. ## Changelog :cl: balance: Cargo ripley's cell starts fully charged. /:cl: * Cargo ripley starts at full charge. --------- Co-authored-by: Pickle-Coding <[email protected]> Co-authored-by: NovaBot13 <[email protected]>

[MIRROR] Rack sounds pr turned rack code refactor (#1622)

d4adfb6

* Rack sounds pr turned rack code refactor * Fix CI, Remove old old edit That edit is no longer needed --------- Co-authored-by: _0Steven <[email protected]> Co-authored-by: SomeRandomOwl <[email protected]>

Steals-The-PRs added the TG Mirror label Mar 25, 2024

Iajret merged commit 76d85a5 into master Mar 25, 2024
24 checks passed

Iajret deleted the upstream-mirror-1622 branch March 25, 2024 20:40

AnywayFarus added a commit that referenced this pull request Mar 25, 2024

Automatic changelog for PR #2563 [ci skip]

3cb898f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MIRROR] Rack sounds pr turned rack code refactor #2563

[MIRROR] Rack sounds pr turned rack code refactor #2563

Steals-The-PRs commented Mar 25, 2024

[MIRROR] Rack sounds pr turned rack code refactor #2563

[MIRROR] Rack sounds pr turned rack code refactor #2563

Conversation

Steals-The-PRs commented Mar 25, 2024

Mirrored on Nova: NovaSector/NovaSector#1622 Original PR: tgstation/tgstation#81973

About The Pull Request

Alternate title: "Label Hoarding PR"

Putting items in racks actually plays the sounds

Attackby single letter parameters, 1 instead of TRUE

Don't let them try to put Abstract items

Split off rack structure attackby wrenching into wrench_act procs

Split off rack item attackby wrenching into wrench_act procs, include sounds

Ancient code removal

Combat mode kicking

Usage context

Why It's Good For The Game

Changelog

Mirrored on Nova: NovaSector/NovaSector#1622
Original PR: tgstation/tgstation#81973