Fix of the BlockLineIterator causing IllegalStateException#7513
Conversation
sovdeeth
left a comment
sovdeeth
left a comment
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
sovdeeth
left a comment
sovdeeth
left a comment
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 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 |
| // 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 |
|
what are the chances we can get tests/regression tests for this bad boy? |
|
Looks good |
…g#7513) * replaced Bukkit's (broken) block iterator with own implementation * checkstyle and fixed the overflowing max target block distance * fixed block duplicates, mimics the behaviour of previous block line iterator (using grid traversal algorithm instead of Bresenham's) * added documentation, changed some variable names so the code is easier to understand * fixed epsilon calculation, small changes * simplify calculations by removing Planes, add more tests * sidestep weird JOML mistake (now fixed, but not in pre 1.21.4) --------- Co-authored-by: sovdee <10354869+sovdeeth@users.noreply.github.com>
5 participants
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