From 025018014df6f1fb73e4f9abe22912ac608558f8 Mon Sep 17 00:00:00 2001 From: Timberpoes Date: Fri, 29 Oct 2021 20:38:00 +0100 Subject: [PATCH] Tackles various problems with keyed_list config entries, fixing broken roundstart races and more! (#62359) While helping RaveRadbury debug some issues with enabling Halloween species early via the brute force method of enabling them in the config rather than the gentleman's solution of testmerging a PR that changes the Halloween date, we discovered something dreadful. Cloth golems cannot be enabled! Infact, any species with a space in the ID cannot be enabled. It uses the splitter despite VALUE_MODE_FLAG being set. So a key entry like ROUNDSTART_RACES cloth golem would get parsed as cloth = golem, then entered into the config as cloth = TRUE NEW AND IMPROVED PART HERE I've re-written how keyed_list config entries are parsed, splitting it into a number of procs to do some discrete block of logic. Based on feedback from MSO, he expected that VALUE_MODE_FLAG keyed_list entries could have elements overridden. However, this functionality was not present in the code. I have implemented it. We now support 3 methods of setting VALUE_MODE_FLAGS. Implicitly enable the config entry: CONFIG_ENTRY config_key_goes_here Explicitly enable the config entry: CONFIG_ENTRY config_key_goes_here 1 Explicitly disable the config entry: CONFIG_ENTRY config_key_goes_here 0 There have been functionality changes too. Previously, everything before the first splitter was the key and everything after was the value. However, in ambiguous config entries (Such as ROUNDSTART_RACES cloth golem 0) it would be unclear if the intent was (cloth, golem 0) or (cloth golem, 0) or indeed if the intent was (cloth golem 0, 1). As a result, there is now the following paradigm in place: Everything after the LAST splitter is the value, everything before is the key and a log_config warning is now given explaining the problem and showing how it was resolved. [2021-10-27 19:48:12.840] WARNING: Multiple splitter characters (" ") found. Using "cloth golem" as config key and "1" as config value. This warning will trigger if multiple splitters are present for any keyed_list config entry, and will trigger on implicit VALUE_MODE_FLAGS entries that have splitters. The example above is it triggering on ROUNDSTART_RACES cloth golem - It has detected that there is potential ambiguity between (cloth, golem) or (cloth golem, 1), has picked a sensible option for the data type and has warned about it. The intent is that no config entry should be ambiguous. It should be clear what is key and what is value when dealing with keyed_list config entries. There's probably more work to do on other config entries to bring them up to this standard, but this is the thing I'm hitting in this PR. Similarly, I have improved the validation aspect of keyed_list config entries with additional logging in general. [2021-10-27 19:47:53.135] ERROR: Invalid KEY_MODE_TYPE typepath. Is not a valid typepath: /mob/living/carbon/monkey I have added a unit test to make sure species IDs do not contain splitters from the two keyed_list subtypes relating to species. I have added sanity checking to the race config subtypes since we have a big dick global list of all races sorted by ID, so a race not existing will fail validation and output a meaningful config log entry. I have removed /datum/config_entry/keyed_list/probability from the code as it is unused with the removal of all game modes except Dynamic. The config change necessitated the renaming of all golem species IDs. Doing so and renaming the clothgolem.ts file to match has fixed the broken cloth golem page too. --- code/__DEFINES/DNA.dm | 52 +++---- code/__HELPERS/_lists.dm | 6 + .../controllers/configuration/config_entry.dm | 134 +++++++++++++----- .../configuration/entries/game_options.dm | 21 ++- code/game/objects/structures/mirror.dm | 14 +- .../human/species_types/shadowpeople.dm | 2 +- .../ruins/objects_and_mobs/sin_ruins.dm | 7 +- code/modules/unit_tests/_unit_tests.dm | 2 +- .../unit_tests/species_config_sanity.dm | 22 +++ config/game_options.txt | 37 ++++- .../species/{clothgolem.ts => cloth_golem.ts} | 0 11 files changed, 208 insertions(+), 89 deletions(-) create mode 100644 code/modules/unit_tests/species_config_sanity.dm rename tgui/packages/tgui/interfaces/PreferencesMenu/preferences/species/{clothgolem.ts => cloth_golem.ts} (100%) diff --git a/code/__DEFINES/DNA.dm b/code/__DEFINES/DNA.dm index 306dacf8caa..cc992f93c3f 100644 --- a/code/__DEFINES/DNA.dm +++ b/code/__DEFINES/DNA.dm @@ -273,29 +273,29 @@ GLOBAL_LIST_INIT(organ_process_order, list( #define SPECIES_SYNTH_MILITARY "military_synth" //Defines for Golem Species IDs -#define SPECIES_GOLEM "iron golem" -#define SPECIES_GOLEM_ADAMANTINE "adamantine golem" -#define SPECIES_GOLEM_PLASMA "plasma golem" -#define SPECIES_GOLEM_DIAMOND "diamond golem" -#define SPECIES_GOLEM_GOLD "gold golem" -#define SPECIES_GOLEM_SILVER "silver golem" -#define SPECIES_GOLEM_PLASTEEL "plasteel golem" -#define SPECIES_GOLEM_TITANIUM "titanium golem" -#define SPECIES_GOLEM_PLASTITANIUM "plastitanium golem" -#define SPECIES_GOLEM_ALIEN "alloy golem" -#define SPECIES_GOLEM_WOOD "wood golem" -#define SPECIES_GOLEM_URANIUM "uranium golem" -#define SPECIES_GOLEM_SAND "sand golem" -#define SPECIES_GOLEM_GLASS "glass golem" -#define SPECIES_GOLEM_BLUESPACE "bluespace golem" -#define SPECIES_GOLEM_BANANIUM "bananium golem" -#define SPECIES_GOLEM_CULT "runic golem" -#define SPECIES_GOLEM_CLOTH "cloth golem" -#define SPECIES_GOLEM_PLASTIC "plastic golem" -#define SPECIES_GOLEM_BRONZE "bronze golem" -#define SPECIES_GOLEM_CARDBOARD "cardboard golem" -#define SPECIES_GOLEM_LEATHER "leather golem" -#define SPECIES_GOLEM_DURATHREAD "durathread golem" -#define SPECIES_GOLEM_BONE "bone golem" -#define SPECIES_GOLEM_SNOW "snow golem" -#define SPECIES_GOLEM_HYDROGEN "Metallic Hydrogen golem" +#define SPECIES_GOLEM "iron_golem" +#define SPECIES_GOLEM_ADAMANTINE "adamantine_golem" +#define SPECIES_GOLEM_PLASMA "plasma_golem" +#define SPECIES_GOLEM_DIAMOND "diamond_golem" +#define SPECIES_GOLEM_GOLD "gold_golem" +#define SPECIES_GOLEM_SILVER "silver_golem" +#define SPECIES_GOLEM_PLASTEEL "plasteel_golem" +#define SPECIES_GOLEM_TITANIUM "titanium_golem" +#define SPECIES_GOLEM_PLASTITANIUM "plastitanium_golem" +#define SPECIES_GOLEM_ALIEN "alloy_golem" +#define SPECIES_GOLEM_WOOD "wood_golem" +#define SPECIES_GOLEM_URANIUM "uranium_golem" +#define SPECIES_GOLEM_SAND "sand_golem" +#define SPECIES_GOLEM_GLASS "glass_golem" +#define SPECIES_GOLEM_BLUESPACE "bluespace_golem" +#define SPECIES_GOLEM_BANANIUM "bananium_golem" +#define SPECIES_GOLEM_CULT "runic_golem" +#define SPECIES_GOLEM_CLOTH "cloth_golem" +#define SPECIES_GOLEM_PLASTIC "plastic_golem" +#define SPECIES_GOLEM_BRONZE "bronze_golem" +#define SPECIES_GOLEM_CARDBOARD "cardboard_golem" +#define SPECIES_GOLEM_LEATHER "leather_golem" +#define SPECIES_GOLEM_DURATHREAD "durathread_golem" +#define SPECIES_GOLEM_BONE "bone_golem" +#define SPECIES_GOLEM_SNOW "snow_golem" +#define SPECIES_GOLEM_HYDROGEN "metallic_hydrogen_golem" diff --git a/code/__HELPERS/_lists.dm b/code/__HELPERS/_lists.dm index f95c429c70a..13aac6f6f96 100644 --- a/code/__HELPERS/_lists.dm +++ b/code/__HELPERS/_lists.dm @@ -315,6 +315,12 @@ . = L[L.len] L.len-- +/// Returns the top (last) element from the list, does not remove it from the list. Stack functionality. +/proc/peek(list/target_list) + var/list_length = length(target_list) + if(list_length != 0) + return target_list[list_length] + /proc/popleft(list/L) if(L.len) . = L[1] diff --git a/code/controllers/configuration/config_entry.dm b/code/controllers/configuration/config_entry.dm index 858f7f8073b..99d1c3270d3 100644 --- a/code/controllers/configuration/config_entry.dm +++ b/code/controllers/configuration/config_entry.dm @@ -199,43 +199,107 @@ return FALSE str_val = trim(str_val) - var/key_pos = findtext(str_val, splitter) - var/key_name = null - var/key_value = null - if(key_pos || value_mode == VALUE_MODE_FLAG) - key_name = copytext(str_val, 1, key_pos) - if(lowercase_key) - key_name = lowertext(key_name) - if(key_pos) - key_value = copytext(str_val, key_pos + length(str_val[key_pos])) - var/new_key - var/new_value - var/continue_check_value - var/continue_check_key - switch(key_mode) - if(KEY_MODE_TEXT) - new_key = key_name - continue_check_key = new_key - if(KEY_MODE_TYPE) - new_key = key_name - if(!ispath(new_key)) - new_key = text2path(new_key) - continue_check_key = ispath(new_key) - switch(value_mode) - if(VALUE_MODE_FLAG) - new_value = TRUE - continue_check_value = TRUE - if(VALUE_MODE_NUM) - new_value = text2num(key_value) - continue_check_value = !isnull(new_value) - if(VALUE_MODE_TEXT) - new_value = key_value - continue_check_value = new_value - if(continue_check_value && continue_check_key && ValidateListEntry(new_key, new_value)) - config_entry_value[new_key] = new_value - return TRUE + var/list/new_entry = parse_key_and_value(str_val) + + var/new_key = new_entry["config_key"] + var/new_value = new_entry["config_value"] + + if(!isnull(new_value) && !isnull(new_key) && ValidateListEntry(new_key, new_value)) + config_entry_value[new_key] = new_value + return TRUE return FALSE +/datum/config_entry/keyed_list/proc/parse_key_and_value(option_string) + // Blank or null option string? Bad mojo! + if(!option_string) + log_config("ERROR: Keyed list config tried to parse with no key or value data.") + return null + + var/list/config_entry_words = splittext(option_string, splitter) + var/config_value + var/config_key + var/is_ambiguous = FALSE + + // If this config entry's value mode is flag, the value can either be TRUE or FALSE. + // However, the config supports implicitly setting a config entry to TRUE by omitting the value. + // This value mode should also support config overrides disabling it too. + // The following code supports config entries as such: + // Implicitly enable the config entry: CONFIG_ENTRY config key goes here + // Explicitly enable the config entry: CONFIG_ENTRY config key goes here 1 + // Explicitly disable the config entry: CONFIG_ENTRY config key goes here 0 + if(value_mode == VALUE_MODE_FLAG) + var/value = peek(config_entry_words) + config_value = TRUE + + if(value == "0") + config_key = jointext(config_entry_words, splitter, length(config_entry_words) - 1) + config_value = FALSE + is_ambiguous = (length(config_entry_words) > 2) + else if(value == "1") + config_key = jointext(config_entry_words, splitter, length(config_entry_words) - 1) + is_ambiguous = (length(config_entry_words) > 2) + else + config_key = option_string + is_ambiguous = (length(config_entry_words) > 1) + // Else it has to be a key value pair and we parse it under that assumption. + else + // If config_entry_words only has 1 or 0 words in it and isn't value_mode == VALUE_MODE_FLAG then it's an invalid config entry. + if(length(config_entry_words) <= 1) + log_config("ERROR: Could not parse value from config entry string: [option_string]") + return null + + config_value = pop(config_entry_words) + config_key = jointext(config_entry_words, splitter) + + if(lowercase_key) + config_key = lowertext(config_key) + + is_ambiguous = (length(config_entry_words) > 2) + + config_key = validate_config_key(config_key) + config_value = validate_config_value(config_value) + + // If there are multiple splitters, it's definitely ambiguous and we'll warn about how we parsed it. Helps with debugging config issues. + if(is_ambiguous) + log_config("WARNING: Multiple splitter characters (\"[splitter]\") found. Using \"[config_key]\" as config key and \"[config_value]\" as config value.") + + return list("config_key" = config_key, "config_value" = config_value) + +/// Takes a given config key and validates it. If successful, returns the formatted key. If unsuccessful, returns null. +/datum/config_entry/keyed_list/proc/validate_config_key(key) + switch(key_mode) + if(KEY_MODE_TEXT) + return key + if(KEY_MODE_TYPE) + if(ispath(key)) + return key + + var/key_path = text2path(key) + if(isnull(key_path)) + log_config("ERROR: Invalid KEY_MODE_TYPE typepath. Is not a valid typepath: [key]") + return + + return key_path + + +/// Takes a given config value and validates it. If successful, returns the formatted key. If unsuccessful, returns null. +/datum/config_entry/keyed_list/proc/validate_config_value(value) + switch(value_mode) + if(VALUE_MODE_FLAG) + return value + if(VALUE_MODE_NUM) + if(isnum(value)) + return value + + var/value_num = text2num(value) + if(isnull(value_num)) + log_config("ERROR: Invalid VALUE_MODE_NUM number. Could not parse a valid number: [value]") + return + + return value_num + if(VALUE_MODE_TEXT) + return value + /datum/config_entry/keyed_list/vv_edit_var(var_name, var_value) return var_name != NAMEOF(src, splitter) && ..() diff --git a/code/controllers/configuration/entries/game_options.dm b/code/controllers/configuration/entries/game_options.dm index 6109f1bc781..5da4cde7132 100644 --- a/code/controllers/configuration/entries/game_options.dm +++ b/code/controllers/configuration/entries/game_options.dm @@ -1,12 +1,5 @@ /datum/config_entry/number_list/repeated_mode_adjust -/datum/config_entry/keyed_list/probability - key_mode = KEY_MODE_TEXT - value_mode = VALUE_MODE_NUM - -/datum/config_entry/keyed_list/probability/ValidateListEntry(key_name) - return key_name in config.modes - /datum/config_entry/keyed_list/max_pop key_mode = KEY_MODE_TEXT value_mode = VALUE_MODE_NUM @@ -106,10 +99,24 @@ key_mode = KEY_MODE_TEXT value_mode = VALUE_MODE_FLAG +/datum/config_entry/keyed_list/roundstart_races/ValidateListEntry(key_name, key_value) + if(key_name in GLOB.species_list) + return TRUE + + log_config("ERROR: [key_name] is not a valid race ID.") + return FALSE + /datum/config_entry/keyed_list/roundstart_no_hard_check // Species contained in this list will not cause existing characters with no-longer-roundstart species set to be resetted to the human race. key_mode = KEY_MODE_TEXT value_mode = VALUE_MODE_FLAG +/datum/config_entry/keyed_list/roundstart_no_hard_check/ValidateListEntry(key_name, key_value) + if(key_name in GLOB.species_list) + return TRUE + + log_config("ERROR: [key_name] is not a valid race ID.") + return FALSE + /datum/config_entry/flag/no_summon_guns //No /datum/config_entry/flag/no_summon_magic //Fun diff --git a/code/game/objects/structures/mirror.dm b/code/game/objects/structures/mirror.dm index c09ad90d476..151d25de69c 100644 --- a/code/game/objects/structures/mirror.dm +++ b/code/game/objects/structures/mirror.dm @@ -142,10 +142,9 @@ /obj/structure/mirror/magic/Initialize(mapload) . = ..() if(!choosable_races.len) - for(var/speciestype in subtypesof(/datum/species)) - var/datum/species/S = speciestype - if(initial(S.changesource_flags) & MIRROR_MAGIC) - choosable_races += initial(S.id) + for(var/datum/species/species_type as anything in subtypesof(/datum/species)) + if(initial(species_type.changesource_flags) & MIRROR_MAGIC) + choosable_races += initial(species_type.name) choosable_races = sort_list(choosable_races) /obj/structure/mirror/magic/lesser/Initialize(mapload) @@ -153,10 +152,9 @@ return ..() /obj/structure/mirror/magic/badmin/Initialize(mapload) - for(var/speciestype in subtypesof(/datum/species)) - var/datum/species/S = speciestype - if(initial(S.changesource_flags) & MIRROR_BADMIN) - choosable_races += initial(S.id) + for(var/datum/species/species_type as anything in subtypesof(/datum/species)) + if(initial(species_type.changesource_flags) & MIRROR_BADMIN) + choosable_races += initial(species_type.name) return ..() /obj/structure/mirror/magic/attack_hand(mob/user, list/modifiers) diff --git a/code/modules/mob/living/carbon/human/species_types/shadowpeople.dm b/code/modules/mob/living/carbon/human/species_types/shadowpeople.dm index 7cd12297123..b822cccdda0 100644 --- a/code/modules/mob/living/carbon/human/species_types/shadowpeople.dm +++ b/code/modules/mob/living/carbon/human/species_types/shadowpeople.dm @@ -1,6 +1,6 @@ /datum/species/shadow // Humans cursed to stay in the darkness, lest their life forces drain. They regain health in shadow and die in light. - name = "???" + name = "Shadow" id = SPECIES_SHADOW sexes = 0 meat = /obj/item/food/meat/slab/human/mutant/shadow diff --git a/code/modules/ruins/objects_and_mobs/sin_ruins.dm b/code/modules/ruins/objects_and_mobs/sin_ruins.dm index adf00faf67e..3bbe34e0cd0 100644 --- a/code/modules/ruins/objects_and_mobs/sin_ruins.dm +++ b/code/modules/ruins/objects_and_mobs/sin_ruins.dm @@ -112,10 +112,9 @@ icon_state = "magic_mirror" /obj/structure/mirror/magic/pride/New() - for(var/speciestype in subtypesof(/datum/species)) - var/datum/species/S = speciestype - if(initial(S.changesource_flags) & MIRROR_PRIDE) - choosable_races += initial(S.id) + for(var/datum/species/species_type as anything in subtypesof(/datum/species)) + if(initial(species_type.changesource_flags) & MIRROR_PRIDE) + choosable_races += initial(species_type.name) ..() /obj/structure/mirror/magic/pride/curse(mob/user) diff --git a/code/modules/unit_tests/_unit_tests.dm b/code/modules/unit_tests/_unit_tests.dm index 1c31a3553ea..c4cb3d07687 100644 --- a/code/modules/unit_tests/_unit_tests.dm +++ b/code/modules/unit_tests/_unit_tests.dm @@ -93,6 +93,7 @@ #include "siunit.dm" #include "spawn_humans.dm" #include "spawn_mobs.dm" +#include "species_config_sanity.dm" #include "species_whitelists.dm" #include "stomach.dm" #include "strippable.dm" @@ -103,7 +104,6 @@ #include "timer_sanity.dm" #include "unit_test.dm" #include "wizard.dm" - #ifdef REFERENCE_TRACKING //Don't try and parse this file if ref tracking isn't turned on. IE: don't parse ref tracking please mr linter #include "find_reference_sanity.dm" #endif diff --git a/code/modules/unit_tests/species_config_sanity.dm b/code/modules/unit_tests/species_config_sanity.dm new file mode 100644 index 00000000000..6bcc9d7cb20 --- /dev/null +++ b/code/modules/unit_tests/species_config_sanity.dm @@ -0,0 +1,22 @@ +/** + * Species IDs are used in keyed_list config entries and their config values can either be set implicitly or explicitly. + * + * In order to accomplish this, the keyed_list looks for a specific splitter that is meant to separate the key from the value. + * + * While it supports multiple instances of the splitter (for example, space) being present, the intent is ambiguous. + * + * To combat that, this unit test runs through every species ID and make sure it doesn't contain the splitter character, so + * valid config entries are never ambiguous. + */ +/datum/unit_test/species_config_sanity/Run() + var/datum/config_entry/keyed_list/roundstart_races/first_config_type = /datum/config_entry/keyed_list/roundstart_races + var/datum/config_entry/keyed_list/roundstart_no_hard_check/second_config_type = /datum/config_entry/keyed_list/roundstart_no_hard_check + + var/first_splitter = initial(first_config_type.splitter) + var/second_splitter = initial(second_config_type.splitter) + for(var/datum/species/species_type as anything in subtypesof(/datum/species)) + var/species_id = initial(species_type.id) + if(findtext(species_id, first_splitter)) + Fail("A species ID contained a config_entry splitter: [species_type] | Splitter: (\"[first_splitter]\") | Species ID: (\"[species_id]\")") + if(findtext(species_id, second_splitter)) + Fail("A species ID contained a config_entry splitter: [species_type] | Splitter: (\"[second_splitter]\") | Species ID: (\"[species_id]\")") diff --git a/config/game_options.txt b/config/game_options.txt index 027ba857da7..199914229be 100644 --- a/config/game_options.txt +++ b/config/game_options.txt @@ -305,6 +305,8 @@ SILICON_MAX_LAW_AMOUNT 12 ##------------------------------------------------------------------------------------------- ## Uncommenting races will allow them to be choosen at roundstart while join_with_muntant_race is on. You'll need at least one. +## See code/__DEFINES/DNA.dm for more options than the ones below. + ## You probably want humans on your space station, but technically speaking you can turn them off without any ill effect ROUNDSTART_RACES human @@ -319,16 +321,37 @@ ROUNDSTART_RACES plasmaman ## Races that are better than humans in some ways, but worse in others ROUNDSTART_RACES ethereal #ROUNDSTART_RACES jelly -#ROUNDSTART_RACES golem -#ROUNDSTART_RACES adamantine -#ROUNDSTART_RACES plasma -#ROUNDSTART_RACES diamond -#ROUNDSTART_RACES gold -#ROUNDSTART_RACES silver -#ROUNDSTART_RACES uranium #ROUNDSTART_RACES abductor #ROUNDSTART_RACES synth +## Including all the various golem subtypes that are better than humans in some ways, but worse in others +#ROUNDSTART_RACES iron_golem +#ROUNDSTART_RACES adamantine_golem +#ROUNDSTART_RACES plasma_golem +#ROUNDSTART_RACES diamond_golem +#ROUNDSTART_RACES gold_golem +#ROUNDSTART_RACES silver_golem +#ROUNDSTART_RACES uranium_golem +#ROUNDSTART_RACES plasteel_golem +#ROUNDSTART_RACES titanium_golem +#ROUNDSTART_RACES plastitanium_golem +#ROUNDSTART_RACES alloy_golem +#ROUNDSTART_RACES wood_golem +#ROUNDSTART_RACES sand_golem +#ROUNDSTART_RACES glass_golem +#ROUNDSTART_RACES bluespace_golem +#ROUNDSTART_RACES bananium_golem +#ROUNDSTART_RACES runic_golem +#ROUNDSTART_RACES cloth_golem +#ROUNDSTART_RACES plastic_golem +#ROUNDSTART_RACES bronze_golem +#ROUNDSTART_RACES cardboard_golem +#ROUNDSTART_RACES leather_golem +#ROUNDSTART_RACES durathread_golem +#ROUNDSTART_RACES bone_golem +#ROUNDSTART_RACES snow_golem +#ROUNDSTART_RACES metallic_hydrogen_golem + ## Races that are straight upgrades. If these are on expect powergamers to always pick them #ROUNDSTART_RACES skeleton #ROUNDSTART_RACES zombie diff --git a/tgui/packages/tgui/interfaces/PreferencesMenu/preferences/species/clothgolem.ts b/tgui/packages/tgui/interfaces/PreferencesMenu/preferences/species/cloth_golem.ts similarity index 100% rename from tgui/packages/tgui/interfaces/PreferencesMenu/preferences/species/clothgolem.ts rename to tgui/packages/tgui/interfaces/PreferencesMenu/preferences/species/cloth_golem.ts