Generalized the psprite system and made it useable by mods.

[Leonard2]

Here's the new pull request.

What this does:

• Fixes some stuff
• Adds A_OverlayFlags with 2 available flags:
  • PSPF_ADDBOB: The layer will follow the weapon's bobbing.
  • PSPF_ADDWEAPON: The layer will follow the weapon's offsets (by adding them to its own offsets).
  A layer that comes from a weapon will have these set by default (including the flash layer of course).
• Changes the default layer's indices and give easy access to them in decorate (for example you could use "PSP_FLASH" instead of 1000 in A_OverlayOffset).

I also updated my example wad with a weapon ("NewWeapon") that makes use of this function:

insane_psprites.pk3

(200.2 KiB) Downloaded 45 times

It makes the weapon move in a circle and firing it will toggle the flash state's addbob flag and using altfire toggles its addweapon flag.



Once you feel this whole submission can be accepted, would it be possible for me to make a final pull request with all the commits rebased, reworded and squashed (not all of them of course, I'm talking about commits like the refactor one) so as to have a clean history?

[Major Cooke]

Why the ps_ to PSP_ change? I don't think this was necessary.

Other than that, it looks quite spiffy! I can't wait for this to undergo review!

[Leonard2]

A lot of the checks had to be redone because of the indice changes so I figured I might as well rename them and add them to decorate.

[ZzZombo]

You should let to disable any of the flags on layers coming from weapons though.

[Major Cooke]

Tomorrow, there will be a video.

---

Ignore the fact that the doomguy has four hands.

[Graf Zahl]

There's still too many special checks for PSP_WEAPON and PSP_FLASH and I am still not convinced that this is either good or even necessary. I re-added the P_SetPsprite function for a reason - this would be a good central place to do any special treatment for the old layers. Currently it is littered throughout the code and may interfere with what some people may try to achieve because the defaults are forced onto these layers no matter what.

[Major Cooke]

So you want:

Code: Select all

	if (ID == PSP_WEAPON || ID == PSP_FLASH || (ID < PSP_TARGETCENTER && Caller->IsKindOf(RUNTIME_CLASS(AWeapon))))
	{
		Flags |= PSPF_ADDBOB;
		if (ID != PSP_WEAPON)
			Flags |= PSPF_ADDWEAPON;
	}

All those, inside of P_SetPSprite? Because most of those are all centered around SetState. Albeit that example there was from DSprite::DSprite.

[Graf Zahl]

I have a problem with these having different semantics if being used from the new generic functions. This is a nightmare to document and an open invitation for needless bug reports.
This special behavior should be set from where these layers are set up, not as an inherent property of them.

[Major Cooke]

So how does this look?
Had to move this part back after realizing it ignored offset(x,y)

[Graf Zahl]

Aside from the problem you mention, yes, that's how I'd like to see it.

[Major Cooke]

I fixed the offset(x,y) issue. https://github.com/MajorCooke/zdoom/com ... 23fc7e43f5

According to Leonard, however, it still breaks things. I'll let him take over from here.

[Leonard2]

I re-added the P_SetPsprite function for a reason - this would be a good central place to do any special treatment for the old layers

GetPSprite was already here for this. It was made because the code that uses the old layers worked under the assumption that the layers were always present no matter what, they were never destroyed.
This is why it only takes the layer enum as a parameter, it is used ONLY with old code. GetPSprite already deals with checking if the layer exists and re-creates it if not, it could also deal with anything else the old layers need.

There's still too many special checks for PSP_WEAPON and PSP_FLASH and I am still not convinced that this is either good or even necessary.

I'll try and explain as best as I can some of these checks.

First, as I said earlier, the old layers could tick even when ReadyWeapon was null.
This is problematic because ReadyWeapon is always what's assumed to be the caller of those layers.
Because of this a few checks have to take this in account otherwise the game could crash.

---

Code: Select all

	if (ID == PSP_WEAPON || ID == PSP_FLASH || (ID < PSP_TARGETCENTER && Caller->IsKindOf(RUNTIME_CLASS(AWeapon))))
	{
		Flags |= PSPF_ADDBOB;
		if (ID != PSP_WEAPON)
			Flags |= PSPF_ADDWEAPON;
	}

This checks if the layer comes from a weapon and if so applies default flags. You might think it's ok for the old layers because in this case it's always ReadyWeapon, but what if it is null? The game crashes.
Extra fun with the targeters as their caller indeed is a weapon but they do not bob/follow it.

---

Code: Select all

	// Special handling for the old hardcoded layers.
	if (ID == PSP_WEAPON || ID == PSP_FLASH || ID >= PSP_TARGETCENTER)
		Caller = Owner->ReadyWeapon;

This is here to take in account the fact that ReadyWeapon can be null when the layer is created.
Notice that the old code did this too anyways:

Code: Select all

			if (state->CallAction(player->mo, player->ReadyWeapon, &newstate))

---

Code: Select all

		if ((ID == PSP_WEAPON || ID == PSP_FLASH || ID >= PSP_TARGETCENTER) || Caller->IsKindOf(RUNTIME_CLASS(AWeapon)))
		{ // The targeter layers are affected by this too.
			if (sv_fastweapons == 2 && ID == PSP_WEAPON)
				Tics = newstate->ActionFunc == nullptr ? 0 : 1;
			else if (sv_fastweapons == 3)
				Tics = (newstate->GetTics() != 0);
			else if (sv_fastweapons)
				Tics = 1;		// great for producing decals :)
		}

Again taking care of ReadyWeapon being null.

---

Code: Select all

		if (ID != PSP_FLASH)
		{ // It's still possible to set the flash layer's offsets with the action function.
			if (newstate->GetMisc1())
			{ // Set coordinates.
				x = newstate->GetMisc1();
			}
			if (newstate->GetMisc2())
			{
				y = newstate->GetMisc2();
			}
		}

Now that the offset behavior was generalized with a flag, if a mod ever used the flash layer with a state that was shared with the weapon layer for example and used offset(x, y) then said mod would break without this check as the flash layer would no longer be where it should be.
This is because before, the flash's offsets were ignored whereas now they would be added on top of the weapon offsets.
Modders can still set the offsets with A_OverlayOffset as the comment points out.

---

Code: Select all

			if (((ID == PSP_WEAPON || ID == PSP_FLASH || ID >= PSP_TARGETCENTER) || Caller->IsKindOf(RUNTIME_CLASS(AWeapon))) &&
				(Tics && Owner->cheats & CF_DOUBLEFIRINGSPEED))
				Tics--;

Taking care of ReadyWeapon being null.

---

Code: Select all

	if (ID == PSP_WEAPON)
	{ // A_WeaponReady will re-set these as needed
		Owner->WeaponState &= ~(WF_WEAPONREADY | WF_WEAPONREADYALT | WF_WEAPONBOBBING | WF_WEAPONSWITCHOK | WF_WEAPONRELOADOK | WF_WEAPONZOOMOK |
								WF_USER1OK | WF_USER2OK | WF_USER3OK | WF_USER4OK);
	}

I can't do anything about this one. The weapon layer sets the WeaponState everytime it changes states, I can't move this anywhere else.

I have a problem with these having different semantics if being used from the new generic functions.

I do get your point.
I could clean out most of these checks and then take care of the old layers behavior in GetPSprite but before I would need to make sure ReadyWeapon is not null when they are called.
I need your input for this though.

Take a look at this action function:

Code: Select all

DEFINE_ACTION_FUNCTION(AActor, A_ItBurnsItBurns)
{
	PARAM_ACTION_PROLOGUE;

	S_Sound (self, CHAN_VOICE, "human/imonfire", 1, ATTN_NORM);

	if (self->player != nullptr && self->player->mo == self)
	{
		P_SetPsprite(self->player, PSP_WEAPON, self->FindState("FireHands"));
		P_SetPsprite(self->player, PSP_FLASH, nullptr);
		self->player->ReadyWeapon = nullptr;
		self->player->PendingWeapon = WP_NOCHANGE;
		self->player->playerstate = PST_LIVE;
		self->player->extralight = 3;
	}
	return 0;
}

According to the wiki article about A_ItBurnsItBurns, it is only ever used for the StrifePlayer class.
After checking, it is only called within the "Burn" state which corresponds to Death.Fire according to this.
This corresponds to the problematic check that caused this whole ReadyWeapon being null issue:

Code: Select all

	bool noweapon = (ReadyWeapon == nullptr && (health > 0 || mo->DamageType != NAME_Fire));

This is what I need your input for.
Assuming this very action function is indeed the reason for this horrible check and that no mods rely on this horrendous kludge, we could change it.
I could let ReadyWeapon be null and have the weapon layer be destroyed as a result, create a new layer with its state set to firehands but then what do I set the caller to?

By the way, you still didn't respond to the question I asked previously:

Once you feel this whole submission can be accepted, would it be possible for me to make a final pull request with all the commits rebased, reworded and squashed (not all of them of course, I'm talking about commits like the refactor one) so as to have a clean history?

[Graf Zahl]

According to the wiki article about A_ItBurnsItBurns, it is only ever used for the StrifePlayer class.
After checking, it is only called within the "Burn" state which corresponds to Death.Fire according to this.
This corresponds to the problematic check that caused this whole ReadyWeapon being null issue:

Code: Select all

	bool noweapon = (ReadyWeapon == nullptr && (health > 0 || mo->DamageType != NAME_Fire));

This is what I need your input for.
Assuming this very action function is indeed the reason for this horrible check and that no mods rely on this horrendous kludge, we could change it.
I could let ReadyWeapon be null and have the weapon layer be destroyed as a result, create a new layer with its state set to firehands but then what do I set the caller to?

Right now that is the only use case for the main weapon layer without a valid weapon.

By the way, you still didn't respond to the question I asked previously:

Once you feel this whole submission can be accepted, would it be possible for me to make a final pull request with all the commits rebased, reworded and squashed (not all of them of course, I'm talking about commits like the refactor one) so as to have a clean history?

No, do not squash them. That would serve no good purpose.

[Major Cooke]

What about the caller though for the strife hands burning? What should it be set to?

[Leonard2]

New pull request yet again.

This should hopefully address all the remaining issues.

I removed the specific checks/treatments and moved everything that's needed in GetPSprite.

Since I couldn't figure out what to set the strife's firehands caller to I just made it possible to have a layer come from the player's own body (which makes sense since the firehands definition are within the player's body) and used that.
I don't have strife however so the strife's firehands are completely untested because of this, let me know if it doesn't work properly or at all.