From 93f4a20699f66c92c42fc81c84a6811fe864149c Mon Sep 17 00:00:00 2001
From: NovaBot <154629622+NovaBot13@users.noreply.github.com>
Date: Wed, 8 May 2024 19:12:15 -0400
Subject: [PATCH] [MIRROR] Fix pride pin reskinning (#2382)
* Fix pride pin reskinning (#82920)
## About The Pull Request
**Edit: Since writing, this pr has been updated to address failing CI
based on code-general suggestions, invalidating the previous
descriptions. The previous descriptions has been included as spoilers
for posterity**
Right, so, this has gone from just a simple pride pin fix to realizing
CI fails with it to doing a more complex lasting fix based on
suggestions.
Recap time. Objects get reskinning set up if they have `unique_reskin`
set when `Initialize(...)` runs.
https://github.com/tgstation/tgstation/blob/9145ecb7e1e44635a1056fc704adfa3d764325e6/code/game/objects/items.dm#L267-L269
Because pride pins use a global list, we set it in `Initialize(...)`...
After we call the parent.
https://github.com/tgstation/tgstation/blob/9145ecb7e1e44635a1056fc704adfa3d764325e6/code/modules/clothing/under/accessories/badges.dm#L196-L198
Obviously this fails.
However, moving this *before* `Initialize(...)`, while fixing the issue,
causes CI to fail due to calling `register_context()` twice.
Why? Well, it's simple. We automatically call `register_context()` if we
have `unique_reskin` set, as seen above, but we *also* call it on
accessory `Initialize(...)` due to it having its own context.
https://github.com/tgstation/tgstation/blob/0c562fd74299f8ce92a81c0a932b8ec4862189af/code/modules/clothing/under/accessories/_accessories.dm#L29-L31
This causes it to try register the same thing twice, which doesn't
_break_ things, but it sure as hell isn't clean.
So talking about this with San in code general, we decided to try go
with the following:
We add two new procs, `setup_reskinning()` and
`check_setup_reskinning()`, and handle all this fuckery within those.
This lets subtypes override them with their own new checks or
differences in setup.
Then we override `setup_reskinning()` for `/obj/item/clothing/under` and
`/obj/item/clothing/accessory` to not register context again, and do the
same for `/obj/item/clothing/accessory/pride` but while also setting
`unique_reskin`.
This fixes it.
Previous implementation for posterity
Back from my short code break, time to fix some of the things I've been
annoyed by.
Firstly, I noticed pride pins could no longer be reskinned since the
alt-click refactor.
Looking into it, this seems to be because we now only register this on
`Initialize(...)` if `unique_reskin` has been set:
https://github.com/tgstation/tgstation/blob/9145ecb7e1e44635a1056fc704adfa3d764325e6/code/game/objects/items.dm#L267-L269
While due to using a global list we don't set this in the item
definition, but in `Initialize(...)` :
https://github.com/tgstation/tgstation/blob/9145ecb7e1e44635a1056fc704adfa3d764325e6/code/modules/clothing/under/accessories/badges.dm#L196-L198
Where we call the parent proc _before_ setting `unique_reskin`, and thus
not registering our ability to reskin.
So all we do is set this to our global list _before_ we call the parent
proc.
```dm
/obj/item/clothing/accessory/pride/Initialize(mapload)
unique_reskin = GLOB.pride_pin_reskins // Set before parent proc checks for it.
. = ..()
```
This fixes it.
## Why It's Good For The Game
Fixes pride pin reskinning.
Theoretically makes it easier to avoid this happening in the future, and
allows `setup_reskinning()` to be manually called in the case of values
being edited post-initialize.
Previous pitch for posterity
Fixes pride pin reskinning.
## Changelog
:cl:
fix: Pride pins can be reskinned again with alt-click.
/:cl:
* Fix pride pin reskinning
---------
Co-authored-by: _0Steven <42909981+00-Steven@users.noreply.github.com>
---
code/game/objects/items.dm | 4 +--
code/game/objects/items_reskin.dm | 31 ++++++++++++++++++-
code/modules/clothing/under/_under.dm | 9 ++++--
.../under/accessories/_accessories.dm | 7 +++++
.../clothing/under/accessories/badges.dm | 8 +++--
5 files changed, 51 insertions(+), 8 deletions(-)
diff --git a/code/game/objects/items.dm b/code/game/objects/items.dm
index 087bffa4f61..77b7d8240c2 100644
--- a/code/game/objects/items.dm
+++ b/code/game/objects/items.dm
@@ -268,9 +268,7 @@
if(LAZYLEN(embedding))
updateEmbedding()
- if(unique_reskin)
- RegisterSignal(src, COMSIG_CLICK_ALT, PROC_REF(on_click_alt_reskin))
- register_context()
+ setup_reskinning()
/obj/item/Destroy(force)
diff --git a/code/game/objects/items_reskin.dm b/code/game/objects/items_reskin.dm
index 1a7c27e098d..9fa3b91d0e1 100644
--- a/code/game/objects/items_reskin.dm
+++ b/code/game/objects/items_reskin.dm
@@ -11,12 +11,41 @@
INVOKE_ASYNC(src, PROC_REF(reskin_obj), user)
return CLICK_ACTION_SUCCESS
+/**
+ * Checks if we should set up reskinning,
+ * by default if unique_reskin is set.
+ *
+ * Called on setup_reskinning().
+ * Inheritors should override this to add their own checks.
+ */
+/obj/item/proc/check_setup_reskinning()
+ SHOULD_CALL_PARENT(TRUE)
+ if(unique_reskin)
+ return TRUE
+
+ return FALSE
+
+/**
+ * Registers signals and context for reskinning,
+ * if check_setup_reskinning() passes.
+ *
+ * Called on Initialize(...).
+ * Inheritors should override this to add their own setup steps,
+ * or to avoid double calling register_context().
+ */
+/obj/item/proc/setup_reskinning()
+ SHOULD_CALL_PARENT(FALSE)
+ if(!check_setup_reskinning())
+ return
+
+ RegisterSignal(src, COMSIG_CLICK_ALT, PROC_REF(on_click_alt_reskin))
+ register_context()
/**
* Reskins object based on a user's choice
*
* Arguments:
- * * M The mob choosing a reskin option
+ * * user The mob choosing a reskin option
*/
/obj/item/proc/reskin_obj(mob/user)
if(!LAZYLEN(unique_reskin))
diff --git a/code/modules/clothing/under/_under.dm b/code/modules/clothing/under/_under.dm
index a4124a3baaf..9e51ca96708 100644
--- a/code/modules/clothing/under/_under.dm
+++ b/code/modules/clothing/under/_under.dm
@@ -51,10 +51,15 @@
if(random_sensor)
//make the sensor mode favor higher levels, except coords.
sensor_mode = pick(SENSOR_VITALS, SENSOR_VITALS, SENSOR_VITALS, SENSOR_LIVING, SENSOR_LIVING, SENSOR_COORDS, SENSOR_COORDS, SENSOR_OFF)
- if(!unique_reskin) // Already registered via unique reskin
- register_context()
+ register_context()
AddElement(/datum/element/update_icon_updates_onmob, flags = ITEM_SLOT_ICLOTHING|ITEM_SLOT_OCLOTHING, body = TRUE)
+/obj/item/clothing/under/setup_reskinning()
+ if(!check_setup_reskinning())
+ return
+
+ // We already register context in Initialize.
+ RegisterSignal(src, COMSIG_CLICK_ALT, PROC_REF(on_click_alt_reskin))
/obj/item/clothing/under/add_context(atom/source, list/context, obj/item/held_item, mob/living/user)
. = ..()
diff --git a/code/modules/clothing/under/accessories/_accessories.dm b/code/modules/clothing/under/accessories/_accessories.dm
index 67c2768ad23..fdaf666638d 100644
--- a/code/modules/clothing/under/accessories/_accessories.dm
+++ b/code/modules/clothing/under/accessories/_accessories.dm
@@ -30,6 +30,13 @@
. = ..()
register_context()
+/obj/item/clothing/accessory/setup_reskinning()
+ if(!check_setup_reskinning())
+ return
+
+ // We already register context regardless in Initialize.
+ RegisterSignal(src, COMSIG_CLICK_ALT, PROC_REF(on_click_alt_reskin))
+
/**
* Can we be attached to the passed clothing article?
*/
diff --git a/code/modules/clothing/under/accessories/badges.dm b/code/modules/clothing/under/accessories/badges.dm
index 7993ae45e00..451a71ea438 100644
--- a/code/modules/clothing/under/accessories/badges.dm
+++ b/code/modules/clothing/under/accessories/badges.dm
@@ -195,9 +195,13 @@ GLOBAL_LIST_INIT(pride_pin_reskins, list(
icon_state = "pride"
obj_flags = UNIQUE_RENAME | INFINITE_RESKIN
-/obj/item/clothing/accessory/pride/Initialize(mapload)
- . = ..()
+/obj/item/clothing/accessory/pride/setup_reskinning()
unique_reskin = GLOB.pride_pin_reskins
+ if(!check_setup_reskinning())
+ return
+
+ // We already register context regardless in Initialize.
+ RegisterSignal(src, COMSIG_CLICK_ALT, PROC_REF(on_click_alt_reskin))
/obj/item/clothing/accessory/deaf_pin
name = "deaf personnel pin"