[~March 13?] drawmugshot ignores specification

[Matt]

To produce: get Hideous Destructor, go to automap, set gender to male or female.

Expected (and in prior versions): Setting gender as female will show FemDoom "SFF" mugshot.

Now: No matter what you set it to, the doomguy "STF" mugshot appears.

[_mental_]

In which version exactly it worked?
The first version that loads hd.pk7 from the given hd20160314a.zip without fatal errors is zdoom-2.9pre-319-g44a6caf.
In this version however setting a gender to female has no effect on mugshot.

[Matt]

In principle this attached file should work identically.

I've commented out the conditional stuff, and left in only the SFF - and it still shows the STF set.

EDIT: Tried adding the SBARINFO right into the wad itself, since that seems to be a relevant issue with TEXTURES. No difference.

EDIT2: Removed attachment

[_mental_]

I tried your test file with ZDoom 2.7.1 and STF mugshot was displayed.
From what I see in the code, skin's face have precedence over a face from status bar info.
So the question from my previous post still persists: in which version it worked?

[Graf Zahl]

I tested this all versions down to 2.2.0 and always got the Doomguy mugshot. Since this thing has no distinctive marks I cannot say what is wrong here, but this test appears to be useless.
Next time please verify that you got it working AS INTENDED on an older version and then name that version.

[Matt]

Problem is that it had always worked until some time within the last few months when someone finally noticed it wasn't working in the most recent GZDoom release. (I have no reason to think this would be a GZDoom-specific bug but it's not impossible now that I think about it. I can find nothing on a GZDoom forum search for mugshot though.)

Fortunately there happens to be video evidence so at least I can prove on this thread I'm not talking out of my ass. I've sent Slydir a PM asking if he could recall which version this was.

[Graf Zahl]

So, is this "Hack broke" or "SBARINFO broke"?
Just looking at the mechanism this uses to decide gender makes be go "Ugh..."

As for "it always worked", has it ever occured to you that this information is essential in tracking down the cause?
All I can do now is pick around in the messed up commit tree to find something. Git bisect won't work here.

[Graf Zahl]

That said, I think the implementation of the drawmugshot command is utterly idiotic. If the mod specifies a name that should take precedence over some skin setting.


To sum it up, the following seems to happen:

Mugshot code prefers skin over everything else, unconditionally.
Every player got a skin, and if it's the default one.
Every skin got a mugshot assigned, it's normally STF.

So whatever you specify, it never gets through. Looks broken by design to me, like a large portion of the player class and skin code. It's a hopelessly messed up affair.

The only relevant thing I could find was this commit:

* - Fixed: Skin mugshots didn't load. (This adds a texture usetype for skin graphics.)

If that is indeed the cause here some serious refactoring of a broken feature is in order.


from 2013.

[randi]

In principle this attached file should work identically.

Do you still have any old versions of Hideous Destructor that can be loaded with old ZDooms?

[Matt]

Thanks for the help in tracking this. It's looking more like "Hack broke" at this point given Graf Zahl's description of how the current setup works, though it's a mystery how it could have ever worked for so long to begin with. Looking at the changelog for anything involving SBARINFO, mugshots or inventory doesn't bring up anything that could reasonably have affected this. [EDIT: could this be it?]


The video is from a version that is definitely later than 2013. The mugshot was first implemented in October 2013, so it's after that change.


It seems that my web host caches old files, so...

I recall updating (G)ZDoom to the most recent SVN in August 2015, and it worked then, so July 20, 2015 should be old enough.

That said, here's October 28, 2013 when the feature was first added.

The ACS script should automatically give the relevant item once you set the player's gender to male or female.

[Blzut3]

So the default face parameter has been deprecated for a long long time (basically since 2008) because it has been in a state of flux of working and not working ever since the beginning. (Between skins and later player classes and morphs. Pretty much every time that code was touched it broke something using that parameter.) Which wouldn't be too horrible except I'm not sure that between release versions of ZDoom the semantics of that parameter has been consistent and I'm certain there are mods out there that specify the parameter even though they don't want it forced. My natural inclination at the moment is to just outright ignore that parameter and add a new one that is specifically for forcing a prefix, but I could use more opinions there.

[randi]

The July 20, 2015 version of Hideous Destructor works as intended with 2.8.1, but fmdmtest does not, which means that they don't work identically. After the scripting branch got merged in, it was pretty broken all around, but the first commit I found where a face was displayed at all showed the marine's. So apparently something happened in the scripting branch that changed it, but since Hideous Destructor is broken for a huge chunk of that, I can't really use it for any sort of testing. So maybe it was exploiting undefined behavior, or perhaps something important changed that needs to be repaired. I can't tell.

If you could redo fmdmtest and ensure that it works on 2.8.1, that would probably help.

[Graf Zahl]

Actually yes, it changed something very important:

Before the scripting branch, the Face property was metadata. Afterward it became an FString in PClassPlayerPawn.
The problem is: The DECORATE parser complained about empty mugshots but assigned the value anyway, so the classes actually did have a name, albeit an invalid one.

And the skin init code only checked for NULL, not empty strings.

With the transition to metaclasses and this turning into an FString these two states became identical.
So this only ever worked because a bug was exploited - one that got a message in the console btw.!

That the feature as such is pretty much broken right now is another story.

[Matt]

This should replicate all the necessary logic that was in the HD implementation.
1. 2 items to be checked, both inheriting from an "InventoryFlag"
2. Looping ACS enter script that constantly checks the player's gender, then gives or takes items depending
3. SBARINFO that draws one or the other depending on the inventory item

[Graf Zahl]

There's no need for it anymore.
As I said, you were exploiting a bug to achieve something the function was not supposed to do.
What I think this really needs is an alternative DrawMugshot function that draws the specified graphic no matter what but I'd prefer to get some input from Blzut3 first.