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

Boot icon fixes. #5106

Closed
wants to merge 2 commits into from
Closed

Boot icon fixes. #5106

wants to merge 2 commits into from

Conversation

silencer-pl
Copy link
Contributor

@silencer-pl silencer-pl commented Dec 2, 2023

About the pull request

This MR moves boots into an overlay under the uniform and touches up boot on mob sprites to look like actual boots, since they now don't have to be cut off for the uniforms.

The only visible effect on CM of this change is that the CL and Synth can now wear boots with their black skirt suit, see pictures.

The more significant effect is that this properly layers socks/boots/shoes under uniforms, meaning that should someone downstream say add uniforms with skirts, boots/stockings will not overlay on top of them.

This also fixes the RMC Boots and makes the item icon properly update to reflect whether they have a knife or not (as they were missing a proc that did that)

Finally it also removes a garbage var and and an empty check using it.

Due to how the global part of update_icon() handles item_state paths, removing the -1 onmob icons breaks onmob icons that appear on spawned-in npcs. I'm sure fixing that will break it in 100 other places, so I opted for the -1 icons instead.

Explain why it's good for the game

Proper, clear layering that does not require dmi shenanigans is good for both spriters and coders.
Onmob icons now properly represent what the boots actually are, which makes the icons universal which is also good.
Happy downstream that does not have to implement this as a separate fix :)

Testing Photographs and Procedure

Screenshots & Videos

Skirt and boots:

image
image

RMC Boot icon:

image

Changelog

🆑silencer_pl
fix: Boots now properly render under skirts/shorts/uniforms etc without icon edits
fix: RMC Boot icons now properly reflect if an object is holstered in them
/:cl:

@github-actions github-actions bot added Sprites Remove the soul from the game. Fix Fix one bug, make ten more labels Dec 2, 2023
@silencer-pl
Copy link
Contributor Author

silencer-pl commented Dec 2, 2023

Indicentally, with how /obj/proc/update_icon() is handled, I dont think item_state actually does antyhing at present, at least durign the initial spawn/map init pass I went into more detail about it at https://discord.com/channels/150315577943130112/745447048261795890/1180603291365806120

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

@github-actions github-actions bot added the Stale beg a maintainer to review your PR label Dec 10, 2023
@silencer-pl
Copy link
Contributor Author

Closing. I can't wait 9 days for a review of 3 lines of code, considering my other MR involving sprites was just let to rot, I can take the hint. This has been merged into my downstream, feel free to reopen the fix when someone feels like actually putting this in.

@silencer-pl silencer-pl deleted the boots branch December 11, 2023 14:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Fix Fix one bug, make ten more Sprites Remove the soul from the game. Stale beg a maintainer to review your PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant