P_RadiusAttack arbitrarily fails to damage

It's not immediately clear what causes it, but the function will sometimes fail to damage objects within its radius depending on their position around the actor. In the GIF below, the balloon remotely pops and calls P_RadiusAttack with a radius of 128 FU.

added Bug label

Oh yeah I ran into this problem before. It happens regardless of enabling or disabling the sight check.

While this is probably at least partially a P_AproxDistance issue the meat of the problem seems to be that the blockmap check just fails to detect enemies sometimes:

void P_RadiusAttack(mobj_t *spot, mobj_t *source, fixed_t damagedist, UINT8 damagetype, boolean sightcheck)
{
	INT32 x, y;
	INT32 xl, xh, yl, yh;
	fixed_t dist;

	dist = FixedMul(damagedist, spot->scale) + MAXRADIUS;
	yh = (unsigned)(spot->y + dist - bmaporgy)>>MAPBLOCKSHIFT;
	yl = (unsigned)(spot->y - dist - bmaporgy)>>MAPBLOCKSHIFT;
	xh = (unsigned)(spot->x + dist - bmaporgx)>>MAPBLOCKSHIFT;
	xl = (unsigned)(spot->x - dist - bmaporgx)>>MAPBLOCKSHIFT;

	BMBOUNDFIX(xl, xh, yl, yh);

	bombspot = spot;
	bombsource = source;
	bombdamage = FixedMul(damagedist, spot->scale);
	bombdamagetype = damagetype;
	bombsightcheck = sightcheck;

	for (y = yl; y <= yh; y++)
		for (x = xl; x <= xh; x++)
			P_BlockThingsIterator(x, y, PIT_RadiusAttack);
}

An immediate P_DamageMobj and return in PIT_RadiusAttack after this code still often fails to damage things.

While working on a branch exposing P_PathTraverse to Lua, I came across a bug similar to this where the traversal just would not detect any objects at certain positions. Eventually, I figured out the cause for both issues:

boolean P_BlockThingsIterator(INT32 x, INT32 y, boolean (*func)(mobj_t *))
{
	mobj_t *mobj;
	blocknode_t *block;

	if (x < 0 || y < 0 || x >= bmapwidth || y >= bmapheight)
		return true;

	// Check interaction with the objects in the blockmap.
	for (block = blocklinks[y*bmapwidth + x]; block; block = block->mnext)
	{
		mobj = block->mobj;

		if (!func(mobj))
			return false;
		if (P_MobjWasRemoved(tmthing)) // func just broke blockmap chain, cannot continue.
			return true;
	}

	return true;
}

If the line calling the function P_MobjWasRemoved and the line below it are removed or commented out, it fixes both problems. I'm not sure if removing these lines will result in anything breaking, and all I know is that they may have been added to make PIT_CheckThing run better (at least, according to Monster Iestyn).

If any other developer can shed some light on why these lines were added, that would be extremely helpful.

mentioned in merge request !2389

mentioned in merge request !2561 (merged)

closed with merge request !2561 (merged)

mentioned in commit f096e569