Labor camp machines - Fixes & Code improvement (#68584)

About The Pull Request

Visible changes (all the fixes):

    Labor point checkers now says the prisoner ID's stats rather than being a to_chat, it also displays their info much better.
    Labor stacker no longer duplicates your points at no cost by attacking the machine with materials, instead it processes it just like it would when sucking materials up.
    Labor stackers additionally now update their input/outputs when they are rotated through shuttles, as it doesn't work on the current version of Lavaland (there's no issue report up on this yet though, oddly).
    Adds a check if you actually have a goal before checking if you can send the shuttle up, preventing prisoners who were sent without a point goal (permanently) from being able to go up whenever they wanted by simply clicking on the button before it greyed out.

The rest (code improvement):

    Added more early returns
    Removed single letter vars
    Renamed terribly named vars and added comments to some of them.
    Removed machinedir in favor of checking their view() when syncing machines, as I didn't find checking an entire side of the map next to an object to be very intuitive.


Lets Lavaland use labor camp machines again
Closes #67764
Just cool general fixes +1
Changelog

cl
fix: Labor camp Prisoners without a sentence can no longer send themselves back up whenever they wanted by clicking the button fast enough.
fix: Prisoners can no longer get infinite labor camp points by clicking the stacking machine with ores.
fix: All ore machines now properly change their direction when they dock a shuttle in a separate direction, fixing Lavaland's labor camp stacking machine.
qol: Labor camp's point checker now states all the information you need, rather than being a paragraph of text that just appears in chat.
/cl

code/modules/mining/laborcamp/laborstacker.dm

@@ -9,16 +9,14 @@ GLOBAL_LIST(labor_sheet_values)
 	icon_state = "console"
 	density = FALSE
 	/// Connected stacking machine
-	var/obj/machinery/mineral/stacking_machine/laborstacker/stacking_machine = null
-	/// Direction of the stacking machine
-	var/machinedir = SOUTH
+	var/obj/machinery/mineral/stacking_machine/laborstacker/stacking_machine
 	/// Needed to send messages to sec radio
-	var/obj/item/radio/Radio
+	var/obj/item/radio/security_radio
 
 /obj/machinery/mineral/labor_claim_console/Initialize(mapload)
 	. = ..()
-	Radio = new /obj/item/radio(src)
-	Radio.set_listening(FALSE)
+	security_radio = new /obj/item/radio(src)
+	security_radio.set_listening(FALSE)
 	locate_stacking_machine()
 	//If we can't find a stacking machine end it all ok?
 	if(!stacking_machine)
@@ -26,15 +24,14 @@ GLOBAL_LIST(labor_sheet_values)
 
 	if(!GLOB.labor_sheet_values)
 		var/sheet_list = list()
-		for(var/sheet_type in subtypesof(/obj/item/stack/sheet))
-			var/obj/item/stack/sheet/sheet = sheet_type
-			if(!initial(sheet.point_value) || (initial(sheet.merge_type) && initial(sheet.merge_type) != sheet_type)) //ignore no-value sheets and x/fifty subtypes
+		for(var/obj/item/stack/sheet/sheet as anything in subtypesof(/obj/item/stack/sheet))
+			if(!initial(sheet.point_value) || (initial(sheet.merge_type) && initial(sheet.merge_type) != sheet)) //ignore no-value sheets and x/fifty subtypes
 				continue
 			sheet_list += list(list("ore" = initial(sheet.name), "value" = initial(sheet.point_value)))
 		GLOB.labor_sheet_values = sort_list(sheet_list, /proc/cmp_sheet_list)
 
 /obj/machinery/mineral/labor_claim_console/Destroy()
-	QDEL_NULL(Radio)
+	QDEL_NULL(security_radio)
 	if(stacking_machine)
 		stacking_machine.labor_console = null
 		stacking_machine = null
@@ -58,22 +55,22 @@ GLOBAL_LIST(labor_sheet_values)
 	var/list/data = list()
 	var/can_go_home = FALSE
 
-	data["emagged"] = FALSE
 	if(obj_flags & EMAGGED)
-		data["emagged"] = TRUE
 		can_go_home = TRUE
-	var/obj/item/card/id/I
-	if(isliving(usr))
-		var/mob/living/L = usr
-		I = L.get_idcard(TRUE)
-	if(istype(I, /obj/item/card/id/advanced/prisoner))
-		var/obj/item/card/id/advanced/prisoner/P = I
-		data["id_points"] = P.points
-		if(P.points >= P.goal)
+	var/obj/item/card/id/worn_id
+	if(isliving(user))
+		var/mob/living/living_user = user
+		worn_id = living_user.get_idcard(TRUE)
+	if(istype(worn_id, /obj/item/card/id/advanced/prisoner))
+		var/obj/item/card/id/advanced/prisoner/worn_prisoner_id = worn_id
+		data["id_points"] = worn_prisoner_id.points
+		if(!worn_prisoner_id.goal)
+			data["status_info"] = "No goal set!"
+		else if(worn_prisoner_id.points >= worn_prisoner_id.goal)
 			can_go_home = TRUE
 			data["status_info"] = "Goal met!"
 		else
-			data["status_info"] = "You are [(P.goal - P.points)] points away."
+			data["status_info"] = "You are [(worn_prisoner_id.goal - worn_prisoner_id.points)] points away."
 	else
 		data["status_info"] = "No Prisoner ID detected."
 		data["id_points"] = 0
@@ -89,41 +86,45 @@ GLOBAL_LIST(labor_sheet_values)
 	if(.)
 		return
 
-	var/mob/M = usr
+	var/mob/user_mob = usr
+
 	switch(action)
+
 		if("claim_points")
-			var/obj/item/card/id/I
-			if(isliving(M))
-				var/mob/living/L = M
-				I = L.get_idcard(TRUE)
-			if(istype(I, /obj/item/card/id/advanced/prisoner))
-				var/obj/item/card/id/advanced/prisoner/P = I
-				P.points += stacking_machine.points
+			var/obj/item/card/id/worn_id
+			if(isliving(user_mob))
+				var/mob/living/living_mob = user_mob
+				worn_id = living_mob.get_idcard(TRUE)
+			if(istype(worn_id, /obj/item/card/id/advanced/prisoner))
+				var/obj/item/card/id/advanced/prisoner/worn_prisoner_id = worn_id
+				worn_prisoner_id.points += stacking_machine.points
 				stacking_machine.points = 0
-				to_chat(M, span_notice("Points transferred."))
+				to_chat(user_mob, span_notice("Points transferred."))
 				return TRUE
 			else
-				to_chat(M, span_alert("No valid id for point transfer detected."))
+				to_chat(user_mob, span_alert("No valid id for point transfer detected."))
+
 		if("move_shuttle")
-			if(!alone_in_area(get_area(src), M))
-				to_chat(M, span_alert("Prisoners are only allowed to be released while alone."))
+			if(!alone_in_area(get_area(src), user_mob))
+				to_chat(user_mob, span_alert("Prisoners are only allowed to be released while alone."))
 				return
+
 			switch(SSshuttle.moveShuttle("laborcamp", "laborcamp_home", TRUE))
 				if(1)
-					to_chat(M, span_alert("Shuttle not found."))
+					to_chat(user_mob, span_alert("Shuttle not found."))
 				if(2)
-					to_chat(M, span_alert("Shuttle already at station."))
+					to_chat(user_mob, span_alert("Shuttle already at station."))
 				if(3)
-					to_chat(M, span_alert("No permission to dock could be granted."))
+					to_chat(user_mob, span_alert("No permission to dock could be granted."))
 				else
 					if(!(obj_flags & EMAGGED))
-						Radio.set_frequency(FREQ_SECURITY)
-						Radio.talk_into(src, "A prisoner has returned to the station. Minerals and Prisoner ID card ready for retrieval.", FREQ_SECURITY)
-					to_chat(M, span_notice("Shuttle received message and will be sent shortly."))
+						security_radio.set_frequency(FREQ_SECURITY)
+						security_radio.talk_into(src, "A prisoner has returned to the station. Minerals and Prisoner ID card ready for retrieval.", FREQ_SECURITY)
+					to_chat(user_mob, span_notice("Shuttle received message and will be sent shortly."))
 					return TRUE
 
 /obj/machinery/mineral/labor_claim_console/proc/locate_stacking_machine()
-	stacking_machine = locate(/obj/machinery/mineral/stacking_machine, get_step(src, machinedir))
+	stacking_machine = locate(/obj/machinery/mineral/stacking_machine) in view(2, src)
 	if(stacking_machine)
 		stacking_machine.labor_console = src
 
@@ -136,9 +137,11 @@ GLOBAL_LIST(labor_sheet_values)
 
 /obj/machinery/mineral/stacking_machine/laborstacker
 	force_connect = TRUE
-	var/points = 0 //The unclaimed value of ore stacked.
-	damage_deflection = 21
-	var/obj/machinery/mineral/labor_claim_console/labor_console //This is abhorent. I know.
+	damage_deflection = 21 //otherwise prisoners will destroy it
+	///Idle points sitting in the machine left to be claimed.
+	var/points = 0
+	///Labor claim console synced to our stacking machine, set by the console.
+	var/obj/machinery/mineral/labor_claim_console/labor_console
 
 /obj/machinery/mineral/stacking_machine/laborstacker/Destroy()
 	if(labor_console)
@@ -148,12 +151,12 @@ GLOBAL_LIST(labor_sheet_values)
 
 /obj/machinery/mineral/stacking_machine/laborstacker/process_sheet(obj/item/stack/sheet/inp)
 	points += inp.point_value * inp.amount
-	..()
+	return ..()
 
-/obj/machinery/mineral/stacking_machine/laborstacker/attackby(obj/item/I, mob/living/user)
-	if(istype(I, /obj/item/stack/sheet) && user.canUnEquip(I) && !user.combat_mode)
-		var/obj/item/stack/sheet/inp = I
-		points += inp.point_value * inp.amount
+/obj/machinery/mineral/stacking_machine/laborstacker/attackby(obj/item/weapon, mob/user, params)
+	if(istype(weapon, /obj/item/stack/sheet))
+		process_sheet(weapon)
+		return
 	return ..()
 
 /**********************Point Lookup Console**************************/
@@ -171,15 +174,13 @@ GLOBAL_LIST(labor_sheet_values)
 		return
 	user.examinate(src)
 
-/obj/machinery/mineral/labor_points_checker/attackby(obj/item/I, mob/user, params)
-	if(istype(I, /obj/item/card/id))
-		if(istype(I, /obj/item/card/id/advanced/prisoner))
-			var/obj/item/card/id/advanced/prisoner/prisoner_id = I
-			to_chat(user, span_notice("<B>ID: [prisoner_id.registered_name]</B>"))
-			to_chat(user, span_notice("Points Collected:[prisoner_id.points]"))
-			to_chat(user, span_notice("Point Quota: [prisoner_id.goal]"))
-			to_chat(user, span_notice("Collect points by bringing smelted minerals to the Labor Shuttle stacking machine. Reach your quota to earn your release."))
-		else
-			to_chat(user, span_warning("Error: Invalid ID"))
-	else
+/obj/machinery/mineral/labor_points_checker/attackby(obj/item/weapon, mob/user, params)
+	if(!istype(weapon, /obj/item/card/id/advanced/prisoner))
 		return ..()
+	var/obj/item/card/id/advanced/prisoner/prisoner_id = weapon
+	if(!prisoner_id.goal) //no goal to reach
+		say("No goal required for this ID.")
+		return
+	say("ID: [prisoner_id.registered_name].")
+	say("Points Collected: [prisoner_id.points] / [prisoner_id.goal].")
+	say("Collect points by bringing smelted minerals to the Labor Shuttle stacking machine. Reach your quota to earn your release.")

code/modules/mining/machine_processing.dm

@@ -38,6 +38,11 @@
 	unregister_input_turf()
 	register_input_turf()
 
+/obj/machinery/mineral/shuttleRotate(rotation, params)
+	. = ..()
+	input_dir = angle2dir(rotation + dir2angle(input_dir))
+	output_dir = angle2dir(rotation + dir2angle(output_dir))
+
 /**
 	Base proc for all `/mineral` subtype machines to use. Place your item pickup behavior in this proc when you override it for your specific machine.
 
@@ -54,34 +59,34 @@
 	return
 
 /// Generic unloading proc. Takes an atom as an argument and forceMove's it to the turf adjacent to this machine in the `output_dir` direction.
-/obj/machinery/mineral/proc/unload_mineral(atom/movable/S)
-	S.forceMove(drop_location())
-	var/turf/T = get_step(src,output_dir)
-	if(T)
-		S.forceMove(T)
+/obj/machinery/mineral/proc/unload_mineral(atom/movable/unloaded_mineral)
+	unloaded_mineral.forceMove(drop_location())
+	var/turf/unload_turf = get_step(src, output_dir)
+	if(unload_turf)
+		unloaded_mineral.forceMove(unload_turf)
 
 /obj/machinery/mineral/processing_unit_console
 	name = "production machine console"
 	icon = 'icons/obj/machines/mining_machines.dmi'
 	icon_state = "console"
 	density = TRUE
-	var/obj/machinery/mineral/processing_unit/machine = null
-	var/machinedir = EAST
+	/// Connected ore processing machine.
+	var/obj/machinery/mineral/processing_unit/processing_machine
 
 /obj/machinery/mineral/processing_unit_console/Initialize(mapload)
 	. = ..()
-	machine = locate(/obj/machinery/mineral/processing_unit, get_step(src, machinedir))
-	if (machine)
-		machine.CONSOLE = src
+	processing_machine = locate(/obj/machinery/mineral/processing_unit) in view(2, src)
+	if (processing_machine)
+		processing_machine.mineral_machine = src
 	else
 		return INITIALIZE_HINT_QDEL
 
 /obj/machinery/mineral/processing_unit_console/ui_interact(mob/user)
 	. = ..()
-	if(!machine)
+	if(!processing_machine)
 		return
 
-	var/dat = machine.get_machine_data()
+	var/dat = processing_machine.get_machine_data()
 
 	var/datum/browser/popup = new(user, "processing", "Smelting Console", 300, 500)
 	popup.set_content(dat)
@@ -96,22 +101,22 @@
 	if(href_list["material"])
 		var/datum/material/new_material = locate(href_list["material"])
 		if(istype(new_material))
-			machine.selected_material = new_material
-			machine.selected_alloy = null
+			processing_machine.selected_material = new_material
+			processing_machine.selected_alloy = null
 
 	if(href_list["alloy"])
-		machine.selected_material = null
-		machine.selected_alloy = href_list["alloy"]
+		processing_machine.selected_material = null
+		processing_machine.selected_alloy = href_list["alloy"]
 
 	if(href_list["set_on"])
-		machine.on = (href_list["set_on"] == "on")
-		machine.begin_processing()
+		processing_machine.on = (href_list["set_on"] == "on")
+		processing_machine.begin_processing()
 
 	updateUsrDialog()
 	return
 
 /obj/machinery/mineral/processing_unit_console/Destroy()
-	machine = null
+	processing_machine = null
 	return ..()
 
 
@@ -124,10 +129,10 @@
 	icon_state = "furnace"
 	density = TRUE
 	needs_item_input = TRUE
-	var/obj/machinery/mineral/CONSOLE = null
 	var/on = FALSE
-	var/datum/material/selected_material = null
-	var/selected_alloy = null
+	var/selected_alloy
+	var/obj/machinery/mineral/mineral_machine
+	var/datum/material/selected_material
 	var/datum/techweb/stored_research
 	///Proximity monitor associated with this atom, needed for proximity checks.
 	var/datum/proximity_monitor/proximity_monitor
@@ -135,23 +140,24 @@
 /obj/machinery/mineral/processing_unit/Initialize(mapload)
 	. = ..()
 	proximity_monitor = new(src, 1)
-	var/list/allowed_materials = list(/datum/material/iron,
-									/datum/material/glass,
-									/datum/material/silver,
-									/datum/material/gold,
-									/datum/material/diamond,
-									/datum/material/plasma,
-									/datum/material/uranium,
-									/datum/material/bananium,
-									/datum/material/titanium,
-									/datum/material/bluespace
-									)
+	var/list/allowed_materials = list(
+		/datum/material/iron,
+		/datum/material/glass,
+		/datum/material/silver,
+		/datum/material/gold,
+		/datum/material/diamond,
+		/datum/material/plasma,
+		/datum/material/uranium,
+		/datum/material/bananium,
+		/datum/material/titanium,
+		/datum/material/bluespace,
+	)
 	AddComponent(/datum/component/material_container, allowed_materials, INFINITY, MATCONTAINER_EXAMINE|BREAKDOWN_FLAGS_ORE_PROCESSOR, allowed_items=/obj/item/stack)
 	stored_research = new /datum/techweb/specialized/autounlocking/smelter
 	selected_material = GET_MATERIAL_REF(/datum/material/iron)
 
 /obj/machinery/mineral/processing_unit/Destroy()
-	CONSOLE = null
+	mineral_machine = null
 	QDEL_NULL(stored_research)
 	return ..()
 
@@ -165,40 +171,40 @@
 	else
 		materials.insert_item(O, breakdown_flags=BREAKDOWN_FLAGS_ORE_PROCESSOR)
 		qdel(O)
-		if(CONSOLE)
-			CONSOLE.updateUsrDialog()
+		if(mineral_machine)
+			mineral_machine.updateUsrDialog()
 
 /obj/machinery/mineral/processing_unit/proc/get_machine_data()
 	var/dat = "<b>Smelter control console</b><br><br>"
 	var/datum/component/material_container/materials = GetComponent(/datum/component/material_container)
-	for(var/datum/material/M in materials.materials)
-		var/amount = materials.materials[M]
-		dat += "<span class=\"res_name\">[M.name]: </span>[amount] cm&sup3;"
-		if (selected_material == M)
+	for(var/datum/material/all_materials as anything in materials.materials)
+		var/amount = materials.materials[all_materials]
+		dat += "<span class=\"res_name\">[all_materials.name]: </span>[amount] cm&sup3;"
+		if (selected_material == all_materials)
 			dat += " <i>Smelting</i>"
 		else
-			dat += " <A href='?src=[REF(CONSOLE)];material=[REF(M)]'><b>Not Smelting</b></A> "
+			dat += " <A href='?src=[REF(mineral_machine)];material=[REF(all_materials)]'><b>Not Smelting</b></A> "
 		dat += "<br>"
 
 	dat += "<br><br>"
 	dat += "<b>Smelt Alloys</b><br>"
 
-	for(var/v in stored_research.researched_designs)
-		var/datum/design/D = SSresearch.techweb_design_by_id(v)
-		dat += "<span class=\"res_name\">[D.name] "
-		if (selected_alloy == D.id)
+	for(var/research in stored_research.researched_designs)
+		var/datum/design/designs = SSresearch.techweb_design_by_id(research)
+		dat += "<span class=\"res_name\">[designs.name] "
+		if (selected_alloy == designs.id)
 			dat += " <i>Smelting</i>"
 		else
-			dat += " <A href='?src=[REF(CONSOLE)];alloy=[D.id]'><b>Not Smelting</b></A> "
+			dat += " <A href='?src=[REF(mineral_machine)];alloy=[designs.id]'><b>Not Smelting</b></A> "
 		dat += "<br>"
 
 	dat += "<br><br>"
 	//On or off
 	dat += "Machine is currently "
 	if (on)
-		dat += "<A href='?src=[REF(CONSOLE)];set_on=off'>On</A> "
+		dat += "<A href='?src=[REF(mineral_machine)];set_on=off'>On</A> "
 	else
-		dat += "<A href='?src=[REF(CONSOLE)];set_on=on'>Off</A> "
+		dat += "<A href='?src=[REF(mineral_machine)];set_on=on'>Off</A> "
 
 	return dat
 
@@ -209,30 +215,31 @@
 		process_ore(target)
 
 /obj/machinery/mineral/processing_unit/process(delta_time)
-	if(on)
-		if(selected_material)
-			smelt_ore(delta_time)
-
-		else if(selected_alloy)
-			smelt_alloy(delta_time)
-
-
-		if(CONSOLE)
-			CONSOLE.updateUsrDialog()
-	else
+	if(!on)
 		end_processing()
+		if(mineral_machine)
+			mineral_machine.updateUsrDialog()
+		return
+
+	if(selected_material)
+		smelt_ore(delta_time)
+	else if(selected_alloy)
+		smelt_alloy(delta_time)
+
+	if(mineral_machine)
+		mineral_machine.updateUsrDialog()
 
 /obj/machinery/mineral/processing_unit/proc/smelt_ore(delta_time = 2)
 	var/datum/component/material_container/materials = GetComponent(/datum/component/material_container)
 	var/datum/material/mat = selected_material
-	if(mat)
-		var/sheets_to_remove = (materials.materials[mat] >= (MINERAL_MATERIAL_AMOUNT * SMELT_AMOUNT * delta_time) ) ? SMELT_AMOUNT * delta_time : round(materials.materials[mat] /  MINERAL_MATERIAL_AMOUNT)
-		if(!sheets_to_remove)
-			on = FALSE
-		else
-			var/out = get_step(src, output_dir)
-			materials.retrieve_sheets(sheets_to_remove, mat, out)
-
+	if(!mat)
+		return
+	var/sheets_to_remove = (materials.materials[mat] >= (MINERAL_MATERIAL_AMOUNT * SMELT_AMOUNT * delta_time) ) ? SMELT_AMOUNT * delta_time : round(materials.materials[mat] /  MINERAL_MATERIAL_AMOUNT)
+	if(!sheets_to_remove)
+		on = FALSE
+	else
+		var/out = get_step(src, output_dir)
+		materials.retrieve_sheets(sheets_to_remove, mat, out)
 
 /obj/machinery/mineral/processing_unit/proc/smelt_alloy(delta_time = 2)
 	var/datum/design/alloy = stored_research.isDesignResearchedID(selected_alloy) //check if it's a valid design
@@ -274,6 +281,6 @@
 /obj/machinery/mineral/processing_unit/on_deconstruction()
 	var/datum/component/material_container/materials = GetComponent(/datum/component/material_container)
 	materials.retrieve_all()
-	..()
+	return ..()
 
 #undef SMELT_AMOUNT

code/modules/mining/machine_stacking.dm

@@ -9,12 +9,10 @@
 	circuit = /obj/item/circuitboard/machine/stacking_unit_console
 	/// Connected stacking machine
 	var/obj/machinery/mineral/stacking_machine/machine
-	/// Direction for which console looks for stacking machine to connect to
-	var/machinedir = SOUTHEAST
 
 /obj/machinery/mineral/stacking_unit_console/Initialize(mapload)
 	. = ..()
-	machine = locate(/obj/machinery/mineral/stacking_machine, get_step(src, machinedir))
+	machine = locate(/obj/machinery/mineral/stacking_machine) in view(2, src)
 	if (machine)
 		machine.console = src
 
@@ -87,7 +85,7 @@
 	var/obj/machinery/mineral/stacking_unit_console/console
 	var/stk_types = list()
 	var/stk_amt = list()
-	var/stack_list[0] //Key: Type.  Value: Instance of type.
+	var/stack_list[0] //Key: Type. Value: Instance of type.
 	var/stack_amt = 50 //amount to stack before releassing
 	var/datum/component/remote_materials/materials
 	var/force_connect = FALSE
@@ -97,7 +95,7 @@
 /obj/machinery/mineral/stacking_machine/Initialize(mapload)
 	. = ..()
 	proximity_monitor = new(src, 1)
-	materials = AddComponent(/datum/component/remote_materials, "stacking", mapload, FALSE, mapload && force_connect)
+	materials = AddComponent(/datum/component/remote_materials, "stacking", mapload, FALSE, (mapload && force_connect))
 
 /obj/machinery/mineral/stacking_machine/Destroy()
 	if(console)