Bans the use of icons and icon_state strings as overlay items; Adds a…

… note about abusing associated lists. (#58383)

Bans the use of icons and icon_state strings as overlay items.

Adds a note about using associated lists to cache things
Why:

Overlays get converted to appearances by byond on insert, so if we want to be able to do list math on the overlay lists, we have to do this conversion ourselves as well, otherwise -= "blah" wouldn't work as it won't be a string anymore.

This requires creating a new appearance every time an overlay operation happens for these use case, or search for cached versions of them in an associated list. images, appearances and mutable appearances can be converted into an appearance for free because they already are appearances

Associated lists have a lot of hidden overhead, leading to them to get abused for caching details that doesn't need to be cached. they are faster to search then normal lists, but slower then global, static, and proc vars, and very likely the same as datum vars, if not slower.

They have a higher per item memory usage then global vars (8b), proc vars (8b), datum vars (16b), or flat lists (8b) because they have to store the key twice, once in the flat array for for loops, and again in the tree structure along with the value.

.github/CONTRIBUTING.md

@@ -547,6 +547,91 @@ for (var/month in 1 to 12)
 for (var/i in reagents)
 ```
 
+### Icons are for image manipulation and defining an obj's `.icon` var, appearances are for everything else.
+BYOND will allow you to use a raw icon file or even an icon datum for underlays, overlays, and what not (you can even use strings to refer to an icon state on the current icon). The issue is these get converted by BYOND to appearances on every overlay insert or removal involving them, and this process requires inserting the new appearance into the global list of appearances, and informing clients about them.
+
+Converting them yourself to appearances and storing this converted value will ensure this process only has to happen once for the lifetime of the round. Helper functions exist to do most of the work for you.
+
+
+Bad:
+```dm
+/obj/machine/update_overlays(blah)
+	if (stat & broken)
+		add_overlay(icon(broken_icon))  //this icon gets created, passed to byond, converted to an appearance, then deleted.
+		return
+	if (is_on)
+		add_overlay("on") //also bad, the converstion to an appearance still has to happen
+	else
+		add_overlay(iconstate2appearance(icon, "off")) //this might seem alright, but not storing the value just moves the repeated appearance generation to this proc rather then the core overlay management. It would only be acceptable (and to some degree perferred) if this overlay is only ever added once (like in init code)
+```
+
+Good:
+```dm
+/obj/machine/update_overlays(var/blah)
+	var/static/on_overlay = iconstate2appearance(icon, "on")
+	var/static/off_overlay = iconstate2appearance(icon, "off")
+	var/static/broken_overlay = icon2appearance(broken_icon)
+	if (stat & broken)
+		add_overlay(broken_overlay) 
+		return
+	if (is_on)
+		add_overlay(on_overlay)
+	else
+		add_overlay(off_overlay)
+	...
+```
+
+Note: images are appearances with extra steps, and don't incur the overhead in conversion.
+
+
+### Do not abuse associated lists.
+Associated lists that could instead be variables or statically defined number indexed lists will use more memory, as associated lists have a 24 bytes per item overhead (vs 8 for lists and most vars), and are slower to search compared to static/global variables and lists with known indexes.
+
+
+Bad:
+```dm
+/obj/machine/update_overlays(var/blah)
+	var/static/our_overlays = list("on" = iconstate2appearance(overlay_icon, "on"), "off" = iconstate2appearance(overlay_icon, "off"), "broken" = iconstate2appearance(overlay_icon, "broken"))
+	if (stat & broken)
+		add_overlay(our_overlays["broken"]) 
+		return
+	...
+```
+
+Good:
+```dm
+#define OUR_ON_OVERLAY 1
+#define OUR_OFF_OVERLAY 2
+#define OUR_BROKEN_OVERLAY 3
+/obj/machine/update_overlays(var/blah)
+	var/static/our_overlays = list(iconstate2appearance(overlay_icon, "on"), iconstate2appearance(overlay_icon, "off"), iconstate2appearance(overlay_icon, "broken"))
+	if (stat & broken)
+		add_overlay(our_overlays[OUR_BROKEN_OVERLAY])
+		return
+	...
+
+#undef OUR_ON_OVERLAY
+#undef OUR_OFF_OVERLAY
+#undef OUR_BROKEN_OVERLAY
+```
+Storing these in a flat (non-associated) list saves on memory, and using defines to reference locations in the list saves CPU time searching the list.
+
+Also good:
+```dm
+/obj/machine/update_overlays(var/blah)
+	var/static/on_overlay = iconstate2appearance(overlay_icon, "on")
+	var/static/off_overlay = iconstate2appearance(overlay_icon, "off")
+	var/static/broken_overlay = iconstate2appearance(overlay_icon, "broken")
+	if (stat & broken)
+		add_overlay(broken_overlay)
+		return
+	...
+```
+Proc variables, static variables, and global variables are resolved at compile time, so the above is equivalent to the second example, but is easier to read, and avoids the need to store a list.
+
+Note: While there has historically been a strong impulse to use associated lists for caching of computed values, this is the easy way out and leaves a lot of hidden overhead. Please keep this in mind when designing core/root systems that are intended for use by other code/coders. It's normally better for consumers of such systems to handle their own caching using vars and number indexed lists, than for you to do it using associated lists.
+
+
 ### Other Notes
 * Code should be modular where possible; if you are working on a new addition, then strongly consider putting it in its own file unless it makes sense to put it with similar ones (i.e. a new tool would go in the "tools.dm" file)
 
@@ -564,6 +649,7 @@ for (var/i in reagents)
 
 * The dlls section of tgs3.json is not designed for dlls that are purely `call()()`ed since those handles are closed between world reboots. Only put in dlls that may have to exist between world reboots.
 
+
 #### Enforced not enforced
 The following coding styles are not only not enforced at all, but are generally frowned upon to change for little to no reason: