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.