Tests, bug fixes and improvements.

code/__DEFINES/skills/helpers.dm

@@ -14,17 +14,35 @@
 ///Doesn't automatically round the value.
 #define SANITIZE_SKILL_LEVEL(path, lvl) clamp(lvl, 0, GLOB.skill_datums[path].max_levels)
 
+/// Simple generic identifier macro.
+#define GET_SKILL_MOD_ID(path, id) (id ? "[path]&[id]" : path)
+
 /**
   * A simple universal comsig for body bound skill modifiers.
   * Technically they are still bound to the mind, but other signal procs will take care of adding and removing the modifier
   * from/to new/old minds.
   */
-#define ADD_SKILL_MODIFIER_BODY(mod, body) \
-	mod.RegisterSignal(body, COMSIG_MOB_ON_NEW_MIND, /datum/skill_modifier.proc/on_mob_new_mind, TRUE)
+#define ADD_SKILL_MODIFIER_BODY(path, id, body, prototype) \
+	prototype = GLOB.skill_modifiers[GET_SKILL_MOD_ID(path, id)] || new path(id);\
+	if(body.mind){\
+		body.mind.add_skill_modifier(prototype.identifier)\
+	} else {\
+		prototype.RegisterSignal(body, COMSIG_MOB_ON_NEW_MIND, /datum/skill_modifier.proc/on_mob_new_mind, TRUE)\
+	}
+
+/// Same as above but to remove the skill modifier.
+#define REMOVE_SKILL_MODIFIER_BODY(path, id, body) \
+	if(GLOB.skill_modifiers[GET_SKILL_MOD_ID(path, id)]){\
+		if(body.mind){\
+			body.mind.remove_skill_modifier(GET_SKILL_MOD_ID(path, id))\
+		} else {\
+			GLOB.skill_modifiers[GET_SKILL_MOD_ID(path, id)].UnregisterSignal(body, COMSIG_MOB_ON_NEW_MIND)\
+		}\
+	}
 
 ///Macro used when adding generic singleton skill modifiers.
 #define ADD_SINGLETON_SKILL_MODIFIER(mind, path, id) \
-	if(!GLOB.skill_modifiers[id]){\
+	if(!GLOB.skill_modifiers[GET_SKILL_MOD_ID(path, id)]){\
 		new path(id)\
 	};\
-	mind.add_skill_modifier(id ? "[path]&[id]" : path)
+	mind.add_skill_modifier(GET_SKILL_MOD_ID(path, id))

code/datums/components/mood.dm

@@ -221,23 +221,15 @@
 	var/apply_bonus = !apply_malus && newval <= ECSTATIC_SANITY_PEN
 	if(apply_malus)
 		if(!malus)
-			malus = new(malus_id++)
+			ADD_SKILL_MODIFIER_BODY(/datum/skill_modifier/bad_mood, malus_id++, L, malus)
 		var/debuff = 1 - (SANITY_DISTURBED - sanity) * MOOD_INSANITY_MALUS
 		malus.value_mod = malus.level_mod = debuff
-		if(!L.mind)
-			ADD_SKILL_MODIFIER_BODY(malus, L)
-		else
-			L.mind.add_skill_modifier(malus.identifier)
 	else if(malus)
 		QDEL_NULL(malus)
 
 	if(apply_bonus)
 		if(!bonus)
-			bonus = new(bonus_id++)
-		if(!L.mind)
-			ADD_SKILL_MODIFIER_BODY(bonus, L)
-		else
-			L.mind.add_skill_modifier(bonus.identifier)
+			ADD_SKILL_MODIFIER_BODY(/datum/skill_modifier/great_mood, bonus_id++, L, bonus)
 	else if(bonus)
 		QDEL_NULL(bonus)
 

code/datums/skills/_skill.dm

@@ -170,7 +170,7 @@ GLOBAL_LIST_INIT_TYPED(skill_datums, /datum/skill, init_skill_datums())
 	var/current_lvl_xp_sum = 0
 	if(level)
 		current_lvl_xp_sum = associative ? levels[levels[level]] : levels[level]
-	var/next_index = max(max_levels, level+1)
+	var/next_index = min(max_levels, level+1)
 	var/next_lvl_xp = associative ? levels[levels[next_index]] : levels[next_index]
 	if(next_lvl_xp > current_lvl_xp_sum)
 		next_lvl_xp -= current_lvl_xp_sum
@@ -178,6 +178,7 @@ GLOBAL_LIST_INIT_TYPED(skill_datums, /datum/skill, init_skill_datums())
 	return "[associative ? current_lvl : "Lvl. [current_lvl]"] ([value - current_lvl_xp_sum]/[next_lvl_xp])[level == max_levels ? " \[MAX!\]" : ""]"
 
 /datum/skill/level/job
+	abstract_type = /datum/skill/level/job
 	levels = list("Basic", "Trained", "Experienced", "Master")
 	competency_thresholds = list(JOB_SKILL_TRAINED, JOB_SKILL_EXPERT, JOB_SKILL_MASTER)
 	competency_mults = list(0.15, 0.1, 0.1)

code/datums/skills/_skill_holder.dm

@@ -109,7 +109,7 @@
 	var/affinity = get_skill_affinity(skill)
 	var/target_value = current + (value * affinity)
 	if(maximum)
-		target_value = max(target_value, maximum)
+		target_value = min(target_value, maximum)
 		if(target_value == maximum) //no more experience to gain, early return.
 			return
 	boost_skill_value_to(skill, target_value, silent, current)
@@ -172,7 +172,7 @@
 /datum/mind/proc/skill_html_readout()
 	var/list/out = list("<center><h1>Skills</h1></center><hr>")
 	out += "<table style=\"width:100%\"><tr><th><b>Skill</b><th><b>Value</b></tr>"
-	for(var/path in skill_holder.skills)
+	for(var/path in skill_holder.skills|skill_holder.skill_value_mods|skill_holder.skill_level_mods)
 		var/datum/skill/S = GLOB.skill_datums[path]
 		var/skill_value = get_skill_value(path)
 		var/skill_level = get_skill_level(path, round = TRUE)

code/datums/skills/_skill_modifier.dm

@@ -23,21 +23,21 @@ GLOBAL_LIST_EMPTY(potential_mods_per_skill)
 	var/priority = MODIFIER_SKILL_PRIORITY_DEF
 
 /datum/skill_modifier/New(id)
-	identifier = type
+	identifier = GET_SKILL_MOD_ID(type, id)
 	if(id)
-		identifier = "[type]&[id]"
+		var/former_id = identifier
 		var/dupe = 0
 		while(GLOB.skill_modifiers[identifier])
-			identifier = "[type]&[id][++dupe]"
+			identifier = "[former_id][++dupe]"
 	GLOB.skill_modifiers[identifier] = src
 
 	if(ispath(target_skills))
 		var/list/mod_L = GLOB.potential_mods_per_skill[target_skills]
 		if(!mod_L)
-			GLOB.potential_mods_per_skill[target_skills] = list(identifier = src)
+			mod_L = GLOB.potential_mods_per_skill[target_skills] = list()
 		else
 			BINARY_INSERT(identifier, mod_L, datum/skill_modifier, src, priority, COMPARE_VALUE)
-			mod_L[identifier] = src
+		mod_L[identifier] = src
 		GLOB.potential_skills_per_mod["[target_skills]"] = list(target_skills)
 	else //Should be a bitfield.
 		var/list/L = GLOB.potential_skills_per_mod["[target_skills]"]
@@ -50,10 +50,10 @@ GLOBAL_LIST_EMPTY(potential_mods_per_skill)
 		for(var/path in L)
 			var/list/mod_L = GLOB.potential_mods_per_skill[path]
 			if(!mod_L)
-				GLOB.potential_mods_per_skill[path] = list(identifier = src)
+				mod_L = GLOB.potential_mods_per_skill[target_skills] = list()
 			else
 				BINARY_INSERT(identifier, mod_L, datum/skill_modifier, src, priority, COMPARE_VALUE)
-				mod_L[identifier] = src
+			mod_L[identifier] = src
 
 /datum/skill_modifier/Destroy()
 	for(var/A in GLOB.potential_skills_per_mod["[target_skills]"])
@@ -70,11 +70,11 @@ GLOBAL_LIST_EMPTY(potential_mods_per_skill)
 		L[P] = GLOB.potential_mods_per_skill[P] & (__L + id)\
 	}\
 	if(M.modifier_flags & MODIFIER_SKILL_ORIGIN_DIFF){\
-		LAZYADDASSOC(O, id, P = G)\
+		LAZYADDASSOC(O, id, "[P]" = G)\
 	}
 
 /datum/mind/proc/add_skill_modifier(id)
-	if(skill_holder.all_current_skill_modifiers[id])
+	if(LAZYACCESS(skill_holder.all_current_skill_modifiers, id))
 		return
 	var/datum/skill_modifier/M = GLOB.skill_modifiers[id]
 	if(!M)
@@ -99,7 +99,7 @@ GLOBAL_LIST_EMPTY(potential_mods_per_skill)
 
 	if(M.modifier_flags & MODIFIER_SKILL_BODYBOUND)
 		M.RegisterSignal(src, COMSIG_MIND_TRANSFER, /datum/skill_modifier.proc/on_mind_transfer)
-		ADD_SKILL_MODIFIER_BODY(M, current)
+		M.RegisterSignal(current, COMSIG_MOB_ON_NEW_MIND, /datum/skill_modifier.proc/on_mob_new_mind, TRUE)
 	RegisterSignal(M, COMSIG_PARENT_QDELETING, .proc/on_skill_modifier_deletion)
 
 #undef ADD_MOD_STEP
@@ -107,7 +107,7 @@ GLOBAL_LIST_EMPTY(potential_mods_per_skill)
 #define REMOVE_MOD_STEP(L, P, O)\
 	LAZYREMOVEASSOC(L, P, id);\
 	if(M.modifier_flags & MODIFIER_SKILL_ORIGIN_DIFF){\
-		LAZYREMOVEASSOC(O, id, P)\
+		LAZYREMOVEASSOC(O, id, "[P]")\
 	}
 
 /datum/mind/proc/remove_skill_modifier(id, mind_transfer = FALSE)
@@ -164,7 +164,7 @@ GLOBAL_LIST_EMPTY(potential_mods_per_skill)
 				to_access = H.original_levels
 			if(MODIFIER_TARGET_AFFINITY)
 				to_access = H.original_affinities
-		. += value - LAZYACCESS(to_access[identifier], skillpath)
+		. += value - LAZYACCESS(to_access[identifier], "[skillpath]")
 
 ///Body bound modifier signal procs.
 /datum/skill_modifier/proc/on_mind_transfer(datum/mind/source, mob/new_character, mob/old_character)