Gotchas of `AActor::AbsorbDamage()` in `DamageMobj()`

[ZzZombo]

`AActor::AbsorbDamage()`:

Code: Select all

for (AActor *item = Inventory; item != nullptr; item = next)

It already performs all the work of traversing the inventory chain and calling `AbsorbDamage()` on each item, gracefully handling `nullptr` `Inventory`.

Now, enter `DamageMobj()`:

Code: Select all

		// Armor for monsters.
		if (!(flags & (DMG_NO_ARMOR|DMG_FORCED)) && target->Inventory != NULL && damage > 0)

If the actor lacks inventory, the function will refuse to call `AActor::AbsorbDamage()`, potentially skipping overridden in subclasses versions of the virtual function. Given there is no harm in just calling the latter this check could be omitted altogether while also fixing this subtle bug.

Also, as seen from the comment originally this piece of code was meant for armor items. However, since then `AActor::AbsorbDamage()` got exposed to ZScript and as far as I know, there is no requirement to adhere to use it strictly for that. Using `DMG_NO_ARMOR` bypasses any custom logic in overridden versions of the function, just like empty inventory. I suggest to make `DMG_NO_ARMOR` ignore any reduced damage returned from the called function, but still call it.

`DMG_FORCED` also subverts any custom handling of incoming damage in the virtual function, so I'd suggest again to make it ignore any reduced damage returned by it if it's in effect.

All these notes apply also to the player-specific handling above: https://github.com/ZDoom/gzdoom/blob/2c ... .cpp#L1311.

[Professor Hastig]

Can you write such code without breaking anything?

[ZzZombo]

I could, but first, I'd like to hear from the other developers what do they think. This is a critical code, so I'd tread lightly here.

[Major Cooke]

You can't just ignore the result, because the whole point of overriding is to put the power in the hands of the modder who has the final say for what the damage output is.

For example, some of my super bosses I only want the player to be able to deal any form of damage at all. That means disabling all damage and ignoring all flags from other monsters. If the internal function ignored this result, then what's the point of overriding the functions in the first place? I don't want ANYONE overriding MY DamageMobj overrides, which is how it should be. My mods, my rules.

That being said, I do wish Modify/AbsorbDamage() functions were called no matter what and just respected the flag on ZScript's side, but unfortunately I think changing them now may do more harm than good.

[Graf Zahl]

There's nothing wrong here.

The internal 'AbsorbDamage' function is merely a helper that isn't even natively virtual, much less scripted. It was merely separated out for keeping the code clean. Think of it as a static local function.

The 'duplicate' checks for existing inventory are just because there's also other things that depend on it.