Turns out a bunch of components do not properly transfer because of (#42691)

one of three things.

    1. They don't use RegisterWithParent or UnregisterFromParent to unregister
       and register signals

    2. They use callbacks which refer to a source object, which is usually deleted
       on transfer, or lost in some manner, or simply makes no sense at all to be
       transferred

    3. the component was never designed to be transferred at all

TransferComponents gave no shits about any of this and just blindly transferred
all components, if they were actually capable of it or not.

I only noticed this because it was causing chairs to break as they would not register signals
and verbs correctly for rotation after being picked up and then placed down, and a player
reported that issue via ahelp.

Luckily we caught it before the rot got anywhere, only chairs and the shuttle subystem
tend to use this proc (Shuttle uses it on turfs), can you imagine if everything was using
this LMAO

Which is good because it's more dangerous than a loaded gun

I have added a can_transfer var, that is true when a component is valid to
actually transfer, which means the dev has actually thought about what happens when
you take the parent object away and swap it for another and all the crazy that is entailed
by this

I have done my best to audit what components are actually
transferable, but things are basically a hot mess (Thanks @Cyberboss )

The following components required edits:
Forensics:
did not register/deregister the clean_act signal properly, did not checkblood on new parent

Rotation:
did not use RegisterWithParent or UnregisterFromParent, turned out
to not be transferable anyway due to having callbacks that can be
passed in to the parent with unknown sources that we can't feasibly
reuse (i.e if you're transferred from a chair to a bed, your old rotation
call backs are no longer valid). Turns out the use case it was for (just chairs)
didn't need it anyway, so I just made it non valid for transfer.

Wet Floor:
Honestly this one is just a hot mess, it should be a subtype of the slippery
component with the extra wet turf handling.

As it is it basically manages a slippery component on top of it's own extra
functionality, so that's a major code smell.

I added registration/unregistration of the signals, and made it's pretransfer
remove the slippery component and the posttransfer add it back (via update_flags)

Components that seem transferable without issues
mirage_border
orbiter
decal
spill
storage (I hope to earth)
This commit is contained in:
oranges
2019-03-06 08:27:29 +13:00
committed by yogstation13-bot
parent d0edb981cd
commit 35a56b92fa
11 changed files with 79 additions and 22 deletions

View File

@@ -7,6 +7,7 @@
#define GET_COMPONENT(varname, path) GET_COMPONENT_FROM(varname, path, src)
#define COMPONENT_INCOMPATIBLE 1
#define COMPONENT_NOTRANSFER 2
// How multiple components of the exact same type are handled in the same datum

View File

@@ -2,6 +2,10 @@
var/dupe_mode = COMPONENT_DUPE_HIGHLANDER
var/dupe_type
var/datum/parent
//only set to true if you are able to properly transfer this component
//At a minimum RegisterWithParent and UnregisterFromParent should be used
//Make sure you also implement PostTransfer for any post transfer handling
var/can_transfer = FALSE
/datum/component/New(datum/P, ...)
parent = P
@@ -154,7 +158,7 @@
return
/datum/component/proc/PostTransfer()
return
return COMPONENT_INCOMPATIBLE //Do not support transfer by default as you must properly support it
/datum/component/proc/_GetInverseTypeList(our_type = type)
//we can do this one simple trick
@@ -281,10 +285,13 @@
if(target.parent)
target.RemoveComponent()
target.parent = src
if(target.PostTransfer() == COMPONENT_INCOMPATIBLE)
var/c_type = target.type
qdel(target)
CRASH("Incompatible [c_type] transfer attempt to a [type]!")
var/result = target.PostTransfer()
switch(result)
if(COMPONENT_INCOMPATIBLE)
var/c_type = target.type
qdel(target)
CRASH("Incompatible [c_type] transfer attempt to a [type]!")
if(target == AddComponent(target))
target._JoinParent()
@@ -294,10 +301,13 @@
return
var/comps = dc[/datum/component]
if(islist(comps))
for(var/I in comps)
target.TakeComponent(I)
for(var/datum/component/I in comps)
if(I.can_transfer)
target.TakeComponent(I)
else
target.TakeComponent(comps)
var/datum/component/C = comps
if(C.can_transfer)
target.TakeComponent(comps)
/datum/component/ui_host()
return parent

View File

@@ -1,6 +1,6 @@
/datum/component/decal
dupe_mode = COMPONENT_DUPE_ALLOWED
can_transfer = TRUE
var/cleanable
var/description
var/mutable_appearance/pic

View File

@@ -1,5 +1,6 @@
/datum/component/forensics
dupe_mode = COMPONENT_DUPE_UNIQUE
can_transfer = TRUE
var/list/fingerprints //assoc print = print
var/list/hiddenprints //assoc ckey = realname/gloves/ckey
var/list/blood_DNA //assoc dna = bloodtype
@@ -21,8 +22,18 @@
blood_DNA = new_blood_DNA
fibers = new_fibers
check_blood()
/datum/component/forensics/RegisterWithParent()
check_blood()
RegisterSignal(parent, COMSIG_COMPONENT_CLEAN_ACT, .proc/clean_act)
/datum/component/forensics/UnregisterFromParent()
UnregisterSignal(parent, list(COMSIG_COMPONENT_CLEAN_ACT))
/datum/component/forensics/PostTransfer()
if(!isatom(parent))
return COMPONENT_INCOMPATIBLE
/datum/component/forensics/proc/wipe_fingerprints()
fingerprints = null
return TRUE

View File

@@ -237,6 +237,3 @@
LOCKON_RANGING_BREAK_CHECK
cd++
CHECK_TICK
/datum/component/lockon_aiming/PostTransfer(datum/new_parent)
return COMPONENT_INCOMPATIBLE

View File

@@ -1,4 +1,5 @@
/datum/component/mirage_border
can_transfer = TRUE
var/obj/effect/abstract/mirage_holder/holder
/datum/component/mirage_border/Initialize(turf/target, direction, range=world.view)

View File

@@ -1,4 +1,5 @@
/datum/component/orbiter
can_transfer = TRUE
dupe_mode = COMPONENT_DUPE_UNIQUE_PASSARGS
var/list/orbiters
var/datum/callback/orbiter_spy

View File

@@ -44,15 +44,17 @@
if(src.rotation_flags & ROTATION_CLOCKWISE)
default_rotation_direction = ROTATION_CLOCKWISE
if(src.rotation_flags & ROTATION_ALTCLICK)
/datum/component/simple_rotation/proc/add_signals()
if(rotation_flags & ROTATION_ALTCLICK)
RegisterSignal(parent, COMSIG_CLICK_ALT, .proc/HandRot)
RegisterSignal(parent, COMSIG_PARENT_EXAMINE, .proc/ExamineMessage)
if(src.rotation_flags & ROTATION_WRENCH)
if(rotation_flags & ROTATION_WRENCH)
RegisterSignal(parent, COMSIG_PARENT_ATTACKBY, .proc/WrenchRot)
if(src.rotation_flags & ROTATION_VERBS)
/datum/component/simple_rotation/proc/add_verbs()
if(rotation_flags & ROTATION_VERBS)
var/atom/movable/AM = parent
if(src.rotation_flags & ROTATION_FLIP)
if(rotation_flags & ROTATION_FLIP)
AM.verbs += /atom/movable/proc/simple_rotate_flip
if(src.rotation_flags & ROTATION_CLOCKWISE)
AM.verbs += /atom/movable/proc/simple_rotate_clockwise
@@ -66,11 +68,30 @@
AM.verbs -= /atom/movable/proc/simple_rotate_clockwise
AM.verbs -= /atom/movable/proc/simple_rotate_counterclockwise
/datum/component/simple_rotation/Destroy()
/datum/component/simple_rotation/proc/remove_signals()
UnregisterSignal(parent, list(COMSIG_CLICK_ALT, COMSIG_PARENT_EXAMINE, COMSIG_PARENT_ATTACKBY))
/datum/component/simple_rotation/RegisterWithParent()
add_verbs()
add_signals()
. = ..()
/datum/component/simple_rotation/PostTransfer()
//Because of the callbacks which we don't track cleanly we can't transfer this
//item cleanly, better to let the new of the new item create a new rotation datum
//instead (there's no real state worth transferring)
return COMPONENT_NOTRANSFER
/datum/component/simple_rotation/UnregisterFromParent()
remove_verbs()
remove_signals()
. = ..()
/datum/component/simple_rotation/Destroy()
QDEL_NULL(can_user_rotate)
QDEL_NULL(can_be_rotated)
QDEL_NULL(after_rotation)
//Signals + verbs removed via UnRegister
. = ..()
/datum/component/simple_rotation/RemoveComponent()

View File

@@ -2,6 +2,7 @@
// Yes this exists purely for the spaghetti meme
/datum/component/spill
can_transfer = TRUE
var/preexisting_item_flags
var/list/droptext

View File

@@ -4,6 +4,7 @@
// /mob/living/Move() in /modules/mob/living/living.dm - hiding storage boxes on mob movement
/datum/component/storage/concrete
can_transfer = TRUE
var/drop_all_on_deconstruct = TRUE
var/drop_all_on_destroy = FALSE
var/transfer_contents_on_component_transfer = FALSE

View File

@@ -1,5 +1,6 @@
/datum/component/wet_floor
dupe_mode = COMPONENT_DUPE_UNIQUE_PASSARGS
can_transfer = TRUE
var/highest_strength = TURF_DRY
var/lube_flags = NONE //why do we have this?
var/list/time_left_list //In deciseconds.
@@ -26,14 +27,19 @@
if(!isopenturf(parent))
return COMPONENT_INCOMPATIBLE
add_wet(strength, duration_minimum, duration_add, duration_maximum)
RegisterSignal(parent, COMSIG_TURF_IS_WET, .proc/is_wet)
RegisterSignal(parent, COMSIG_TURF_MAKE_DRY, .proc/dry)
permanent = _permanent
if(!permanent)
START_PROCESSING(SSwet_floors, src)
addtimer(CALLBACK(src, .proc/gc, TRUE), 1) //GC after initialization.
last_process = world.time
/datum/component/wet_floor/RegisterWithParent()
RegisterSignal(parent, COMSIG_TURF_IS_WET, .proc/is_wet)
RegisterSignal(parent, COMSIG_TURF_MAKE_DRY, .proc/dry)
/datum/component/wet_floor/UnregisterFromParent()
UnregisterSignal(parent, list(COMSIG_TURF_IS_WET, COMSIG_TURF_MAKE_DRY))
/datum/component/wet_floor/Destroy()
STOP_PROCESSING(SSwet_floors, src)
var/turf/T = parent
@@ -136,12 +142,19 @@
/datum/component/wet_floor/PreTransfer()
var/turf/O = parent
O.cut_overlay(current_overlay)
//That turf is no longer slippery, we're out of here
//Slippery components don't transfer due to callbacks
qdel(O.GetComponent(/datum/component/slippery))
/datum/component/wet_floor/PostTransfer()
if(!isopenturf(parent))
return COMPONENT_INCOMPATIBLE
var/turf/T = parent
T.add_overlay(current_overlay)
//Make sure to add/update any slippery component on the new turf (update_flags calls LoadComponent)
update_flags()
//NB it's possible we get deleted after this, due to inherit
/datum/component/wet_floor/proc/add_wet(type, duration_minimum = 0, duration_add = 0, duration_maximum = MAXIMUM_WET_TIME, _permanent = FALSE)
var/static/list/allowed_types = list(TURF_WET_WATER, TURF_WET_LUBE, TURF_WET_ICE, TURF_WET_PERMAFROST)