Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Assorted minor alt-click reskinning fixes, primarily context-based #83105

Merged
merged 5 commits into from
May 16, 2024

Conversation

00-Steven
Copy link
Contributor

@00-Steven 00-Steven commented May 6, 2024

About The Pull Request

This is a collection of tiny alt-click context fixes that I found during testing #82920, but I felt were not right to put in there.
Most of the following explanation is for posterity, like they're mostly one-liners, there's only so much explanation to do.

First off, the emotion mask would reset current_skin for infinite reskinning, while we have the INFINITE_RESKIN flag:

/obj/item/clothing/mask/joy/reskin_obj(mob/user)
. = ..()
user.update_worn_mask()
current_skin = null//so we can infinitely reskin

We set this to INFINITE_RESKIN for sanity's sake.

Then, /obj/item/clothing/under/add_context(...) would call its parent, but sometimes return NONE when its parent returned CONTEXTUAL_SCREENTIP_SET:

return changed ? CONTEXTUAL_SCREENTIP_SET : NONE

This is bad, because reskinning context is handled on the parent (/obj/item), and we have an item inheriting this which can be reskinned, the mech pilot's suit:
/obj/item/clothing/under/costume/mech_suit
name = "mech pilot's suit"
desc = "A mech pilot's suit. Might make your butt look big."
icon_state = "red_mech_suit"
inhand_icon_state = null
body_parts_covered = CHEST|GROIN|LEGS|FEET|ARMS|HANDS
cold_protection = CHEST|GROIN|LEGS|FEET|ARMS|HANDS
female_sprite_flags = NO_FEMALE_UNIFORM
alternate_worn_layer = GLOVES_LAYER //covers hands but gloves can go over it. This is how these things work in my head.
can_adjust = FALSE
unique_reskin = list(
"Red" = "red_mech_suit",
"White" = "white_mech_suit",
"Blue" = "blue_mech_suit",
"Black" = "black_mech_suit",
)

So we make this return the parent return value rather than NONE:

return changed ? CONTEXTUAL_SCREENTIP_SET : .

Next up, /obj/item/clothing/accessory/add_context(...) would never actually call the parent and thus neither the reskinning context. It also checks for whether you have an item in your active hand when context is added, even though the context it adds actually only applies when the accessory itself is in your active hand.

/obj/item/clothing/accessory/add_context(atom/source, list/context, obj/item/held_item, mob/user)
if(!isnull(held_item))
return NONE
context[SCREENTIP_CONTEXT_RMB] = "Wear [above_suit ? "below" : "above"] suit"
return CONTEXTUAL_SCREENTIP_SET

So we instead make it call the parent first, check for whether the accessory itself is in our active hand, and return the parent value if not:

/obj/item/clothing/accessory/add_context(atom/source, list/context, obj/item/held_item, mob/user)
	. = ..()
	if(held_item != source)
		return .
	(...)

This resolves our issue.

We're almost there!
/obj/item/reagent_containers/spray/medical/add_context(...) exists, but is entirely redundant due to this now being handled on the base item, and also misses some of the checks it has.

/obj/item/reagent_containers/spray/medical/add_context(atom/source, list/context, obj/item/held_item, mob/user)
. = ..()
if(!current_skin)
context[SCREENTIP_CONTEXT_ALT_LMB] = "Reskin"
return CONTEXTUAL_SCREENTIP_SET

So we just remove it.

Finally, what is to me the funniest one:

if(!(obj_flags & INFINITE_RESKIN) && current_skin)
return NONE

To add reskinning context, we check item_flags for INFINITE_RESKIN, while it is actually on obj_flags.
So, instead, we were checking for the equivalent value in item_flags, being IN_STORAGE.
#define INFINITE_RESKIN (1<<11) // We can reskin this item infinitely

#define IN_STORAGE (1<<11) //is this item in the storage item, such as backpack? used for tooltips

And thus reskinning context for infinitely reskinnables would only show up if they were in storage.
For now, we just update this to use obj_flags instead.

That's everything I found so far, which this should all fix.

Why It's Good For The Game

Having working item usage context tends to be a good thing.

Changelog

馃啈
fix: Emotion masks no longer use a janky workaround for infinite reskinning.
fix: Mech pilot suit shows reskinning usage context correctly.
fix: Accessories show "wear above/below suit" usage context appropriately.
fix: Accessories don't block reskinning usage context when they shouldn't.
fix: Showing reskinning usage context cares about the infinite reskinning flag, rather than whether it's in storage or not.
del: Removed redundant reskinning usage context code from medical sprays, now shows reskinning usage context like other reskinnables.
/:cl:

@tgstation-server tgstation-server added Fix Rewrites a bug so it appears in different circumstances Removal This was too fun, too fun! I'm turning this feature around labels May 6, 2024
@00-Steven 00-Steven marked this pull request as ready for review May 6, 2024 23:36
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Fix Rewrites a bug so it appears in different circumstances Removal This was too fun, too fun! I'm turning this feature around Stale Even the uncaring universe rejects you, why even go on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants