Fix of the BlockLineIterator causing IllegalStateException #7513
Conversation
There was a problem hiding this comment.
as mentioned in discord, the behavior of jumping by 1 meter at a time will lead to skipped blocks and blocks being repeated if the direction isn't along an axis.
BlockIterator specifically accounts for this, which is why it was used. If we want to move away from it (not sure we need to), we need to duplicate that behavior.
…terator (using grid traversal algorithm instead of Bresenham's)
|
There's still a small difference. The Bukkit Block Iterator occasionally fails to connect blocks unless they share faces, which leads to extra blocks being included in the line, but that in my opinion would be considered unwanted in most cases. |
sovdeeth
added
the
bug
There was a problem hiding this comment.
I'm not sure we need to implement all the plane stuff to have an effective iterator
We can basically simplify the calcParam function by getting the x/y/z value of the edge of the block (which you do with the PlanePoints) and then doing
(xPlanePoint - from.getX()) / step.getX() to get the number of steps to that edge.
It's the exact same math but written in a simpler way.
That said, if you think that having the plane implementation is more useful, let me know.
| * @param a a | ||
| * @param b b | ||
| * @param c c |
There was a problem hiding this comment.
| * @param a a | |
| * @param b b | |
| * @param c c | |
| * @param a a (normal vector x) | |
| * @param b b (normal vector y) | |
| * @param c c (normal vector z) |
| * @param start first block | ||
| * @param direction direction | ||
| * @param distance distance |
There was a problem hiding this comment.
| * @param start first block | |
| * @param direction direction | |
| * @param distance distance | |
| * @param start first block | |
| * @param direction direction to travel in | |
| * @param distance maximum distance to travel |
| * @param start start location | ||
| * @param direction direction | ||
| * @param distance distance |
There was a problem hiding this comment.
| * @param start start location | |
| * @param direction direction | |
| * @param distance distance | |
| * @param start start location | |
| * @param direction direction to travel in | |
| * @param distance maximum distance to travel |
| Vector first = p2.clone().subtract(p1); | ||
| Vector second = p3.clone().subtract(p1); |
There was a problem hiding this comment.
| Vector first = p2.clone().subtract(p1); | |
| Vector second = p3.clone().subtract(p1); | |
| Vector first = p2.clone().subtract(p1); | |
| Vector second = p3.clone().subtract(p1); |
first and second aren't much better than v1/v2
maybe something like axis1 and 2 or planeVector, or orthogonal vector, or anything hinting at what it's being used for
if nothing pops to mind, i'd still prefer vector1/2 instead of just first/second
| * @param direction | ||
| * @param distance | ||
| * @throws IllegalStateException randomly (Bukkit bug) | ||
| * Calculates the parameter (distance) to the next block in the direction of {@link #step} from |
There was a problem hiding this comment.
If we're clarifying param is distance, why not just call it distance? Since the step vector is normalized, it truly is distance
| return b != null && b.getLocation().add(0.5, 0.5, 0.5).distanceSquared(start) >= distSq; | ||
| } | ||
| }, false); | ||
| private double calculateParamToNext(Vector from) { |
There was a problem hiding this comment.
Do you think it's worth having static unit vectors instead of creating them each time?
| * @param direction | ||
| * @param distance | ||
| * @throws IllegalStateException randomly (Bukkit bug) | ||
| * Calculates the distance between a point and plane. |
There was a problem hiding this comment.
| * Calculates the distance between a point and plane. | |
| * Calculates the distance between a point and plane along the step vector. |
| // in this use-case the value is either between 0 and sqrt(2) or infinity, we choose the | ||
| // minimum out of 3 distances, in case of infinity we return 2, so it never gets chosen | ||
| // (there is always one distance guaranteed to be between 0 and sqrt(2), see #calculateExact(Vector)) | ||
| if (!Double.isFinite(ratio)) return 2; |
There was a problem hiding this comment.
Is this really necessary? we only use it in comparison anyway so infinity is just as valid as 2. If you'd like to keep it returning 2 then let me know, i think the comment could use some work in that case.
| double t = plane.a() * step.getX() + plane.b() * step.getY() + plane.c() * step.getZ(); | ||
| double ratio = value / t; |
There was a problem hiding this comment.
ratio is actually t
(Ax + By + Cz) + t*(A*stepX + B*stepY + C*stepZ) = -D
hence
t = (-D - (Ax + By + Cz)) / (A*stepX + B*stepY + C*stepZ)
|
PlanePoints are not aligned with the direction of the |
Description
This PR fixes an issue with the BlockLineIterator, previously backed by Bukkit BlockIterator, missing the last block in a line resulting in IllegalStateException. The new implementation consistently iterates through all blocks between two points/locations without relying on Bukkit BlockIterator.
Target Minecraft Versions: any
Requirements: none
Related Issues: #6437 and #7496