Fix of the BlockLineIterator causing IllegalStateException

src/main/java/ch/njol/skript/expressions/ExprBlocks.java

@@ -28,13 +28,14 @@
 import ch.njol.util.coll.iterator.ArrayIterator;
 
 @Name("Blocks")
-@Description({"Blocks relative to other blocks or between other blocks. Can be used to get blocks relative to other blocks or for looping.",
-		"Blocks from/to and between will return a straight line whereas blocks within will return a cuboid."})
+@Description({"Blocks relative to other blocks or between other blocks.",
+	"Can be used to get blocks relative to other blocks or for looping.",
+	"Blocks from/to and between will return a straight line whereas blocks within will return a cuboid."})
 @Examples({"loop blocks above the player:",
-		"loop blocks between the block below the player and the targeted block:",
-		"set the blocks below the player, the victim and the targeted block to air",
-		"set all blocks within {loc1} and {loc2} to stone",
-		"set all blocks within chunk at player to air"})
+	"loop blocks between the block below the player and the targeted block:",
+	"set the blocks below the player, the victim and the targeted block to air",
+	"set all blocks within {loc1} and {loc2} to stone",
+	"set all blocks within chunk at player to air"})
 @Since("1.0, 2.5.1 (within/cuboid/chunk)")
 public class ExprBlocks extends SimpleExpression<Block> {
 
@@ -100,7 +101,7 @@ protected Block[] get(Event event) {
 			return from.stream(event)
 					.filter(Location.class::isInstance)
 					.map(Location.class::cast)
-				    .filter(location -> {
+					.filter(location -> {
 						if (SUPPORTS_WORLD_LOADED)
 							return location.isWorldLoaded();
 						return location.getChunk().isLoaded();
@@ -129,15 +130,20 @@ public Iterator<Block> iterator(Event event) {
 				Object object = from.getSingle(event);
 				if (object == null)
 					return null;
-				Location location = object instanceof Location ? (Location) object : ((Block) object).getLocation().add(0.5, 0.5, 0.5);
+				Location location = object instanceof Location
+					? (Location) object
+					: ((Block) object).getLocation().add(0.5, 0.5, 0.5);
 				Direction direction = this.direction.getSingle(event);
 				if (direction == null || location.getWorld() == null)
 					return null;
-				Vector vector = object != location ? direction.getDirection((Block) object) : direction.getDirection(location);
+				Vector vector = object != location
+					? direction.getDirection((Block) object)
+					: direction.getDirection(location);
 				// Cannot be zero.
 				if (vector.getX() == 0 && vector.getY() == 0 && vector.getZ() == 0)
 					return null;
-				int distance = SkriptConfig.maxTargetBlockDistance.value();
+				// start block + (max - 1) == max
+				int distance = SkriptConfig.maxTargetBlockDistance.value() - 1;
 				if (this.direction instanceof ExprDirection) {
 					Expression<Number> numberExpression = ((ExprDirection) this.direction).amount;
 					if (numberExpression != null) {
@@ -188,7 +194,7 @@ public String toString(@Nullable Event event, boolean debug) {
 			return "blocks from " + from.toString(event, debug) + " to " + end.toString(event, debug);
 		} else {
 			assert direction != null;
-			return "block" + (isSingle() ? "" : "s") + " " + direction.toString(event, debug) + " " + from.toString(event, debug);
+			return "blocks " + direction.toString(event, debug) + " " + from.toString(event, debug);
 		}
 	}
 

src/main/java/ch/njol/skript/util/BlockLineIterator.java

@@ -1,84 +1,132 @@
 package ch.njol.skript.util;
 
-import ch.njol.skript.bukkitutil.WorldUtils;
-import ch.njol.util.Math2;
 import org.bukkit.Location;
+import org.bukkit.World;
 import org.bukkit.block.Block;
-import org.bukkit.util.BlockIterator;
 import org.bukkit.util.Vector;
-import org.jetbrains.annotations.Nullable;
+import org.jetbrains.annotations.Contract;
+import org.jetbrains.annotations.NotNull;
+import org.joml.Vector3d;
 
-import ch.njol.util.NullableChecker;
-import ch.njol.util.coll.iterator.StoppableIterator;
+import java.util.Iterator;
+import java.util.NoSuchElementException;
 
-public class BlockLineIterator extends StoppableIterator<Block> {
+/**
+ * Iterates through blocks in a straight line from a start to end location (inclusive).
+ * <p>
+ * Given start and end locations are always cloned but may not be block-centered.
+ * Iterates through all blocks the line passes through in order from start to end location.
+ */
+public class BlockLineIterator implements Iterator<Block> {
+
+	private final Vector3d current;
+	private final Vector3d end;
+	private final Vector3d centeredEnd;
+	final Vector3d step; // package private for tests
+	private final World world;
+	private boolean finished;
+
+	/**
+	 * @param start start location
+	 * @param end end location
+	 */
+	public BlockLineIterator(@NotNull Location start, @NotNull Location end) {
+		this.current = start.toVector().toVector3d();
+		this.world = start.getWorld();
+		this.end = end.toVector().toVector3d();
+		this.centeredEnd = centered(this.end);
+		this.step = this.end.sub(current, new Vector3d()).normalize();
+	}
+
+	/**
+	 * @param start first block
+	 * @param end last block
+	 */
+	public BlockLineIterator(@NotNull Block start, @NotNull Block end) {
+		this(start.getLocation().toCenterLocation(), end.getLocation().toCenterLocation());
+	}
 
 	/**
-	 * @param start
-	 * @param end
-	 * @throws IllegalStateException randomly (Bukkit bug)
+	 * @param start start location
+	 * @param direction direction to travel in
+	 * @param distance maximum distance to travel
 	 */
-	public BlockLineIterator(Block start, Block end) throws IllegalStateException {
-		super(new BlockIterator(start.getWorld(), start.getLocation().toVector(),
-				end.equals(start) ? new Vector(1, 0, 0) : end.getLocation().subtract(start.getLocation()).toVector(), // should prevent an error if start = end
-				0, 0
-			),
-		new NullableChecker<Block>() {
-			private final double overshotSq = Math.pow(start.getLocation().distance(end.getLocation()) + 2, 2);
-
-			@Override
-			public boolean check(@Nullable Block block) {
-				assert block != null;
-				if (block.getLocation().distanceSquared(start.getLocation()) > overshotSq)
-					throw new IllegalStateException("BlockLineIterator missed the end block!");
-				return block.equals(end);
-			}
-		}, true);
+	public BlockLineIterator(Location start, @NotNull Vector direction, double distance) {
+		this(start, start.clone().add(direction.clone().normalize().multiply(distance)));
+	}
+
+	/**
+	 * @param start first block
+	 * @param direction direction to travel in
+	 * @param distance maximum distance to travel
+	 */
+	public BlockLineIterator(@NotNull Block start, Vector direction, double distance) {
+		this(start.getLocation().toCenterLocation(), direction, distance);
+	}
+
+	@Override
+	public boolean hasNext() {
+		return !finished;
+	}
+
+	@Override
+	public Block next() {
+		if (!hasNext()) throw new NoSuchElementException("Reached the final block destination");
+		// sanity check (is the current->end vector pointing away from step)
+		if (end.sub(current, new Vector3d()).dot(step) < 0) throw new NoSuchElementException("Overshot the final block!");
+		// get block and check end
+		Vector3d center = centered(current);
+		Block block = getBlock(center, world);
+		if (center.equals(centeredEnd)) finished = true;
+		// calculate next position
+		double t = stepsToNextFace(current, step, center) + Math.ulp(1);
+		current.fma(t, step);
+		return block;
 	}
 
 	/**
-	 * @param start
-	 * @param direction
-	 * @param distance
-	 * @throws IllegalStateException randomly (Bukkit bug)
+	 * Calculates the number of steps to the next closest block face this ray, defined by start and step, will encounter.
+	 * Block faces are determined by the center vector, which is interpreted as the center of the block.
+	 * @param start the current location of the ray to check.
+	 * @param step the direction of the ray.
+	 * @param center the center location of the block the ray is currently within.
+	 * @return a scalar floating point number representing the number of times step must be added to start in order
+	 * 			to arrive at the closest block face.
 	 */
-	public BlockLineIterator(Location start, Vector direction, double distance) throws IllegalStateException {
-		super(new BlockIterator(start.getWorld(), start.toVector(), direction, 0, 0), new NullableChecker<Block>() {
-			private final double distSq = distance * distance;
-
-			@Override
-			public boolean check(final @Nullable Block b) {
-				return b != null && b.getLocation().add(0.5, 0.5, 0.5).distanceSquared(start) >= distSq;
-			}
-		}, false);
+	static double stepsToNextFace(Vector3d start, @NotNull Vector3d step, Vector3d center) {
+		Vector3d neededSteps = new Vector3d(Math.signum(step.x), Math.signum(step.y), Math.signum(step.z))
+				.mulAdd(0.5, center)
+				.sub(start)
+				.div(step, new Vector3d()); // need to make new vector due to JOML method signature issue
+		// get min component, ignoring NaN
+		if (Double.isNaN(neededSteps.x))
+			neededSteps.x = Double.POSITIVE_INFINITY;
+		if (Double.isNaN(neededSteps.y))
+			neededSteps.y = Double.POSITIVE_INFINITY;
+		if (Double.isNaN(neededSteps.z))
+			neededSteps.z = Double.POSITIVE_INFINITY;
+		return neededSteps.get(neededSteps.minComponent());
 	}
 
 	/**
-	 * @param start
-	 * @param direction
-	 * @param distance
-	 * @throws IllegalStateException randomly (Bukkit bug)
+	 * Creates vector at the center of a block at the coordinates provided
+	 * by {@code vector}.
+	 *
+	 * @param vector point
+	 * @return coordinates at the center of a block at given point
 	 */
-	public BlockLineIterator(Block start, Vector direction, double distance) throws IllegalStateException {
-		this(start.getLocation().add(0.5, 0.5, 0.5), direction, distance);
+	@Contract("_ -> new")
+	private static Vector3d centered(@NotNull Vector3d vector) {
+		return vector.floor(new Vector3d()).add(0.5, 0.5, 0.5);
 	}
 
 	/**
-	 * Makes the vector fit within the world parameters.
-	 * 
-	 * @param location The original starting location.
-	 * @param direction The direction of the vector that will be based on the location.
-	 * @return The newly modified Vector if needed.
+	 * @param vector the xyz coordinates of the block to get.
+	 * @param world the world which the block should be obtained from
+	 * @return the block at the given xyz coords in the given world.
 	 */
-	private static Vector fitInWorld(Location location, Vector direction) {
-		int lowest = WorldUtils.getWorldMinHeight(location.getWorld());
-		int highest = location.getWorld().getMaxHeight();
-		Vector vector = location.toVector();
-		int y = location.getBlockY();
-		if (y >= lowest && y <= highest)
-			return vector;
-		double newY = Math2.fit(lowest, location.getY(), highest);
-		return new Vector(location.getX(), newY, location.getZ());
+	private static @NotNull Block getBlock(@NotNull Vector3d vector, @NotNull World world) {
+		return Vector.fromJOML(vector).toLocation(world).getBlock();
 	}
 
 }

src/main/java/ch/njol/util/coll/iterator/StoppableIterator.java

@@ -8,8 +8,9 @@
 import ch.njol.util.NullableChecker;
 
 /**
- * @author Peter Güttinger
+ * @deprecated unused
  */
+@Deprecated
 public class StoppableIterator<T> implements Iterator<T> {
 
 	private final Iterator<T> iter;

src/test/java/ch/njol/skript/util/BlockLineIteratorTest.java

@@ -0,0 +1,44 @@
+package ch.njol.skript.util;
+
+import org.bukkit.Location;
+import org.joml.Vector3d;
+import org.junit.Assert;
+import org.junit.Test;
+
+import java.util.NoSuchElementException;
+
+import static org.junit.Assert.assertEquals;
+
+public class BlockLineIteratorTest {
+
+	@Test
+	public void testStepsToNextFace() {
+		Vector3d start = new Vector3d(0,0,0);
+		Vector3d center = new Vector3d(0.5,0.5,0.5);
+		Vector3d step = new Vector3d(0,0,0);
+		assertEquals(Double.POSITIVE_INFINITY, BlockLineIterator.stepsToNextFace(start, step, center), Math.ulp(1d));
+		step.x = 1;
+		assertEquals(1.0, BlockLineIterator.stepsToNextFace(start, step, center), Math.ulp(1d));
+		step.y = 1;
+		step.normalize();
+		assertEquals(Math.sqrt(2), BlockLineIterator.stepsToNextFace(start, step, center), Math.ulp(1d));
+		step.x = 1;
+		step.y = 1;
+		step.z = 1;
+		step.normalize();
+		assertEquals(Math.sqrt(3), BlockLineIterator.stepsToNextFace(start, step, center), Math.ulp(1d));
+		start.x = 0.99;
+		assertEquals(Math.sqrt(3) / 100, BlockLineIterator.stepsToNextFace(start, step, center), Math.ulp(1d));
+	}
+
+	@Test
+	public void testOvershoot() {
+		Location start = new Location(null, 0,0,0);
+		Location end = new Location(null, 0,10,0);
+		var iterator = new BlockLineIterator(start, end);
+		iterator.step.mul(-1);
+		// step is now wrong way, so it should trigger overshoot protection
+		Assert.assertThrows(NoSuchElementException.class, iterator::next);
+	}
+
+}

src/test/skript/tests/regressions/5566-block iterator being 100 always.sk

@@ -4,5 +4,6 @@ test "100 blocks fix":
 	set {_l} to test-location ~ vector(0, -1, 10)
 	set block at {_l} to air
 
-	assert blocks 5 below {_l} contains air, grass block, dirt, dirt, bedrock and void air with "Failed to get correct blocks (got '%blocks 5 below test-location%')"
+	set {_blocks::*} to (block at {_l}), (block at {_l} ~ vector(0, -1, 0)), (block at {_l} ~ vector(0, -2, 0)), (block at {_l} ~ vector(0, -3, 0)), (block at {_l} ~ vector(0, -4, 0)), and (block at {_l} ~ vector(0, -5, 0))
+	assert blocks 5 below {_l} is {_blocks::*} with "Failed to get correct blocks"
 	assert size of blocks 3 below location below {_l} is 4 with "Failed to match asserted size"

src/test/skript/tests/regressions/7496-block iterator missing end block.sk

@@ -0,0 +1,19 @@
+test "basic block iterator test":
+	set {_l1} to location(0, 100, 0, world "world")
+	set {_l2} to location(10, 100, 10, world "world")
+	set {_origin} to location(5, 100, 5, world "world")
+	loop blocks within {_l1} and {_l2}:
+		set {_loc} to location of loop-value
+		assert blocks from {_origin} to {_loc} is set with "failed to find blocks between %{_origin}% and %{_loc}%."
+
+test "ensure non-centered block iterators are not equal":
+	set {_l1} to location(0.1, 0.1, 0.1, world "world")
+	set {_l2} to location(0.9, 0.9, 0.9, world "world")
+	set {_dir} to vector(1, 0, -1)
+	assert blocks {_dir} {_l1} are not blocks {_dir} {_l2} with "block iterators from offset locations returned same values."
+
+test "ensure same block iterators are equal":
+	set {_l1} to location(0.1, 0.1, 0.1, world "world")
+	set {_l2} to location(0.9, 0.9, 0.9, world "world")
+	set {_dir} to vector(1, 0, -1)
+	assert blocks {_dir} (block at {_l1}) are blocks {_dir} (block at {_l2}) with "block iterators from same block returned different values."