Fix pride pin reskinning

[00-Steven]

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.

tgstation/code/game/objects/items.dm

Lines 267 to 269 in 9145ecb

	if(unique_reskin)
	RegisterSignal(src, COMSIG_CLICK_ALT, PROC_REF(on_click_alt_reskin))
	register_context()

Because pride pins use a global list, we set it in Initialize(...)... After we call the parent.

tgstation/code/modules/clothing/under/accessories/badges.dm

Lines 196 to 198 in 9145ecb

	/obj/item/clothing/accessory/pride/Initialize(mapload)
	. = ..()
	unique_reskin = GLOB.pride_pin_reskins

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.

tgstation/code/modules/clothing/under/accessories/_accessories.dm

Lines 29 to 31 in 0c562fd

	/obj/item/clothing/accessory/Initialize(mapload)
	. = ..()
	register_context()

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:

tgstation/code/game/objects/items.dm

Lines 267 to 269 in 9145ecb

	if(unique_reskin)
	RegisterSignal(src, COMSIG_CLICK_ALT, PROC_REF(on_click_alt_reskin))
	register_context()

While due to using a global list we don't set this in the item definition, but in Initialize(...) :

tgstation/code/modules/clothing/under/accessories/badges.dm

Lines 196 to 198 in 9145ecb

	/obj/item/clothing/accessory/pride/Initialize(mapload)
	. = ..()
	unique_reskin = GLOB.pride_pin_reskins

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.

/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

🆑
fix: Pride pins can be reskinned again with alt-click.
/:cl:

[san7890]

There is a better way to fix this that makes it more extendable for future coders:

Do the following in [tgstation/code/game/objects/items.dm] Lines 267 to 269 in [9145ecb] Initialize() that runs the code if either one of the obj_flags is set (in addition to unique_reskin). This is just leveraging the existing framework as the pins already have this flag set.

if(unique_reskin || (obj_flags & (UNIQUE_RENAME | INFINITE_RESKIN)))
 RegisterSignal(src, COMSIG_CLICK_ALT, PROC_REF(on_click_alt_reskin)) 
 register_context()

Much better.

[san7890]

Also you're failing CI doing it the way it is presently so just do my suggestion

[00-Steven]

As mentioned on the discord, the failing CI is a matter of oops double register due to accessory+reskin.
Anyhow, drafting.

[github-actions]

This PR has been inactive for long enough to be automatically marked as stale. This means it is at risk of being auto closed in ~ 7 days, please address any outstanding review items and ensure your PR is finished, if these are all true and you are auto-staled anyway, you need to actively ask maintainers if your PR will be merged. Once you have done any of the previous actions then you should request a maintainer remove the stale label on your PR, to reset the stale timer. If you feel no maintainer will respond in that time, you may wish to close this PR youself, while you seek maintainer comment, as you will then be able to reopen the PR yourself

[00-Steven]

funny how the commits I forgot to push show up as being before the stale label post

[00-Steven]

Now just need to update the main pr body

[00-Steven]

writeup's good enough:tm:, undrafting once checks're done