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

Fix Select Equipment Presets #5758

Merged
merged 3 commits into from
Feb 21, 2024
Merged

Conversation

Kaga-404
Copy link
Contributor

@Kaga-404 Kaga-404 commented Feb 20, 2024

About the pull request

This is literally a one-line fix to an apparently unnoticed bug affecting multiple loadouts, particularly certain ERTs. Explanation below.

Select Equipment now spawns in an ID before equipment.

Explain why it's good for the game

ID Locks check via the COMSIG_ITEM_ATTEMPTING_EQUIP signal when someone tries to equip them, and searches for an equipped ID to lock with. If there's none, it cancels the equip.

/obj/item/proc/mob_can_equip() sends out that signal as part of its testing, and the usual mob equip_to_slot_or_del() proc used in most presets ends up checking the item's mob_can_equip() before equipping.

IDs were generated AFTER equipment within /datum/equipment_preset/proc/load_preset() in _select_equipment.dm. This means every single item with an ID lock would be spawned in, the mob would see if it could equip them, the item would return no, cancelling the equip, and the item would be deleted.

Pre-equipped Spec loadouts were the most affected, losing all of their armor and ID-locked sights. Most special ERT NVGs were affected as well, being subtypes of the Sniper Spec's NVGs, and sharing the same MOB_LOCK_ON_EQUIP flag.

If there's a reason IDs were spawned in after, this might break something, but I doubt it. Just went tracking once I noticed this bug, and found this as the most effective solution.

An alternative solution was to use the unsafe equip_to_slot() proc instead.

Testing Photographs and Procedure

Screenshots & Videos

Put screenshots and videos here with an empty line between the screenshots and the <details> tags.

Changelog

🆑 Kaga
fix: Fixed a bug that broke ID-Lockable items in presets. Rejoice, PMC Snipers, pre-equipped USCM Specs, UPP Commandos, and WY Deathsquads.
/:cl:

@github-actions github-actions bot added the Fix Fix one bug, make ten more label Feb 20, 2024
@harryob
Copy link
Member

harryob commented Feb 20, 2024

If there's a reason IDs were spawned in after, this might break something, but I doubt it.

#3435

this pr looks like it was dancing around difficulties in when the name is loaded for clf synths, but didn't address the core issue - your pr would end up reverting the quasi-fix

@Kaga-404
Copy link
Contributor Author

If there's a reason IDs were spawned in after, this might break something, but I doubt it.

#3435

this pr looks like it was dancing around difficulties in when the name is loaded for clf synths, but didn't address the core issue - your pr would end up reverting the quasi-fix

Did some further investigating, found the issue - you're absolutely correct that the PR's fix was really sloppy. The issue isn't tied to CLF Synths, it's tied to what happens when you Select-Equipment versus spawn in a new mob.

What goes wrong, from just a couple minutes of testing, is that the 'broken' presets are those which call load_name() within their load_gear() procs. load_name() is already called when load_preset() is called, towards the beginning. So they get an ID with their first randomly generated name, and then again when their ID is spawned.

If load_id() is after gear, then it's spawned in with their second generated name. If it's before, it generates with the first.

The second reported error (Select-Equipment not changing names) is intentional. Select-Equipment, as opposed to ERTs and Create-Human, does not randomize. The first load_name() isn't triggered, and so they get an ID with their old name, and then get their mob name changed by the load_gear(). I assume this was done in an attempt to make it so people could be transformed into those Synth presets without needing to spawn in a new mob, but that's really not a good reason. SE for keeping a name but changing species and gear, Create Human and put a CKEY inside or spawn an ERT/Freed Mob if you want it to match.

Patching in the fix now, at least for CLF Multipurpose Synth and UPP Combat Synth, by removing the secondary load_name() procs. This shouldn't impact normal gameplay at all, only makes it so spontaneously transforming someone into a synth with those presets by Select Equipment will not change their name at all, giving them an ID that matches their original name. Using VV to change their name (by clicking on the name) will automatically update the ID anyway.

Copy link
Member

@harryob harryob left a comment

Choose a reason for hiding this comment

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

sounds good

@harryob harryob added this pull request to the merge queue Feb 21, 2024
Merged via the queue into cmss13-devs:master with commit 2abcb7b Feb 21, 2024
26 checks passed
cm13-github added a commit that referenced this pull request Feb 21, 2024
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants