Allows vv investigate /appearance + better checking image

[EvilDragonfiend]

About The Pull Request

• Allows vv investigate /appearance
• Makes checking images in vv editor
• Originally from VV editor can now see /appearance types BeeStation/BeeStation-Hornet#10885 (port-ish)

VV editor can now see /appearance types
These ones are kinda blackboxed, and we have no idea what these are in general.
I made it investigatable, but because of the nature of /appearance, the code is super janky, but it's inevitable curse.

I left an important comment inside:

Comment

< OH MY GOD. Can't you just make "/image/proc/foo()" instead of making these? >
	/appearance is a hardcoded byond type, and it is very internal type.
	Its type is actually /image, but it isn't truly /image. We defined it as "/appearance"
	new procs to /image will only work to actual /image references, but...
	/appearance references are not capable of executing procs, because these are not real /image
	This is why these global procs exist. Welcome to the curse.

Why It's Good For The Game

Debugging tool is good for us

Testing evidence photos

click to see images

/image types no longer show first image incorrectly when its icon_state is null
This means it won't render first image (medical status) from hud.dmi when you investigate hud image in hud_list of a mob

You can see /appearance types are merged within.

APC example

Mob example

no vv option, because it's dangerous

/image and /alternative_appearance shows the same format

full variables list (excluding blacklisted ones)

Changelog

🆑
code: /appearance type references are now investigatable in vv editor.
code: /image references display a summarised name with its dmi file name and icon_state when it's investigated from a datum where it uses. Format is: "mob_hud_image = /image (hud.dmi:medical) [0x00000]"
code: /image types no longer show first image incorrectly when its icon_state is null
/:cl:

[san7890]

hey thanks for all the images since they do help but in the future (or you can edit your PR body right now) can you use the dropdown "spoiler" function that Github Markdown supports? to use it you just gotta use this format:

<details>
<summary>Text for spoiler </summary>

Spoilered content
</details>

Then you can have a nice dropdown

and put all your photos in here

[san7890]

my knowledge on how appearances and image byond fuckary is quite limited so i'll take your word on it (i've read a few MrStonedOne rants about it) but we can definitely make your approach a lot better by using a few more tools.

[san7890]

@EvilDragonfiend there's nothing dangerous because if anything undocumented breaks it or changes will break on a BYOND-version upgrade (when things are expected to break anyways) and it is vitally important to know if something is broken rather than cover it up, because you can't fix things that you don't know are broken. it's going to be slightly uglier to read, that is true, but i prefer utility over elegance in cases like these. your worries with NAMEOF() are also unwarranted, look at the documentation (unless this is the "wrong answer" you were talking about):

/**
 * NAMEOF: Compile time checked variable name to string conversion
 * evaluates to a string equal to "X", but compile errors if X isn't a var on datum.
 * datum may be null, but it does need to be a typed var.
 **/
#define NAMEOF(datum, X) (#X || ##datum.##X)

it compiles to the equivalent of a string! it's just important to assert that what we know exists works, and then fix it when it stops working.

also, if you don't like the length of a statement in a switch chain using these macros, you can just do:

#define APPEARANCE_NAMEOF(whatever, x) // code here
#define A_NAMEOF(whatever, x) APPEARANCE_NAMEOF(##whatever, ##x)

you can do APP_NAMEOF instead but it's nice to have the full name writen out and then have another macro define wrapper around it, we already do a similar pattern like this with REM/REAGENTS_EFFECT_MULTIPLIER

[san7890]

if it doesn't work it's fine but i'd like to at least try. we can also put these lists as a global or something and unit-test it too (even though vars doesn't work we can try)

[EvilDragonfiend]

aight I will try

[san7890]

i looked over at my screen and saw this so i figured i'd just mention it right now

[EvilDragonfiend]

ah, actually not possible to use NAMEOF in switch because..

it should be /atom/movable/appearance to make it work, but we know /appearance is obviously not from /movable.
Bee headcoer, Bacon, didn't like that I was using /atom/movable/appearance because of the said reason, and suggested using : unsafe access.

So, again, would it be a good idea here implying var/atom/movable/appearance here..?

[EvilDragonfiend]

So cursed lmao

[EvilDragonfiend]

Open Dream doesn't like it.

[san7890]

mouse_drop_zone is interesting that opendream doesn't have it implemented yet but it's valid in DM, you can change that one to a generic string and put a comment regarding it for the time being. i would recommend making an issue report notifying opendream that this variable exists here: https://github.com/OpenDreamProject/OpenDream/issues (and link to the DM ref if applicable). they probably won't be able to implement it in its entirety but it's good to have it on their tracker

they're able to add anything that they know of that exists in valid DM but isn't supported yet in OpenDream at the very least (a list of unimplemented syntax) that will let it bypass the lints we run on it in this repository

[san7890]

anyways having as much NAMEOF() coverage as possible is fine, we'll work within our limitations and use raw strings otherwise.

[EvilDragonfiend]

I think I sorted out all now

[san7890]

@EvilDragonfiend If you really think we need the try/catch pattern then split out the entire switch() chain into a new proc because otherwise the indentation is just too ugly for a mile-long proc (the number of switch/if statements obfuscates the true nature of the proc), and then invoke that in the try block. so

/proc/proc_a(variable_name)
 try
  value = locate_variable(variable_name)
 catch
  //whatever

/proc/locate_variable(variable_name)
 switch(variable_name)
  if(x)
   return whatever

we do incur a little bit more vertical footprint but it's worth it to lessen the amount of horizontal footprint (which is far uglier). also the locate_variable() proc is more re-usable in case that matters in the future.

[san7890]

sorry for keeping you in limbo for a bit, got busy and was puzzled after I saw some snippets from the Beestation review (specifically the one about aliasing for /atom/movable here. i think i've figured out something that might work though, parent_type.

basically what we do is we do something like client_interface where we have a representative datum to "mock" out the potential extant vars on an appearance and access that instead of having the confusing movable stuff be involved here. what we would do is just

/// An alias datum that allows us to access and view the variables of an appearance by keeping certain known, yet undocumented, variables that we can access and read in a datum for debugging purposes.
/// Kindly do not use this outside of a debugging context.
/datum/appearance_data
 parent_type = /atom/movable // This is necessary to access the variables on compile-time.

Then, we can also potentially resolve some of the OD issues like NAMEOF() and mouse_drop_zone by declaring it as a var on this datum. It's a really hacky alias technique that came to me in a dream a few nights ago. Even if we can't resolve the NAMEOF() stuff not working, it'll be slightly better than what we're presently doing instead of treating an appearance like a movable atom. Just test this out for me and it'll be good to ship.

[EvilDragonfiend]

That's actually a good idea, but note that I'll do that differently in a few points:

• Replacing for(var/each in dummy_image.vars) into this is bad here because it will copy all atom/movable vars and we need to remove manually a lot.
• I'd do /image/appearance instead of /datum/appearance_data to mock more obvious.
• mouse_drop_zone already exists, OD just doesn't like it. (unless we have a define like #ifdef OPENDREAM?)
  and it looks we have it

[san7890]

That's fine

[EvilDragonfiend]

Okay, pretty looks good, and it looks OD likes it too

[san7890]

It's fine in its current state and I hope we can continue to iterate and improve this framework

[LemonInTheDark]

for the record, the way this actually works is very dumb
appearances carry vis_flags, but you can't read them directly off
what you CAN do is copy the appearance onto a MOVABLE, and read the vis_flags off THAT. v stupid am sorry

[LemonInTheDark]

        var/static/atom/movable/pluto = new() // all alone at the end of the universe
        pluto.appearance = appearance_father // We copy over our appearance onto an atom. This is done so we can read vars carried by but not accessible on appearances
        var/vis_flags = pluto.vis_flags

like this ya feel? (I am assuming you're keeping something of this form downstream somewhere, this'll make that less hellish)

[EvilDragonfiend]

still hellish you say, but thank you for the advice.
my coder boss will decide what'd be best anyway.