fix: add a WrapperChunk to allow "Invalidating" a ChunkHolder on submission #3354
Conversation
dordsor21
commented
Oct 26, 2025
dordsor21
commented
- fixes issues where edits were not fully undone
- invalidate a ChunkHolder when submitting, via a "wrapper" to allow filters to be "aware"/forced to use a new ChunkHolder when it is submitted
- this also allows us to not submit a ChunkHolder in IQueueExtent#filter if the wrapper does not holder it anymore (i.e. it was forcibly submitted, and no more work was done)
…ission - fixes issues where edits were not fully undone - invalidate a ChunkHolder when submitting, via a "wrapper" to allow filters to be "aware"/forced to use a new ChunkHolder when it is submitted - this also allows us to *not* submit a ChunkHolder in IQueueExtent#filter if the wrapper does not holder it anymore (i.e. it was forcibly submitted, and no more work was done)
|
Looks like it works fine + nothing new comes up in profiler Would also be good if others could test and try to reproduce the issue with history (to see if it's "definitely fixed") |
SirYwell
left a comment
SirYwell
left a comment
There was a problem hiding this comment.
I'm not a fan of all the mutability in general, but I guess there currently isn't really a better way without rewriting larger parts of the whole plugin.
| private boolean createCopy = false; | ||
| private long initTime = -1L; | ||
| private SideEffectSet sideEffectSet; | ||
| private WrapperChunk<? extends IChunk> parentWrapper = null; |
There was a problem hiding this comment.
| private WrapperChunk<? extends IChunk> parentWrapper = null; | |
| private WrapperChunk<?> parentWrapper = null; |
IChunk already is the bound.
| final IChunkGet get = getOrCreateGet(); | ||
| final IChunkSet set = getOrCreateSet(); | ||
| if (parentWrapper == null) { | ||
| parentWrapper = new WrapperChunk<>(this, () -> this.extent.getOrCreateChunk(getX(), getZ())); |
There was a problem hiding this comment.
This is a bit weird as the WrapperChunk ctor already calls setWrapper? Can this be cleaned up somehow?
| if (this.parentWrapper != null) { | ||
| try { | ||
| this.parentWrapper.invalidate(this); | ||
| } catch (IllegalStateException ignored) { |
There was a problem hiding this comment.
Either it is valid for the invalidation to not succeed, in which case it shouldn't throw an exception but e.g., rather return a boolean, or we shouldn't silently ignore the exception.
| * @throws IllegalStateException if the expected {@link ChunkHolder} is not the currently wrapped chunk | ||
| */ | ||
| void invalidate(ChunkHolder<?> expected) { | ||
| if (this.chunk != null && expected != this.chunk) { |
There was a problem hiding this comment.
this.chunk is volatile, combined with this double read that's rather suspicious. What's the idea behind that? Why is it allowed to invalidate if the field is null?
| } | ||
|
|
||
| private T getWrapped() { | ||
| return chunk != null ? chunk : (chunk = supplier.get()); |
There was a problem hiding this comment.
This is also a double read from a volatile field, why can't the second read not return null? And why do we need volatile then?
| * @throws IllegalStateException if there is already a wrapper set and a new wrapper instance is attempted to be se | ||
| * @since TODO | ||
| */ | ||
| @ApiStatus.Internal |
There was a problem hiding this comment.
That annotation should probably be on both methods (or even on the interface?)
| if (newChunk == chunk) { | ||
| newChunk = chunk.get(); | ||
| } else { | ||
| chunk.setWrapped((T) newChunk); |
There was a problem hiding this comment.
How can this happen? Such an implementation of applyChunk would be type-unsafe itself. That sounds flawed.
There was a problem hiding this comment.
There's not necessarily anything stopping it, our generic types are pretty cursed so I figured it worth implementing just in case (it isn't an issue in FAWE, but that's only because we never do anything in Filter#applyChunk)
There was a problem hiding this comment.
Sounds like we should replace the signature to just return IChunk instead of U (and probably also just accept IChunk as a parameter, the method doesn't really benefit from U tbh). This would be binary compatible but not api compatible.
