Possible Refactor

Godot Version

4.3 Mono

Question

I wrote this function that takes in the position of a room and finds the closest position that aligns it to a tilemap, however since the width and height logic is so similar I thought about reducing the scope of the function to only look at the x and y one at a time and just call it twice with different parameters but that doesnt seem very efficient either. Any suggestions to improve the efficiency of this code…

public static Vector2I GridSnap(Vector2I position, int width, int height, int cellSize)
    {
        int newX = int.MinValue;
        int newY = int.MinValue;

        if (width / cellSize % 2 != 0) {
            for (int upX = position.X; upX < position.X + cellSize; upX++) {
                if (upX % (cellSize / 2) == 0 && upX % cellSize != 0) {
                    newX = upX;
                    break;
                }
            }
            for (int downX = position.X; downX > position.X - cellSize; downX--) {
                if (downX % (cellSize / 2) == 0 && downX % cellSize != 0) {
                    if (newX - position.X > position.X - downX) {
                        newX = downX;
                        break;
                    }
                }
            }
        }
        else {
            for (int upX = position.X; upX < position.X + cellSize; upX++) {
                if (upX % cellSize == 0) {
                    newX = upX;
                    break;
                }
            }
            for (int downX = position.X; downX > position.X - cellSize; downX--) {
                if (downX % cellSize == 0) {
                    if (newX - position.X > position.X - downX) {
                        newX = downX;
                        break;
                    }
                }
            }
        }
        if (height / cellSize % 2 != 0) {
            for (int upY = position.Y; upY < position.Y + cellSize; upY++) {
                if (upY % (cellSize / 2) == 0 && upY % cellSize != 0) {
                    newY = upY;
                    break;
                }
            }
            for (int downY = position.Y; downY > position.Y - cellSize; downY--) {
                if (downY % (cellSize / 2) == 0 && downY % cellSize != 0) {
                    if (position.Y - newY > downY - position.Y) {
                        newY = downY;
                        break;
                    }
                }
            }
        }
        else {
            for (int upY = position.Y; upY < position.Y + cellSize; upY++) {
                if (upY % cellSize == 0) {
                    newY = upY;
                    break;
                }
            }
            for (int downY = position.Y; downY > position.Y - cellSize; downY--) {
                if (downY % cellSize == 0 ) {
                    if (newY - position.Y > position.Y - downY) {
                        newY = downY;
                        break;
                    }
                }
            }
        }
        return new Vector2I(newX,newY);
    } 
1 Like

I am pretty confident you can reduce your algorithm down to a truncated or rounded division and multiplication.

int newX = Mathf.RoundToInt(position.X / cellSize) * cellSize;
int newY = Mathf.RoundToInt(position.Y / cellSize) * cellSize;

If you want to keep your for-loop logic I would make use of boolean math to reduce the divided if statements.

public static Vector2I GridSnap(Vector2I position, int width, int height, int cellSize)
{
	int newX = int.MinValue;
	int newY = int.MinValue;

	bool width_odd = (width / cellSize) % 2 != 0;
	bool height_odd = (width / cellSize) % 2 != 0;

	// it will always find a downX value, so upX is overwritten completely. remove this loop
	//for (int upX = position.X; upX < position.X + cellSize; upX++) {
		//if (upX % (cellSize / 2) == 0 && upX % cellSize != 0) {
			//newX = upX;
			//break;
		//}
	//}
	for (int downX = position.X; downX > position.X - cellSize; downX--) {
		bool divisable = downX % cellSize == 0;
		bool half_divisable = downX % (cellSize/2) == 0;

		if (divisible != width_odd and (!width_odd or half_divisible))
			if (newX - position.X > position.X - downX) {
				newX = downX;
				break;
			}
		}
	}

	for (int downY = position.Y; downY > position.Y - cellSize; downY--) {
		bool divisable = downY % cellSize == 0;
		bool half_divisable = downY % (cellSize/2) == 0;

		if (divisable != height_odd and (!height_odd or half_divisable)) {
			if (position.Y - newY > downY - position.Y) {
				newY = downY;
				break;
			}
		}
	}
	return new Vector2I(newX,newY);
} 
1 Like

It will always find an up and down value i just need both to see which one is closer to the original position and without newX being set in the up loop the logic in the downloop doesnt work

1 Like

Ah then definitely try using the rounding functions I mentioned.

1 Like