From 13f87e4a26e3fe2e2e85d36fda6f824cc3e3ec8f Mon Sep 17 00:00:00 2001 From: VMSolidus Date: Fri, 29 May 2026 16:37:57 -0400 Subject: [PATCH] Various Skill Bugfixes (#22533) This PR fixes a bunch of skills related bugs, the biggest of which were the result of the system being overly trusting of the database, when in reality due to a bunch of unpredictable edge cases, the database is not guaranteed to always have what I think it has. To fix these bugs, I've had to slightly refactor how skills are generated on player characters and antagonists, such that the burden of proof for skills is with the Skills Subsystem rather than the Database. Skills generated for a fresh character that has NO preferences saved (Worst case scenario): image Promoting that same character to Antagonist now increases certain skills to a minimum baseline: image By Mel's request, Bluespace Technicians spawn with all skills fully maxed out for debugging purposes: image --- code/controllers/subsystems/skills.dm | 21 ++++++++++++++++--- .../combat/leadership_skill_component.dm | 5 ++++- .../service/ministry_skill_component.dm | 14 ++++++++----- code/datums/skills/_skills.dm | 14 ++++++++++--- code/game/antagonist/antagonist_create.dm | 10 ++++++--- code/modules/admin/verbs/bluespacetech.dm | 6 +++++- .../client/preference_setup/skills/skills.dm | 5 ----- code/modules/client/preferences.dm | 6 ++++++ .../hellfirejag-bunch-of-skill-fixes.yml | 10 +++++++++ 9 files changed, 70 insertions(+), 21 deletions(-) create mode 100644 html/changelogs/hellfirejag-bunch-of-skill-fixes.yml diff --git a/code/controllers/subsystems/skills.dm b/code/controllers/subsystems/skills.dm index 289611aa5fe..02959c27798 100644 --- a/code/controllers/subsystems/skills.dm +++ b/code/controllers/subsystems/skills.dm @@ -5,7 +5,16 @@ SUBSYSTEM_DEF(skills) /// This is essentially the list we use to read skills in the character setup. var/list/skill_tree = list() - /// The set of all skills that are "forced" in order to guarantee necessary components are applied. + /** + * The set of all known skill singletons. + * These are always typed as /singleton/skill and as such are safe to for(var/singleton/skill/skill as anything in SSskills.all_skills) + */ + var/list/all_skills = list() + + /** + * The set of all skills that are "forced" in order to guarantee necessary components are applied. + * These are always typed as /singleton/skill and as such are safe to for(var/singleton/skill/skill as anything in SSskills.required_skills) + */ var/list/required_skills = list() /datum/controller/subsystem/skills/Initialize() @@ -20,8 +29,9 @@ SUBSYSTEM_DEF(skills) // Next, we add the empty subcategory lists if they're not present. At this point, the tree would look like "Combat" -> "Melee" -> empty list // After that's done, if our skill is not present, add it to the empty list of the subcategory. for(var/singleton/skill/skill as anything in GET_SINGLETON_SUBTYPE_LIST(/singleton/skill)) - if (skill.required) - required_skills += skill.type + all_skills += skill + if (skill.required && skill.component_type) + required_skills += skill var/singleton/skill_category/skill_category = GET_SINGLETON(skill.category) if(!(skill.subcategory in skill_tree[skill_category])) skill_tree[skill_category] |= skill.subcategory @@ -30,3 +40,8 @@ SUBSYSTEM_DEF(skills) if(!(skill in skill_tree[skill_category][skill.subcategory])) skill_tree[skill_category][skill.subcategory] |= skill return SS_INIT_SUCCESS + +/datum/controller/subsystem/skills/Destroy() + all_skills.Cut() + required_skills.Cut() + return ..() diff --git a/code/datums/components/skills/combat/leadership_skill_component.dm b/code/datums/components/skills/combat/leadership_skill_component.dm index 6ec370d09c6..ce44ae5ba30 100644 --- a/code/datums/components/skills/combat/leadership_skill_component.dm +++ b/code/datums/components/skills/combat/leadership_skill_component.dm @@ -22,13 +22,16 @@ /datum/action/leadership/proc/get_target(owner, atom/target, modifiers) SIGNAL_HANDLER + UnregisterSignal(owner, COMSIG_MOB_CLICKON) + if (. == COMSIG_MOB_CANCEL_CLICKON) + return . // Another signal-handler already got to it. + // Both forms of deliver speech will immediately return control to the caller without blocking. // Unfortunately StrongDMM is apparently incapable of reading that. if (owner == target) UNLINT(deliver_speech_area(owner)) else UNLINT(deliver_speech_target(owner, target)) - UnregisterSignal(owner, COMSIG_MOB_CLICKON) return COMSIG_MOB_CANCEL_CLICKON /datum/action/leadership/proc/deliver_speech_area(mob/owner) diff --git a/code/datums/components/skills/service/ministry_skill_component.dm b/code/datums/components/skills/service/ministry_skill_component.dm index 1aeea1b2729..f5876ddba8f 100644 --- a/code/datums/components/skills/service/ministry_skill_component.dm +++ b/code/datums/components/skills/service/ministry_skill_component.dm @@ -27,22 +27,26 @@ /datum/action/ministry/proc/get_target(owner, atom/target, modifiers) SIGNAL_HANDLER + UnregisterSignal(owner, COMSIG_MOB_CLICKON) + if (. == COMSIG_MOB_CANCEL_CLICKON) + return . // Another signal-handler already got to it somehow. + + . = COMSIG_MOB_CANCEL_CLICKON if (owner == target) to_chat(owner, SPAN_NOTICE("You cannot offer a blessing to yourself.")) - return COMSIG_MOB_CANCEL_CLICKON + return . if (!astype(target, /mob)?.client) to_chat(owner, SPAN_NOTICE("[target] cannot receive a blessing.")) - return COMSIG_MOB_CANCEL_CLICKON + return . if (get_dist(owner, target) >= 2) to_chat(owner, SPAN_NOTICE("You must be adjacent to [target] to offer them a blessing.")) - return COMSIG_MOB_CANCEL_CLICKON + return . // StrongDMM for whatever ungodly reason can't tell that this proc won't block the caller. UNLINT(try_give_blessing(target)) - UnregisterSignal(owner, COMSIG_MOB_CLICKON) - return COMSIG_MOB_CANCEL_CLICKON + return . /datum/action/ministry/proc/try_give_blessing(mob/target) set waitfor = FALSE diff --git a/code/datums/skills/_skills.dm b/code/datums/skills/_skills.dm index e8c7db67bf4..4f76ff14036 100644 --- a/code/datums/skills/_skills.dm +++ b/code/datums/skills/_skills.dm @@ -27,10 +27,18 @@ /// The sub-category of this skill. Used to better sort skills. var/subcategory /** - * Required skills are always included in the user's saved skills preference, even if it's at the lowest rank. - * This is needed for skills that rely on components. + * Required skills are always loaded at a minimum skill level of 1 regardless of if they were saved to the preferences. + * This is useful for skills that provide a penalty below a certain skill level. */ var/required = FALSE + + /** + * For required skills on humanoid antags, they are always guaranteed a minimum of this skill level. + * If they have the skill previously at a lower level, their skill level is increased to this upon being promoted to antagonist. + * If they don't have the skill at all, they are given the skill at this level. + */ + var/antag_level = SKILL_LEVEL_TRAINED + /// The component datum that this skill will add during character spawning var/component_type = null /// Map of skill levels to level costs. How many skill points it takes to purchase this rank in a skill. @@ -74,7 +82,7 @@ */ /singleton/skill/proc/on_spawn(mob/owner, skill_level) SHOULD_CALL_PARENT(TRUE) - if (!owner || !component_type) + if (!owner || !component_type || (!required && skill_level == SKILL_LEVEL_UNFAMILIAR)) return owner.AddComponent(component_type, skill_level) diff --git a/code/game/antagonist/antagonist_create.dm b/code/game/antagonist/antagonist_create.dm index fd13fe62f29..186f8e09514 100644 --- a/code/game/antagonist/antagonist_create.dm +++ b/code/game/antagonist/antagonist_create.dm @@ -20,9 +20,13 @@ announce_antagonist_spawn() LAZYDISTINCTADD(SSticker.mode.antag_templates, src) - // Antags wipe skill components so that they can bypass skill restrictions entirely. - for(var/skill in target.current.GetComponents(/datum/component/skill)) - qdel(skill) + // Antags are always guaranteed certain minimum skill levels. + // Such that if a player character is promoted to an antagonist, they are always boosted up to a minimum competence required for antagging. + for(var/singleton/skill/skill as anything in SSskills.required_skills) + var/antag_skill_rank = skill.antag_level + var/datum/component/skill/skill_comp = target.current.LoadComponent(skill.component_type, antag_skill_rank) + if (skill_comp.skill_level < antag_skill_rank) + skill_comp.skill_level = antag_skill_rank /datum/antagonist/proc/create_default(var/mob/source) var/mob/living/M diff --git a/code/modules/admin/verbs/bluespacetech.dm b/code/modules/admin/verbs/bluespacetech.dm index d7a32100f5a..b2c7ddeb3c2 100644 --- a/code/modules/admin/verbs/bluespacetech.dm +++ b/code/modules/admin/verbs/bluespacetech.dm @@ -127,6 +127,10 @@ feedback_add_details("admin_verb","BST") + // Load all skills as max for a bst + for (var/singleton/skill/skill as anything in SSskills.all_skills) + skill.on_spawn(bst, skill.maximum_level) + return 1 /client/proc/bst_post_spawn(mob/living/carbon/human/bst/bst) @@ -141,7 +145,7 @@ var/datum/weakref/original_mob /mob/living/carbon/human/bst/Destroy(force) - original_mob = null + QDEL_NULL(original_mob) return ..() /mob/living/carbon/human/bst/can_inject(var/mob/user, var/error_msg, var/target_zone) diff --git a/code/modules/client/preference_setup/skills/skills.dm b/code/modules/client/preference_setup/skills/skills.dm index 4da48caba86..27ff8868334 100644 --- a/code/modules/client/preference_setup/skills/skills.dm +++ b/code/modules/client/preference_setup/skills/skills.dm @@ -68,11 +68,6 @@ loaded_skills = list() pref.skills = list() - for(var/key in SSskills.required_skills) - var/singleton/skill/skill = GET_SINGLETON(key) - if (istype(skill)) - pref.skills[skill.type] = SKILL_LEVEL_UNFAMILIAR - for(var/key,value in loaded_skills) if (!key) continue diff --git a/code/modules/client/preferences.dm b/code/modules/client/preferences.dm index c449d82856a..4287ce1c510 100644 --- a/code/modules/client/preferences.dm +++ b/code/modules/client/preferences.dm @@ -528,10 +528,16 @@ GLOBAL_LIST_EMPTY_TYPED(preferences_datums, /datum/preferences) if(istype(P) && (P.ability_flags & PSI_FLAG_CANON)) P.apply(character) + // Load all of the player-set skills first. for(var/skill_type in skills) var/singleton/skill/skill = GET_SINGLETON(skill_type) skill.on_spawn(character, skills[skill.type]) + // Attempt to load all the "required" skills. + // Player-set skills won't be overwritten here as LoadComponent will never re-initialize a component that already exists. + for(var/singleton/skill/required_skill as anything in SSskills.required_skills) + character.LoadComponent(required_skill.component_type, SKILL_LEVEL_UNFAMILIAR) + if(icon_updates) character.force_update_limbs() character.update_mutations(0) diff --git a/html/changelogs/hellfirejag-bunch-of-skill-fixes.yml b/html/changelogs/hellfirejag-bunch-of-skill-fixes.yml new file mode 100644 index 00000000000..d44a80457cf --- /dev/null +++ b/html/changelogs/hellfirejag-bunch-of-skill-fixes.yml @@ -0,0 +1,10 @@ +author: Hellfirejag +delete-after: True +changes: + - bugfix: "Hopefully fixed a hard delete with Bluespace Techs." + - rscadd: "Bluespace Techs now spawn with all skills fully maxed out" + - rscadd: "Upon being promoted to Antagonist, player characters now have certain skills (such as Firearms) always increased to at least a minimum level of competence." + - bugfix: "Fixed a bug where both Leadership and Ministry could apply to the same mouse click." + - bugfix: "Fixed Ministry not cancelling if you fail to click on a valid target." + - bugfix: "Fixed player characters who have never visited the Skills menu failing to generate required skills." + - bugfix: "Fixed player characters spawning with non-required skills (like Ministry) at rank 1."