:fire: Remove done boolean from LineClear

[AJenbo]

Alternately I could change the wile condition to while (position != endPoint && (position == startPoint || Clear(entity, position))) {

P.s. the two paths in the big if-else are identical except for the operational axis. I wonder if we could do something clever to merge that. Some thing like this maybe?

bool LineClear(bool (*Clear)(int, Point), int entity, Point startPoint, Point endPoint)
{
	int d;
	int incD, dincD, dincH;

	Point position = startPoint;

	int d1 = endPoint.x - position.x;
	int d2 = endPoint.y - position.y;
	bool horizontal = abs(d1) > abs(d2);
	if (horizontal)
		std::swap(d1, d2);
	if (d1 < 0) {
		std::swap(position, endPoint);
		d1 = -d1;
		d2 = -d2;
	}
	if (d2 > 0) {
		d = 2 * d2 - d1;
		dincD = 2 * d2;
		dincH = 2 * (d2 - d1);
		incD = 1;
	} else {
		d = 2 * d2 + d1;
		dincD = 2 * d2;
		dincH = 2 * (d1 + d2);
		incD = -1;
	}
	while (position != endPoint && (position == startPoint || Clear(entity, position))) {
		if ((d <= 0) ^ (incD < 0)) {
			d += dincD;
		} else {
			d += dincH;
			if (horizontal)
				position.y += incD;
			else
				position.x += incD;
		}
		if (horizontal)
			position.x++;
		else
			position.y++;
	}

	return position == endPoint;
}

[qndel]

what about

bool LineClear(bool (*Clear)(int, Point), int entity, Point startPoint, Point endPoint)
{
	int d;
	int incD, dincD, dincH;

	Point position = startPoint;

	int d1 = endPoint.x - position.x;
	int d2 = endPoint.y - position.y;
	bool horizontal = abs(d1) > abs(d2);
	if (horizontal)
		std::swap(d1, d2);
	if (d1 < 0) {
		std::swap(position, endPoint);
		d1 = -d1;
		d2 = -d2;
	}
	if (d2 > 0)
		incD = 1;
	else
		incD = -1;
	d = 2 * d2 - (d1 * incD);
	dincD = 2 * d2;
	dincH = 2 * (d2 - (d1 * incD));
	while (position != endPoint && (position == startPoint || Clear(entity, position))) {
		int &coord = (horizontal ? position.x : position.y);
		if ((d <= 0) ^ (incD < 0)) {
			d += dincD;
		} else {
			d += dincH;
			coord += incD;
		}
		coord++;
	}

	return position == endPoint;
}

[AJenbo]

@qndel nice, then we can even move all the declerations to where the assignments:

That takes it from 78 to 30, two steps down in the code smell scale :tada: Not as good as LineClearF1 which was completely eradicated, but still :D

Edit: Noticed that coord is used in both cases, but it should be flipped in one :)

[julealgon]

Small comment before I review (after dinner): I noticed you are using :fire: here instead of 🔥. Did you have issues again with the unicode characters?

[AJenbo]

Small comment before I review (after dinner): I noticed you are using :fire: here instead of 🔥. Did you have issues again with the unicode characters?

Typing :fire: is faster the Googling the emoji. Don't know if there is a fancy way to enter it, but not important to me. The few I remember you will often find that I still type the tag for instead of copying the emoji. But it also depends if I'm using macOS, Linux or Android at the time of writing.

[AJenbo]

Just play-tested this, and we defiantly broke something, got men will just run around you and not attack :(

Edit: Fixed, first case was also flipped.

[AJenbo]

Diving deeper in to this function makes me think it's absolutely terrible. When moving in a negative direction it doesn't check the start or end position, but when moving in a positive direction it checks the end position.

It also doesn't align with the path that arrows actually travel; leading to scenarios where the monster thinks it can hit you but the arrow just hits a wall instead.

A possible solution would be to adapt it to match the logic in GetMissilePos() (missile movment), but this function is based on velocity and may check the same tile multiple times, or worse yet skip tiles if the velocity is high enough. The setup for this is a bit expensive as GetMissileVel() uses both sqrt and double. @NiteKat has also proposed a solution to the issue here: https://github.com/diasurgical/devilutionX/pull/1013