From 344d3b62664baac5ef48a9d7253aed8240bc7777 Mon Sep 17 00:00:00 2001 From: MichiRecRoom <1008889+LikeLakers2@users.noreply.github.com> Date: Wed, 5 Feb 2025 14:00:27 -0500 Subject: [PATCH] Optimizes `/proc/icon_exists()` (#89357) ## About The Pull Request This PR reimplements https://github.com/tgstation/tgstation/pull/71538 atop `master`. Quoting the original PR: > Every `icon_exists()` call will cache the entire file. Past me didn't realise _why_ file opts were so expensive, but I do now. This is immeasurably slower on a single call, and _significantly_ faster on subsequent calls to the same file. I attempted to handle some of the review comments that were posted there, by splitting screaming functionality into its own proc. * `if(icon_state in icon_states(file))` and `if(!(icon_state in icon_states(file)))` were refactored to use `icon_exists(file, icon_state)`. * Where screaming was seemingly wanted (and where there wasn't a more descriptive error inside the `if` block), I refactored them to use `icon_exists_or_scream(file, icon_state)` * The exception to the above was under `/datum/unit_test/turf_icons/Run()` and `/datum/unit_test/worn_icons/Run()`, where `icon_states()` was being passed a mode flag. Given that this is only used in unit tests (where performance isn't a priority), I opted to leave these be. Additionally, I revised the documentation comment for `/proc/icon_exists()`, as I felt it was a bit vague currently. ## Why It's Good For The Game https://youtu.be/Z9G1Mf6TZRs ## Changelog No player-facing changes (hopefully). --------- Co-authored-by: SyncIt21 <110812394+SyncIt21@users.noreply.github.com> --- code/__HELPERS/icons.dm | 43 +++++++++++-------- code/__HELPERS/lighting.dm | 2 +- code/controllers/subsystem/overlays.dm | 3 +- code/datums/greyscale/layer.dm | 3 +- .../objects/items/devices/chameleonproj.dm | 4 +- code/modules/admin/verbs/debug.dm | 26 +++++------ .../admin/view_variables/view_variables.dm | 2 +- code/modules/asset_cache/asset_list.dm | 2 +- code/modules/asset_cache/assets/crafting.dm | 2 +- .../asset_cache/assets/research_designs.dm | 4 +- .../living/carbon/human/human_update_icons.dm | 2 +- code/modules/surgery/bodyparts/_bodyparts.dm | 2 +- code/modules/unit_tests/suit_storage_icons.dm | 4 +- code/modules/unit_tests/turf_icons.dm | 8 ++-- 14 files changed, 56 insertions(+), 51 deletions(-) diff --git a/code/__HELPERS/icons.dm b/code/__HELPERS/icons.dm index 583db808773..9d9afc57bcb 100644 --- a/code/__HELPERS/icons.dm +++ b/code/__HELPERS/icons.dm @@ -1184,28 +1184,37 @@ GLOBAL_LIST_EMPTY(transformation_animation_objects) animate(pixel_x = initialpixelx + rand(-pixelshiftx,pixelshiftx), pixel_y = initialpixely + rand(-pixelshifty,pixelshifty), time = shake_interval) animate(pixel_x = initialpixelx, pixel_y = initialpixely, time = shake_interval) -///Checks if the given iconstate exists in the given file, caching the result. Setting scream to TRUE will print a stack trace ONCE. -/proc/icon_exists(file, state, scream) +/// Checks whether a given icon state exists in a given icon file. If `file` and `state` both exist, +/// this will return `TRUE` - otherwise, it will return `FALSE`. +/// +/// If you want a stack trace to be output when the given state/file doesn't exist, use +/// `/proc/icon_exists_or_scream()`. +/proc/icon_exists(file, state) var/static/list/icon_states_cache = list() - if(icon_states_cache[file]?[state]) - return TRUE + if(isnull(file) || isnull(state)) + return FALSE //This is common enough that it shouldn't panic, imo. - if(icon_states_cache[file]?[state] == FALSE) - return FALSE - - var/list/states = icon_states(file) - - if(!icon_states_cache[file]) + if(isnull(icon_states_cache[file])) icon_states_cache[file] = list() + for(var/istate in icon_states(file)) + icon_states_cache[file][istate] = TRUE - if(state in states) - icon_states_cache[file][state] = TRUE + return !isnull(icon_states_cache[file][state]) + +/// Functions the same as `/proc/icon_exists()`, but with the addition of a stack trace if the +/// specified file or state doesn't exist. +/// +/// Stack traces will only be output once for each file. +/proc/icon_exists_or_scream(file, state) + if(icon_exists(file, state)) return TRUE - else - icon_states_cache[file][state] = FALSE - if(scream) - stack_trace("Icon Lookup for state: [state] in file [file] failed.") - return FALSE + + var/static/list/screams = list() + if(!isnull(screams[file])) + screams[file] = TRUE + stack_trace("State [state] in file [file] does not exist.") + + return FALSE /** * Returns the size of the sprite in tiles. diff --git a/code/__HELPERS/lighting.dm b/code/__HELPERS/lighting.dm index 96087ba1737..a3d3522ebff 100644 --- a/code/__HELPERS/lighting.dm +++ b/code/__HELPERS/lighting.dm @@ -9,7 +9,7 @@ //Test to make sure emissives with broken or missing icon states are created if(PERFORM_ALL_TESTS(focus_only/invalid_emissives)) - if(icon_state && !icon_exists(icon, icon_state, scream = FALSE)) //Scream set to False so we can have a custom stack_trace + if(icon_state && !icon_exists(icon, icon_state)) //Scream set to False so we can have a custom stack_trace stack_trace("An emissive appearance was added with non-existant icon_state \"[icon_state]\" in [icon]!") return appearance diff --git a/code/controllers/subsystem/overlays.dm b/code/controllers/subsystem/overlays.dm index fda892be7a4..96041edd423 100644 --- a/code/controllers/subsystem/overlays.dm +++ b/code/controllers/subsystem/overlays.dm @@ -47,8 +47,7 @@ SUBSYSTEM_DEF(overlays) if (istext(overlay)) // This is too expensive to run normally but running it during CI is a good test if (PERFORM_ALL_TESTS(focus_only/invalid_overlays)) - var/list/icon_states_available = icon_states(icon) - if(!(overlay in icon_states_available)) + if(!icon_exists(icon, overlay)) var/icon_file = "[icon]" || "Unknown Generated Icon" stack_trace("Invalid overlay: Icon object '[icon_file]' [REF(icon)] used in '[src]' [type] is missing icon state [overlay].") continue diff --git a/code/datums/greyscale/layer.dm b/code/datums/greyscale/layer.dm index f12fb1992c9..6512a6c3d56 100644 --- a/code/datums/greyscale/layer.dm +++ b/code/datums/greyscale/layer.dm @@ -92,8 +92,7 @@ /datum/greyscale_layer/icon_state/Initialize(icon_file) . = ..() - var/list/icon_states = icon_states(icon_file) - if(!(icon_state in icon_states)) + if(!icon_exists(icon_file, icon_state)) CRASH("Configured icon state \[[icon_state]\] was not found in [icon_file]. Double check your json configuration.") icon = new(icon_file, icon_state) diff --git a/code/game/objects/items/devices/chameleonproj.dm b/code/game/objects/items/devices/chameleonproj.dm index fbdf3bae40a..5783b3c867f 100644 --- a/code/game/objects/items/devices/chameleonproj.dm +++ b/code/game/objects/items/devices/chameleonproj.dm @@ -77,9 +77,7 @@ saved_appearance = temp.appearance /obj/item/chameleon/proc/check_sprite(atom/target) - if(target.icon_state in icon_states(target.icon)) - return TRUE - return FALSE + return icon_exists(target.icon, target.icon_state) /obj/item/chameleon/proc/toggle(mob/user) if(!can_use || !saved_appearance) diff --git a/code/modules/admin/verbs/debug.dm b/code/modules/admin/verbs/debug.dm index fc054e9a9cb..0ca4072ff77 100644 --- a/code/modules/admin/verbs/debug.dm +++ b/code/modules/admin/verbs/debug.dm @@ -814,57 +814,57 @@ ADMIN_VERB(check_missing_sprites, R_DEBUG, "Debug Worn Item Sprites", "We're can continue //Is there an explicit worn_icon to pick against the worn_icon_state? Easy street expected behavior. if(sprite.worn_icon) - if(!(sprite.icon_state in icon_states(sprite.worn_icon))) + if(!icon_exists(sprite.worn_icon, sprite.icon_state)) to_chat(user, span_warning("ERROR sprites for [sprite.type]. Slot Flags are [sprite.slot_flags]."), confidential = TRUE) else if(sprite.worn_icon_state) if(sprite.slot_flags & ITEM_SLOT_MASK) actual_file_name = 'icons/mob/clothing/mask.dmi' - if(!(sprite.worn_icon_state in icon_states(actual_file_name))) + if(!icon_exists(actual_file_name, sprite.worn_icon_state)) to_chat(user, span_warning("ERROR sprites for [sprite.type]. Mask slot."), confidential = TRUE) if(sprite.slot_flags & ITEM_SLOT_NECK) actual_file_name = 'icons/mob/clothing/neck.dmi' - if(!(sprite.worn_icon_state in icon_states(actual_file_name))) + if(!icon_exists(actual_file_name, sprite.worn_icon_state)) to_chat(user, span_warning("ERROR sprites for [sprite.type]. Neck slot."), confidential = TRUE) if(sprite.slot_flags & ITEM_SLOT_BACK) actual_file_name = 'icons/mob/clothing/back.dmi' - if(!(sprite.worn_icon_state in icon_states(actual_file_name))) + if(!icon_exists(actual_file_name, sprite.worn_icon_state)) to_chat(user, span_warning("ERROR sprites for [sprite.type]. Back slot."), confidential = TRUE) if(sprite.slot_flags & ITEM_SLOT_HEAD) actual_file_name = 'icons/mob/clothing/head/default.dmi' - if(!(sprite.worn_icon_state in icon_states(actual_file_name))) + if(!icon_exists(actual_file_name, sprite.worn_icon_state)) to_chat(user, span_warning("ERROR sprites for [sprite.type]. Head slot."), confidential = TRUE) if(sprite.slot_flags & ITEM_SLOT_BELT) actual_file_name = 'icons/mob/clothing/belt.dmi' - if(!(sprite.worn_icon_state in icon_states(actual_file_name))) + if(!icon_exists(actual_file_name, sprite.worn_icon_state)) to_chat(user, span_warning("ERROR sprites for [sprite.type]. Belt slot."), confidential = TRUE) if(sprite.slot_flags & ITEM_SLOT_SUITSTORE) actual_file_name = 'icons/mob/clothing/belt_mirror.dmi' - if(!(sprite.worn_icon_state in icon_states(actual_file_name))) + if(!icon_exists(actual_file_name, sprite.worn_icon_state)) to_chat(user, span_warning("ERROR sprites for [sprite.type]. Suit Storage slot."), confidential = TRUE) else if(sprite.icon_state) if(sprite.slot_flags & ITEM_SLOT_MASK) actual_file_name = 'icons/mob/clothing/mask.dmi' - if(!(sprite.icon_state in icon_states(actual_file_name))) + if(!icon_exists(actual_file_name, sprite.icon_state)) to_chat(user, span_warning("ERROR sprites for [sprite.type]. Mask slot."), confidential = TRUE) if(sprite.slot_flags & ITEM_SLOT_NECK) actual_file_name = 'icons/mob/clothing/neck.dmi' - if(!(sprite.icon_state in icon_states(actual_file_name))) + if(!icon_exists(actual_file_name, sprite.icon_state)) to_chat(user, span_warning("ERROR sprites for [sprite.type]. Neck slot."), confidential = TRUE) if(sprite.slot_flags & ITEM_SLOT_BACK) actual_file_name = 'icons/mob/clothing/back.dmi' - if(!(sprite.icon_state in icon_states(actual_file_name))) + if(!icon_exists(actual_file_name, sprite.icon_state)) to_chat(user, span_warning("ERROR sprites for [sprite.type]. Back slot."), confidential = TRUE) if(sprite.slot_flags & ITEM_SLOT_HEAD) actual_file_name = 'icons/mob/clothing/head/default.dmi' - if(!(sprite.icon_state in icon_states(actual_file_name))) + if(!icon_exists(actual_file_name, sprite.icon_state)) to_chat(user, span_warning("ERROR sprites for [sprite.type]. Head slot."), confidential = TRUE) if(sprite.slot_flags & ITEM_SLOT_BELT) actual_file_name = 'icons/mob/clothing/belt.dmi' - if(!(sprite.icon_state in icon_states(actual_file_name))) + if(!icon_exists(actual_file_name, sprite.icon_state)) to_chat(user, span_warning("ERROR sprites for [sprite.type]. Belt slot."), confidential = TRUE) if(sprite.slot_flags & ITEM_SLOT_SUITSTORE) actual_file_name = 'icons/mob/clothing/belt_mirror.dmi' - if(!(sprite.icon_state in icon_states(actual_file_name))) + if(!icon_exists(actual_file_name, sprite.icon_state)) to_chat(user, span_warning("ERROR sprites for [sprite.type]. Suit Storage slot."), confidential = TRUE) #ifndef OPENDREAM diff --git a/code/modules/admin/view_variables/view_variables.dm b/code/modules/admin/view_variables/view_variables.dm index fbebccd4459..58c98bb114a 100644 --- a/code/modules/admin/view_variables/view_variables.dm +++ b/code/modules/admin/view_variables/view_variables.dm @@ -54,7 +54,7 @@ ADMIN_VERB_AND_CONTEXT_MENU(debug_variables, R_NONE, "View Variables", "View the else // it means: icon_state="" if(!dmi_nullstate_checklist[icon_filename_text]) dmi_nullstate_checklist[icon_filename_text] = ICON_STATE_CHECKED - if("" in icon_states(image_object.icon)) + if(icon_exists(image_object.icon, "")) // this dmi has nullstate. We'll allow "icon_state=null" to show image. dmi_nullstate_checklist[icon_filename_text] = ICON_STATE_NULL diff --git a/code/modules/asset_cache/asset_list.dm b/code/modules/asset_cache/asset_list.dm index 39e9cf925da..48e5e7e39a4 100644 --- a/code/modules/asset_cache/asset_list.dm +++ b/code/modules/asset_cache/asset_list.dm @@ -392,7 +392,7 @@ GLOBAL_LIST_EMPTY(asset_datums) /datum/asset/spritesheet/proc/queuedInsert(sprite_name, icon/I, icon_state="", dir=SOUTH, frame=1, moving=FALSE) #ifdef UNIT_TESTS - if (I && icon_state && !(icon_state in icon_states(I))) // check the base icon prior to extracting the state we want + if (I && icon_state && !icon_exists(I, icon_state)) // check the base icon prior to extracting the state we want stack_trace("Tried to insert nonexistent icon_state '[icon_state]' from [I] into spritesheet [name] ([type])") return #endif diff --git a/code/modules/asset_cache/assets/crafting.dm b/code/modules/asset_cache/assets/crafting.dm index 99088ed1a70..1d6b9469d1f 100644 --- a/code/modules/asset_cache/assets/crafting.dm +++ b/code/modules/asset_cache/assets/crafting.dm @@ -40,7 +40,7 @@ icon_state ||= initial(preview_item.icon_state_preview) || initial(preview_item.icon_state) if(PERFORM_ALL_TESTS(focus_only/bad_cooking_crafting_icons)) - if(!icon_exists(icon_file, icon_state, scream = TRUE)) + if(!icon_exists_or_scream(icon_file, icon_state)) return Insert("a[id]", icon(icon_file, icon_state, SOUTH)) diff --git a/code/modules/asset_cache/assets/research_designs.dm b/code/modules/asset_cache/assets/research_designs.dm index cf56d33ddbb..6f9c96e5496 100644 --- a/code/modules/asset_cache/assets/research_designs.dm +++ b/code/modules/asset_cache/assets/research_designs.dm @@ -15,7 +15,7 @@ icon_file = initial(path.research_icon) icon_state = initial(path.research_icon_state) if (PERFORM_ALL_TESTS(focus_only/invalid_research_designs)) - if(!(icon_state in icon_states(icon_file))) + if(!icon_exists(icon_file, icon_state)) stack_trace("design [path] with icon '[icon_file]' missing state '[icon_state]'") continue I = icon(icon_file, icon_state, SOUTH) @@ -48,7 +48,7 @@ icon_state = initial(item.icon_state) if (PERFORM_ALL_TESTS(focus_only/invalid_research_designs)) - if(!(icon_state in icon_states(icon_file))) + if(!icon_exists(icon_file, icon_state)) stack_trace("design [path] with icon '[icon_file]' missing state '[icon_state]'") continue I = icon(icon_file, icon_state, SOUTH) diff --git a/code/modules/mob/living/carbon/human/human_update_icons.dm b/code/modules/mob/living/carbon/human/human_update_icons.dm index 27c78ee932c..9b743f95901 100644 --- a/code/modules/mob/living/carbon/human/human_update_icons.dm +++ b/code/modules/mob/living/carbon/human/human_update_icons.dm @@ -104,7 +104,7 @@ There are several things that need to be remembered: //icon_file MUST be set to null by default, or it causes issues. //handled_by_bodyshape MUST be set to FALSE under the if(!icon_exists()) statement, or everything breaks. //"override_file = handled_by_bodyshape ? icon_file : null" MUST be added to the arguments of build_worn_icon() - //Friendly reminder that icon_exists(file, state, scream = TRUE) is your friend when debugging this code. + //Friendly reminder that icon_exists_or_scream(file, state) is your friend when debugging this code. var/handled_by_bodyshape = TRUE var/icon_file var/woman diff --git a/code/modules/surgery/bodyparts/_bodyparts.dm b/code/modules/surgery/bodyparts/_bodyparts.dm index 2d1b3570e3d..2b6f111b2eb 100644 --- a/code/modules/surgery/bodyparts/_bodyparts.dm +++ b/code/modules/surgery/bodyparts/_bodyparts.dm @@ -1058,7 +1058,7 @@ else limb.icon_state = "[limb_id]_[body_zone]" - icon_exists(limb.icon, limb.icon_state, TRUE) //Prints a stack trace on the first failure of a given iconstate. + icon_exists_or_scream(limb.icon, limb.icon_state) //Prints a stack trace on the first failure of a given iconstate. . += limb diff --git a/code/modules/unit_tests/suit_storage_icons.dm b/code/modules/unit_tests/suit_storage_icons.dm index 7cc987bb468..543db168851 100644 --- a/code/modules/unit_tests/suit_storage_icons.dm +++ b/code/modules/unit_tests/suit_storage_icons.dm @@ -35,12 +35,12 @@ continue if(worn_icon) //easiest to check since we override everything. - if(!(icon_state in icon_states(worn_icon))) + if(!icon_exists(worn_icon, icon_state)) log_test("\t[count] - [item_path] using invalid [worn_icon_state ? "worn_icon_state" : "icon_state"], \"[icon_state]\" in worn_icon override file, '[worn_icon]'") count++ continue - if(!(icon_state in icon_states('icons/mob/clothing/belt_mirror.dmi'))) + if(!icon_exists('icons/mob/clothing/belt_mirror.dmi', icon_state)) already_warned_icons += icon_state log_test("\t[count] - [item_path] using invalid [worn_icon_state ? "worn_icon_state" : "icon_state"], \"[icon_state]\"") count++ diff --git a/code/modules/unit_tests/turf_icons.dm b/code/modules/unit_tests/turf_icons.dm index 6f371172898..945130ada0c 100644 --- a/code/modules/unit_tests/turf_icons.dm +++ b/code/modules/unit_tests/turf_icons.dm @@ -9,7 +9,7 @@ var/icon_file = initial(turf_path.icon) if(isnull(icon_state) || isnull(icon_file)) continue - if(!(icon_state in icon_states(icon_file))) + if(!icon_exists(icon_file, icon_state)) TEST_FAIL("[turf_path] using invalid icon_state - \"[icon_state]\" in icon file, '[icon_file]") for(var/turf/closed/mineral/turf_path as anything in typesof(/turf/closed/mineral)) //minerals use a special (read: snowflake) MAP_SWITCH definition that changes their icon based on if we're just compiling or if we're actually PLAYING the game. @@ -18,7 +18,7 @@ var/icon_file = initial(turf_path.icon) if(isnull(icon_state) || isnull(icon_file)) continue - if(!(icon_state in icon_states(icon_file))) + if(!icon_exists(icon_file, icon_state)) if(modular_mineral_turf_file && (icon_state in icon_states(modular_mineral_turf_file, 1))) continue if(!(icon_state in icon_states('icons/turf/mining.dmi', 1))) @@ -45,13 +45,13 @@ var/list/burnt_states = instanced_turf.burnt_states() for(var/state in burnt_states) - if(!(state in icon_states(damaged_dmi))) + if(!icon_exists(damaged_dmi, state)) TEST_FAIL("[open_turf_path] has an invalid icon in burnt_states - \"[state]\", in '[damaged_dmi]'") var/list/broken_states = instanced_turf.broken_states() for(var/state in broken_states) - if(!(state in icon_states(damaged_dmi))) + if(!icon_exists(damaged_dmi, state)) TEST_FAIL("[open_turf_path] has an invalid icon in broken_states - \"[state]\", in '[damaged_dmi]'") run_loc_floor_bottom_left = run_loc_floor_bottom_left.ChangeTurf(initial_turf_type) //cleanup.