-
-
Notifications
You must be signed in to change notification settings - Fork 311
fix: don't process out of bound section while trimming Y sections #2902
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
| int minLayer = (minY - 1) >> 4; | ||
| int maxLayer = (maxY + 1) >> 4; | ||
| int minLayer = minY >> 4; | ||
| int maxLayer = maxY >> 4; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would theoretically work with maxY + 1 when reverting the change in L97 (the ternary) - but that does not really feel wanted.
Currently, when crossing the chunk section border on the upper end (e.g. Y 255), the maxLayer is the section from 256-271. That still is fine and works - the layer of the section representing maxY is set to __RESERVED__ as it should - but the chunk section from 256-271 is set to 4096 chars only containing __RESERVED__ instead of just being set to null.
With the change, that now is more consistent. In the previous example, the section is filled with the correct blocks from Y 240-254 and 255 is set to __RESERVED__. The section 256-271 is set to null.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would make sense to clarify the meaning of minLayer and maxLayer. From my understanding, minLayer is the highest layer that should be partially trimmed below, and maxLayer is the lowest layer that should be partially trimmed above.
The edge cases here are, when not adding/subtracting before the shift, when minY % 16 == 0 (the full section should not be trimmed) or maxY % 16 == 15 (the full section should not be trimmed,).
In the minLayer case, that's easy because 0 << 8 == 0, so the loop won't run.
In the maxLayer case, 15 << 8 == 3840, means we would clear one layer too early. But (15 + 1) % 16 == 0, so adding the layer this way results in the whole section being wiped. But I think moving the addition should work: 15 % 16 == 15,(15 + 1) << 8 == 4096, so ((maxX & 15) + 1) << 8 should simplify the maxLayer case.
That's at least my understanding, maybe I missed something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've simplified the logic itself a bit which should make the whole thing easier to understand I'd say? (At least it does for me). And yeah, the simplification should work (and does based on my testing).
If the new loop logic is alright, I could (and should) apply that to the other part of the trimY method (where keepInsideRange is false)
worldedit-core/src/main/java/com/fastasyncworldedit/core/queue/IBatchProcessor.java
Outdated
Show resolved
Hide resolved
SirYwell
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good now.
Overview
Fixes #2894
Description
trimY now doesn't overflow into unrelated sections anymore, allowing out of bounds editing. Seems to work for me so far - tests are passing. I couldn't find any edge cases or how that might break other existing behavior.
I'd highly appreciate additional tests by others.