ZDoom

[quote="Leonard2"][quote="Graf Zahl"]I nixed moving this code to the other function[/quote]
You have yet to reply to my question and explain this sudden change of mind.
I made this function [u]exactly[/u] for this purpose (handling the old layers behavior), moved everything needed for both the weapon and targeter layers there while you told both MC and I that that's what you wanted rather than having checks scattered all over.
And now suddenly this should be done elsewhere. Why?[/quote]
I too am curious about this.

[quote="Graf Zahl"]I nixed moving this code to the other function[/quote]
You have yet to reply to my question and explain this sudden change of mind.
I made this function [u]exactly[/u] for this purpose (handling the old layers behavior), moved everything needed for both the weapon and targeter layers there while you told both MC and I that that's what you wanted rather than having checks scattered all over.
And now suddenly this should be done elsewhere. Why?

[quote="Graf Zahl"]which renders the removal of the checks ineffective[/quote]
The checks I removed are useless no matter where the flags/offset are changed.

And as I have said, I nixed moving this code to the other function which renders the removal of the checks ineffective. And since there was a conflict with the actual fix and these changes I closed the PR.

[url=https://a.pomf.cat/fwkvvp.webm]Recording showing the crash[/url].
[attachment=0]CrashReport.zip[/attachment]

As for A_ItBurnsItBurns, all destroy does is unlink the psprite and flag it as a dead dobject.
Regardless of this, you can see in the second commit (which removed the checks) the line was moved after the flags/offset change anyways and then those were moved elsewhere by the next commit.

			psp->SetState(self->FindState("FireHands"));
			psp->Flags &= PSPF_ADDWEAPON | PSPF_ADDBOB;
			psp->y = WEAPONTOP;

Sorry, but let me be clear: I could always reproduce the crash before my commit and I never managed to reproduce it after my commit. With that test pk3, it should show nothing, of course.

Regarding the changes to A_ItBurnsItBurns, in the previous version:

[code]
psp->SetState(self->FindState("FireHands"));
psp->Flags &= PSPF_ADDWEAPON | PSPF_ADDBOB;
psp->y = WEAPONTOP;
[/code]

when 'self->FindState("FireHands")' returns NULL, in DPSprite::SetState 'psp' would be Destroy()ed. Is it really okay that the 'psp' object can still be accessed and modified like with psp->Flags and psp->y?

I did the same thing but like you said it doesn't crash all the time. I had to try multiple times to make it crash before your commit.
Basically what happens is that A_CrispyPlayer sets the state to garbage data and sometimes it crashes sometimes it doesn't and the strifehands layer is left hanging forever.
You can check for yourself by using "stat psprites" you'll notice the layer is present but you can't see anything.
Of course I fixed this in the last submission I made.

If you don't believe me I could record it and give you the resulting crash report.

[quote="Leonard2"]While you're right that A_CrispyPlayer with test.pk3 crashes before your commit, it also crashes after it.[/quote]

Interestingly, I couldn't make it crash after it, so there's something wrong here, like how did you reproduce the crash :P . I used 'give all', opened the grenade launcher with fire grenades and fired so that the player burns out.

Your log indicates that the crash was from A_CrispyPlayer. While you're right that A_CrispyPlayer with test.pk3 crashes before your commit, it also crashes after it.
Graf was referring to the extra checks in A_ItBurnsItBurns instead, I left the ones you added to A_CrispyPlayer.

Well, the test.pk3 file did crash before my commit. Attached the proof. Also, please next time give a clearer name to the wads (I renamed it to test_bad_strifehands.pk3)!

[quote="Graf Zahl"]It WILL crash if someone clears the Firehands states off a child class.[/quote]
I tested this myself even before I commited this.
See for yourself:
[attachment=0]test.pk3[/attachment]
The new player class clears out FireHands.
It doesn't crash and the strifehands layer is properly destroyed.
I'm wondering if you even bothered to read the commit messages I made..

[quote="Graf Zahl"]It's the second time that you reverted code that got added by someone else without thinking it through.[/quote]
You admitted yourself the first time that your change wasn't needed. All it does codewise is a simple syntax change from "GetPSprite()->SetState()" to "P_SetPsprite()".
Even for debugging, if all you want is to debug specific layers before setstate is called then what's wrong with just setting the breakpoint in GetPSprite and checking the layer there?

[quote="Graf Zahl"]And what's it with shoving all code into the generic functions? No, that's not the way to do it! I very intentionally set the position info for STRIFEHANDS outside of the SetState function and I am going to remove more such special cases when I find the time.[/quote]
What? I didn't even touch SetState..
GetPSprite is far from being a generic function (again it's only intended to be used with the old layers) and neither are the strife action functions.
Besides, you were the one complaining about having specific checks all over. MC even asked you if what you wanted is to have them all in P_SetPsprite (which was remplaced by GetPSprite in my submission) and even went as far as to make a pull request about it.
Why suddenly change your mind like that? Just to make it harder for people doing submissions?

Why did you 'remove the unneeded checks' that were deliberately added?
It's the second time that you reverted code that got added by someone else without thinking it through.

As a reminder: It WILL crash if someone clears the Firehands states off a child class.

And what's it with shoving all code into the generic functions? No, that's not the way to do it! I very intentionally set the position info for STRIFEHANDS outside of the SetState function and I am going to remove more such special cases when I find the time.

[url=https://github.com/rheit/zdoom/pull/695]Here[/url], I fixed the strife hands.
The PR fixes 2 crashes related to strife and one small bug I found.

Hopefully there are no issues left with strife's firehands.

Here are test wads for all the issues I addressed:
[attachment=0]fixes.zip[/attachment]

Let's just say I "[i]bought[/i]" strife and now I can test it.

[quote="Ed the Bat"]I just really wish there was a better way to do what A_ItBurnsItBurns does. A (restricted, no less) function from within the playerclass affecting the weapons? Seems iffy at best. And I've heard Graf himself say it's a disgusting hack, the way it is right now.[/quote]
I guess you simply didn't see it in the thread but this was actually addressed as well.
I generalized it so that you can create overlays from within the player's own body and made strife's firehands use that internally.

Must've posted at the same time as you had merged it. Oh well. :P

'Was' being the operative term here.

It was a bug wasn't it?