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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent random slots from hiding character noses and hair #1028

Closed
wants to merge 11 commits into from
Closed

Conversation

DinoWattz
Copy link
Contributor

@DinoWattz DinoWattz commented Mar 28, 2024

Why / Balance

Small change that fixes #982

Technical details

Changed the enumerator at line 97 to make it check only the "HEAD" and "MASK" slots.
Fixed formatting at line 167.

Media

  • I have added screenshots/videos to this PR showcasing its changes ingame, or this PR does not require an ingame showcase

@github-actions github-actions bot added the Changes: C# Changes any cs files label Mar 28, 2024
@DinoWattz DinoWattz marked this pull request as ready for review March 28, 2024 17:37
Copy link
Member

@DEATHB4DEFEAT DEATHB4DEFEAT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would probably be better to have clothing check if it's where it should be before doing things instead of adding yet another check against pockets.

@DinoWattz
Copy link
Contributor Author

It would probably be better to have clothing check if it's where it should be before doing things instead of adding yet another check against pockets.

I don't think that's necessary, if it's not in the pocket slot then it's either equipped in the right place or somewhere in the inventory which doesn't count as clothing

@DangerRevolution
Copy link
Contributor

It would probably be better to have clothing check if it's where it should be before doing things instead of adding yet another check against pockets.

I don't think that's necessary, if it's not in the pocket slot then it's either equipped in the right place or somewhere in the inventory which doesn't count as clothing

Death is suggesting future-proofing clothing; rather than band-aiding this. i.e if we get more clothing slots in the future, or a character gets some new type of wacky clothing slot or feature. Less so about overdoing this simple fix and more future-prevention.

@Pspritechologist
Copy link
Contributor

Pspritechologist commented Mar 28, 2024

It would probably be better to have clothing check if it's where it should be before doing things instead of adding yet another check against pockets.

I don't think that's necessary, if it's not in the pocket slot then it's either equipped in the right place or somewhere in the inventory which doesn't count as clothing

There are a variety of situations where it may be equipped to something that isn't a specific 'pocket' slot, and designing this in a way that any additions in YAML cause this system to break isn't a good design pattern.
Serialization needs to be considered as a variable when designing systems, they should never break because content was added.

@DinoWattz
Copy link
Contributor Author

Death is suggesting future-proofing clothing; rather than band-aiding this. i.e if we get more clothing slots in the future, or a character gets some new type of wacky clothing slot or feature. Less so about overdoing this simple fix and more future-prevention.

If someone were to add a normal clothing slot this wouldn't affect much since this is only aimed towards masks and hats (ones with "HidesNose" and "HidesHair" tag);
But if someone were to add a clothing slot that accepts almost anything like a pocket slot but isn't one I'd hunt them down myself it'd really cause an issue similar to the one this PR is trying to fix.

I'll see if I can improve it somehow.

@DinoWattz DinoWattz marked this pull request as draft March 28, 2024 20:26
@Pspritechologist
Copy link
Contributor

Death is suggesting future-proofing clothing; rather than band-aiding this. i.e if we get more clothing slots in the future, or a character gets some new type of wacky clothing slot or feature. Less so about overdoing this simple fix and more future-prevention.

If someone were to add a normal clothing slot this wouldn't affect much since this is only aimed towards masks and hats (ones with "HidesNose" and "HidesHair" tag); But if someone were to add a clothing slot that accepts almost anything like a pocket slot but isn't one I'd hunt them down myself it'd really cause an issue similar to the one this PR is trying to fix.

I'll see if I can improve it somehow.

This already exists?? Don't spider people literally have a non-pocket free slot??
In a game where variables are free to be modified, it simply isn't your place to decide what way to use them is valid. If data is exposed, it muse be accounted for.

You also just literally don't gain anything by not doing it the proper way.

@DinoWattz
Copy link
Contributor Author

DinoWattz commented Mar 28, 2024

This already exists?? Don't spider people literally have a non-pocket free slot?? In a game where variables are free to be modified, it simply isn't your place to decide what way to use them is valid. If data is exposed, it muse be accounted for.

You also just literally don't gain anything by not doing it the proper way.

Arachnids have two extra pocket slots, I checked and they really are flagged as pocket slots, I'm more afraid of whatever is called "suitstorage".
Either way I already have an idea of how to change the code now.

@DinoWattz DinoWattz changed the title Fix pocket slots hiding character visual layers Prevent random slots from hiding character noses and hair Mar 28, 2024
@DinoWattz
Copy link
Contributor Author

DinoWattz commented Mar 28, 2024

It now only checks for the necessary slots, masks would only hide your nose/hair if it's on your face and hats if it's on your head.

@DinoWattz DinoWattz marked this pull request as ready for review March 28, 2024 21:36
@DinoWattz DinoWattz requested a review from DEATHB4DEFEAT March 28, 2024 21:37
Copy link
Contributor

@NullWanderer NullWanderer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't this also affect our upstream?

@DinoWattz
Copy link
Contributor Author

Wouldn't this also affect our upstream?

Just tested here and upstream also suffers from the same issue (#982), would fixing it here also fix it there? I'm kind of new to this

@NullWanderer
Copy link
Contributor

Wouldn't this also affect our upstream?

Just tested here and upstream also suffers from the same issue (#982), would fixing it here also fix it there? I'm kind of new to this

No, its the other way around. They cannot take fixes from here as we are under an incompatible license, however, changes they make will make their way down to us. It is better to contribute fixes to them, and have it make its way down to us.

@NullWanderer
Copy link
Contributor

Long story short, changes that affect files that aren't namespaced as DeltaV, Nyanotrasen or SimpleStation(I think) should be sent to https://github.com/space-wizards/space-station-14 instead, as that way it'll reach most server, instead of only benefiting us and our downstreams.

@DinoWattz
Copy link
Contributor Author

No, its the other way around. They cannot take fixes from here as we are under an incompatible license, however, changes they make will make their way down to us. It is better to contribute fixes to them, and have it make its way down to us.

Long story short, changes that affect files that aren't namespaced as DeltaV, Nyanotrasen or SimpleStation(I think) should be sent to https://github.com/space-wizards/space-station-14 instead, as that way it'll reach most server, instead of only benefiting us and our downstreams.

Alright, thanks for telling me

so I should send a PR to upstream instead, I'll close this one.

@DinoWattz DinoWattz closed this Apr 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changes: C# Changes any cs files S: Needs Review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Having masks in the pocket slot will hide snout markings
5 participants