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>
This commit is contained in:
MichiRecRoom
2025-02-05 20:00:27 +01:00
committed by GitHub
co-authored by SyncIt21
parent 465f654548
commit 344d3b6266
14 changed files with 56 additions and 51 deletions
+26 -17
View File
@@ -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.
+1 -1
View File
@@ -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
+1 -2
View File
@@ -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
+1 -2
View File
@@ -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)
@@ -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)
+13 -13
View File
@@ -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
@@ -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
+1 -1
View File
@@ -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
+1 -1
View File
@@ -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))
@@ -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)
@@ -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
+1 -1
View File
@@ -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
@@ -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++
+4 -4
View File
@@ -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.