-
Notifications
You must be signed in to change notification settings - Fork 477
Add ServerChunkEvents.CHUNK_LEVEL_TYPE_CHANGE #4541
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
Add ServerChunkEvents.CHUNK_LEVEL_TYPE_CHANGE #4541
Conversation
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.
Edit: I am probably wrong about the threads that the level changes take place, since I was confusing this with chunks actually becoming available to the world, including the difference of getChunk and getChunkNow.
I think implementing this without major changes is a bad idea due to the missing explanation of the meaning of the chunk level type changes.
I believe that if understood correctly, the chunk level type is NOT what you want to track, as the status that is being tracked here only describes whether the game aims to load a chunk, but not whether the chunk is actually being loaded or available to the general game logic.
In my mod, lithium, which tracks whether a chunk is accessible in the world, I had several issues tracking this status correctly. I managed to get all events fire on the server thread, to avoid threading issues. Furthermore, Lithium (almost correctly) tracks whether the chunk is accessible in the world, instead of whether the game tries to make it accessible (status set but chunk future not done yet). I believe that if the API just tracks the level/status, many authors will incorrectly assume that this information is sufficient to know that a chunk is available in the world.
Including the other load statuses, especially regarding entity ticking, adds another layer of complexity and unclear meanings, requiring even more clear documentation.
|
|
||
| @Inject(method = "increaseLevel", at = @At("TAIL")) | ||
| private void increaseLevel(ServerChunkLoadingManager chunkLoadingManager, CompletableFuture<OptionalChunk<WorldChunk>> chunkFuture, Executor executor, ChunkLevelType target, CallbackInfo ci) { | ||
| ServerChunkEvents.CHUNK_LEVEL_TYPE_CHANGE.invoker().onChunkLevelTypeChange((ServerWorld) world, this.pos, ChunkLevels.getType(this.lastTickLevel), target); |
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.
On which thread should this be called, and should this be ensured with a similar check as in
https://github.com/CaffeineMC/lithium/blob/ebcbdb134365ab1862461548f857cdecf96e4d31/common/src/main/java/net/caffeinemc/mods/lithium/common/world/chunk/ChunkStatusTracker.java#L48
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 done some testing and it does seem like the event is always consistently called on the main server thread.
| /** | ||
| * Called when a chunk changes its {@link ChunkLevelType}. | ||
| * | ||
| * <p>When this event is called, the chunk's level type has already changed. |
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.
Which meaning does changing the chunk level type have?
There might be many misconceptions about this, leading to lots of incorrect implementations. The code comment is very minimal and doesn't indicate any potential issues.
For example, if a mod author believes the chunk status indicates one of the following, there will likely be major issues:
Whether the chunk is loaded, whether entities are ticking in that chunk, whether entities are loaded in that chunk, whether the chunk is ticking blocks in that chunk, etc.
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 updated the documentation of the event to reflect the changes.
...ycle-events-v1/src/main/java/net/fabricmc/fabric/mixin/event/lifecycle/ChunkHolderMixin.java
Show resolved
Hide resolved
|
This is a side note but
These are all level decreases, and I don't think this changes the actual functionality of the event but just pointing it out. |
|
Rewording my concerns: |
…hunk's future is actually completed.
|
The event is now called when the chunk future has actually completed, and is called on the main thread's executor. I added some rough logging tests to verify the chunk's behaviour aligns with the level type but I'm honestly not sure how to test such a thing correctly. This has been implemented but there is a potential issue:
If a chunk goes straight from ENTITY_TICKING to INACCESSIBLE, and all the intermediate level types are artificially called, then the calls for ENTITY_TICKING -> BLOCK_TICKING and BLOCK_TICKING -> FULL may make modders assume the chunk is still accessible when it really isn't. |
…w levelTypes are actually sequential
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.
Im not familar at all with the code this is targgeting, if one of my comments doesnt make senese please tell me.
...-v1/src/testmod/java/net/fabricmc/fabric/test/event/lifecycle/ServerChunkLifecycleTests.java
Outdated
Show resolved
Hide resolved
...-v1/src/testmod/java/net/fabricmc/fabric/test/event/lifecycle/ServerChunkLifecycleTests.java
Outdated
Show resolved
Hide resolved
...-v1/src/testmod/java/net/fabricmc/fabric/test/event/lifecycle/ServerChunkLifecycleTests.java
Outdated
Show resolved
Hide resolved
...-v1/src/testmod/java/net/fabricmc/fabric/test/event/lifecycle/ServerChunkLifecycleTests.java
Outdated
Show resolved
Hide resolved
|
|
||
| if (!levelTypes.isEmpty()) { | ||
| StringBuilder sb = new StringBuilder(world.getRegistryKey().getValue() + " "); | ||
| levelTypes.forEach((levelType, integer) -> sb.append(levelType).append(": ").append(integer).append(", ")); |
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.
Its not immediately clear what we are logging here? Is it the number of level type changes in the last second?
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.
Yes, it is the number of events fired for each ChunkLevelType, specifially only shows the newLevelType for brevity. I've added a comment there and named the lambda params a bit more descriptively.
|
|
||
| chunkFuture.thenAccept(optionalChunk -> optionalChunk.ifPresent(worldChunk -> { | ||
| if (!server.isOnThread()) { | ||
| server.send(new ServerTask(server.getTicks()-10, runnable)); // ensure the server thread runs it within this tick |
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.
Whats the -10 for?
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.
Basically it kinda tricks the server into thinking the task was sent 10 ticks ago. Since the server always executes any tasks sent over 3 ticks ago (even if the server is lagging), this should guarantee it executes within the same tick.
See MinecraftServer#canExecute
protected boolean canExecute(ServerTask serverTask) {
return serverTask.getCreationTicks() + 3 < this.ticks || this.shouldKeepTicking();
}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.
Does this mean that mods using the API cannot rely on the event firing as soon as the chunk becomes available, otherwise risking that their datastructures might be inconsistent with the world for up to a almost an entire gametick?
Can you have a usecase that the API will be helpful for, because I don't understand it the way it is
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.
Firing the event immediately would be ideal but I have no idea how it's possible to get the server thread to do that.
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.
In 1.21.5 mojang mappings I suggest injecting at the end or close to the end of ChunkStatusTasks.full (the last part of the chunk loading futures, which actually adds block entities etc. to the world), allowing the event to fire on the server thread just before the future completes. In lithium I inject into setFullStatus, because I wanted the event before block entities are added
| }; | ||
|
|
||
| Runnable runnable = () -> { | ||
| if (this.getLevelType().isAfter(target)) { // If chunk level type got demoted before promotion event fires, then don't fire it. This almost never happens tho. |
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.
Is this hiding a bug? Is this a timing issue as you get the levelType in a later tick than when the task was created?
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.
Sort of. This is to cater for the rare edge case where the chunk's level type gets demoted before a prior promotion event gets fired.
e.g.
Chunk goes from INACCESSIBLE -> FULL, promotion event is waiting to fire on server thread.
During the wait chunk goes back to INACCESSIBLE for whatever reasons, demotion event fires immediately.
Promotion event finally fires on server thread, so users get FULL as the final status, when the chunk is actually INACCESSIBLE now.
In practice from my testing, this never happened at all, but I still put this here just in case.
...ycle-events-v1/src/main/java/net/fabricmc/fabric/mixin/event/lifecycle/ChunkHolderMixin.java
Outdated
Show resolved
Hide resolved
|
Consider taking a look at Savva's chunk loading rant videos (https://www.youtube.com/@savva4424/videos), which document various chunk loading issues. I think avoiding all pitfalls with the API is not possible, so some tradeoffs have to be made |
|
I've been making some progress on overhauling the current implementation but I've run into an issue.
My new implementation does this, but I realised that after the chunk future has completed, there is still one last callback that runs, which is the // mojmap: ChunkHolder.scheduleFullChunkPromotion
private void increaseLevel(ServerChunkLoadingManager chunkLoadingManager, CompletableFuture<OptionalChunk<WorldChunk>> chunkFuture, Executor executor, ChunkLevelType target) {
this.levelIncreaseFuture.cancel(false);
CompletableFuture<Void> completableFuture = new CompletableFuture();
completableFuture.thenRunAsync(() -> chunkLoadingManager.onChunkStatusChange(this.pos, target), executor);
this.levelIncreaseFuture = completableFuture;
chunkFuture.thenAccept(optionalChunk -> optionalChunk.ifPresent(chunk -> completableFuture.complete(null)));
}
So i'm not really sure what to do here. |
|
There is no good way to fire right after the future is completed. I suggest to document this behavior and (probably) make another event that fires at On the side note, |
I'm not quite sure what you mean by this. I understood that ChunkStatusTasks.full can only be used to fire INACCESSIBLE->FULL right before the chunk future completes. In the new implementation, FULL->BLOCK_TICKING is fired right when the tickingFuture completes, and the tickingFuture always runs on the server thread so that works out. Then BLOCK_TICKING->ENTITY_TICKING is fired when the entity tracking statuses have been updated (which may be some time after the entityTickingFuture completes), since only after that do entities actually start ticking. |
When your callback is fired, the future have already been completed for an undefined amount of time, even if the future is completed on the main thread. You can't prevent mods from doing silly stuff. |
|
I've completed rewritten the entire implementation. Here's how it works now: INACCESSIBLE->FULL fires similarly to how Lithium does it. I've tried to add extra checks to prevent duplicate fires but there's still duplicate events firing occasionally. FULL->BLOCK_TICKING fires right after the tickingFuture is complete. BLOCK_TICKING->ENTITY_TICKING fires after the FULL->BLOCK_TICKING event. At this point, entities are not actually ticking yet. I'm still not sure how I feel about this since the event might potentially fire a few gameticks before entities start ticking. From my understanding, entityTickingFuture loads in chunks around the chunk that's becoming ENTITY_TICKING, so I don't really see where I could hook into to fire the event besides the onChunkStatusChange callback. The demotion events now only fire if a previous equivalent promotion event has actually fired. Some tests were also added to ensure all the events are fired in-sync and in a logical order. |
|
I now realise that with the current implementation, for all promotion events (INACCESSIBLE->FULL, FULL->BLOCK_TICKING, BLOCK_TICKING -> ENTITY_TICKING) there is no hard guarantee that entities are actually accessible at all. Realistically there is no way to provide that guarantee unless the events are fired after onChunkStatusChange(). I'm wondering if there is a need to split the event into two, one that fires "before" and one "after", if that makes sense. The after event would have to fire at the tail of the onChunkStatusChange() lambda, though it was noted that onChunkStatusChange() is buggy so it's not clear that will work well. At this point, I think it's necessary to nail down the semantics of the API before making any further code changes. |
|
For this I suggest finding out what the usecases for mod authors would be. Seems like there are multiple places in the code, e.g. mojmap: Some entities are also created during chunk generation (not as nbt), e.g. cats (or was it camels?) that are generated with villages. Maybe extra care is needed there. (Players have their own code paths for adding them to entity sections) |
|
I think the before event would be useful for most use cases. When the events are called entities are already loaded, just not accessible via the various ServerWorld.getEntity() methods. I tried looking into how chunk level type changes actually affects the game's processes, and this is what I understood so far: FULL -> BLOCK_TICKING
BLOCK_TICKING -> ENTITY_TICKING
This makes me think that we should keep INACCESSIBLE->FULL where it is now, and fire the other two promotion events after INACCESSIBLE->FULL regardless of whether the corresponding chunk future has completed. The documentation would have to clarify the entity accessibility issue and that the events are fired before the chunk's behaviour is 100% aligned with its level type. This would allow mods to prepare for the new level type kinda like what ishland said here. |
|
The implementation has been redone as per my previous comment. The event now also provides a non-null WorldChunk instead of just a ChunkPos. INACCESSIBLE->FULL also no longer accidentally fires duplicate events. |
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 don't see any immediate problems for the implementation of the API for now. The javadoc might need some clarification.
| /** | ||
| * Called when a chunk changes its {@link ChunkLevelType}. | ||
| * | ||
| * <p>When this event is called, the chunk's {@link WorldChunk#getLevelType()} has already changed. However, the chunk's actual ticking behavior may not fully align with its level type yet. | ||
| * Additionally, it is not guaranteed that entities from the chunk are immediately accessible when this event is called. | ||
| */ |
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.
The javadoc sound very vague.
I would document it as something like this:
Called when a chunk's actual ticking behavior is about to align with its {@link ChunkLevelType}.
And if you want to go further into the explanation: (I have added a few more points here)
When this event is being called:
<ul>
<li>The chunk's {@link WorldChunk#getLevelType()} has already changed. </li>
<li>Entities within the chunk are not guaranteed to be accessible. </li>
<li>The {@link CompletableFuture} corresponding to the level type is not guaranteed to be done. </li>
<li>When transitioning from {@link ChunkLevelType.INACCESSIBLE} to {@link ChunkLevelType.FULL}, calling {@link ServerChunkManager#getChunk(int, int, ChunkStatus, boolean)} to fetch the current chunk at {@link ChunkStatus#FULL} status results in undefined behavior. </li>
</ul>
Regarding the last point, doing that in the vanilla system is almost certain to cause a deadlock. But from the standpoint of an alternative chunk system implementation, I would prefer a more relaxed interpretation of this behavior. (For example instead of deadlocking, throwing an exception instead)
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.
<li>The chunk's corresponding level type future in {@link ChunkHolder} is not guaranteed to be done.</li>I think mentioning ChunkHolder is necessary since it's not clear to people unfarmiliar with the topic what future is being referred to.
For the last point, I understand the deadlock problem but I don't see any reason for event users to call that method since the WorldChunk is already provided by the event.
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.
Note that neoforge does have a fix for it (by just providing the chunk object of the not yet finished future (ugh)), so some multi-platform mods may just try to access the chunk via the world anyway.
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 mentioning ChunkHolder is necessary since it's not clear to people unfarmiliar with the topic what future is being referred to.
Agreed.
For the last point, I understand the deadlock problem but I don't see any reason for event users to call that method since the WorldChunk is already provided by the event.
That's an easy mistake to make. So might just as well include it in the javadoc.
As a side note, NeoForge shouldn't even allow this kind of behavior in the first place. Such bandaid can have unintended side effects as it is presenting almost-full chunks as full chunks.
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 committed the javadoc changes. CHUNK_LOAD and CHUNK_UNLOAD javadocs have also been updated with some extra clarifications.
|
If there are no other issues, then this API should be ready to go. |
...ycle-events-v1/src/main/java/net/fabricmc/fabric/mixin/event/lifecycle/ChunkHolderMixin.java
Outdated
Show resolved
Hide resolved
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.
Im happy with this, I cannot really comment on the implimentation too much. Ill put this into last call, if there are any further concerns please make them be known!
* Add ServerChunkEvents.CHUNK_LEVEL_TYPE_CHANGE * Fix naming * Amend ServerChunkEvents#CHUNK_UNLOAD documentation to clarify server chunk unload timing. * License header and checkstyle * Call event for intermediate level type demotion and call event when chunk's future is actually completed. * Only fire the event async if not already on server thread * When the event fires to promote ChunkLevelType, ensure the old and new levelTypes are actually sequential * If chunk level type got demoted before promotion event fires, then don't fire it. * Update documentation for the event. * test cleanup and use for loop for decreaseLevel * more tests * it mostly works except BLOCK_TICKING->ENTITY_TICKING is still firing off-thread * BLOCK_TICJING -> ENTITY_TICKING now fires on server thread * checkstyle * some javadoc * BLOCK_TICKING->ENTITY_TICKING fires earlier now, and enhance tests * checkstyle * review comments * handle exception instead of letting completeablefuture swallow it * complete rewrite * improve tests and some doc edits * add test to verify all chunks in all worlds are INACCESSIBLE when server stopped. * javadoc changes from review * use @OverRide instead of @unique (cherry picked from commit 230071a)
* Add ServerChunkEvents.CHUNK_LEVEL_TYPE_CHANGE * Fix naming * Amend ServerChunkEvents#CHUNK_UNLOAD documentation to clarify server chunk unload timing. * License header and checkstyle * Call event for intermediate level type demotion and call event when chunk's future is actually completed. * Only fire the event async if not already on server thread * When the event fires to promote ChunkLevelType, ensure the old and new levelTypes are actually sequential * If chunk level type got demoted before promotion event fires, then don't fire it. * Update documentation for the event. * test cleanup and use for loop for decreaseLevel * more tests * it mostly works except BLOCK_TICKING->ENTITY_TICKING is still firing off-thread * BLOCK_TICJING -> ENTITY_TICKING now fires on server thread * checkstyle * some javadoc * BLOCK_TICKING->ENTITY_TICKING fires earlier now, and enhance tests * checkstyle * review comments * handle exception instead of letting completeablefuture swallow it * complete rewrite * improve tests and some doc edits * add test to verify all chunks in all worlds are INACCESSIBLE when server stopped. * javadoc changes from review * use @OverRide instead of @unique
* Add ServerChunkEvents.CHUNK_LEVEL_TYPE_CHANGE * Fix naming * Amend ServerChunkEvents#CHUNK_UNLOAD documentation to clarify server chunk unload timing. * License header and checkstyle * Call event for intermediate level type demotion and call event when chunk's future is actually completed. * Only fire the event async if not already on server thread * When the event fires to promote ChunkLevelType, ensure the old and new levelTypes are actually sequential * If chunk level type got demoted before promotion event fires, then don't fire it. * Update documentation for the event. * test cleanup and use for loop for decreaseLevel * more tests * it mostly works except BLOCK_TICKING->ENTITY_TICKING is still firing off-thread * BLOCK_TICJING -> ENTITY_TICKING now fires on server thread * checkstyle * some javadoc * BLOCK_TICKING->ENTITY_TICKING fires earlier now, and enhance tests * checkstyle * review comments * handle exception instead of letting completeablefuture swallow it * complete rewrite * improve tests and some doc edits * add test to verify all chunks in all worlds are INACCESSIBLE when server stopped. * javadoc changes from review * use @OverRide instead of @unique
Upstream has released updates that appear to apply and compile correctly Upstream Changes: 9d042bab fix: clear "broken" flag when unloaded but not removed 4f2326f7 fix: mitigate sodium bug in another way 7684ab5d new: add elapsed time to task dump bda4198f change: cleanup retry stacktraces f2707da4 Merge branch 'feature/fully-threaded-sched' into dev/1.21.5 7e571abb perf: track ticket size in a separate array 7870d4be fix: missed synchronization 86977bbf change: rename lowMemoryMode to !useLegacyScheduling 06c51bb8 perf: reduce background executor dispatch de8f6d7e change: enable lowMemoryMode by default c91858de perf: more faster 9207f963 perf: consolidate level update notifications eb9ff302 change: use inlined AtomicBoolean 47147991 fix: another fix for concurrency 44b2b53a perf: ever so slightly faster 90ea98f8 fix: small leak 38647703 perf: slightly more perf 7b58b1da fix: some performance issues 92a710f4 new: fully threaded scheduling 2324fe6c fix: never downsize global map 5e526126 build: 0.3.4+alpha.0 dbb3119f new: implements FabricMC/fabric#4541 3af69783 build: use jdk24 for builds 0432c593 new: preventEarlyClientMovementTicks 4c060875 fix: some more adjustment to fluid postprocessing filter 110b645b fix: oops moment 12b859b1 perf: filterFluidPostProcessing 606f5f58 fix: correct failfast behavior b945d666 chore: toString for some statuses
Add a new event that fires when a chunk's level type changes. (Relates to #4507)
Changed the name from
CHUNK_STATUS_CHANGEas to not confuse it with theChunkStatusclass which refers to chunk generation stages.The event only provides a
ChunkPosand not aWorldChunk. If really needed, a potentially nullWorldChunkcan be obtained viaChunkHolder#getLatest()but I do not think it's necessary.The documentation of
CHUNK_UNLOADhas also been updated to clarify server chunk unload timing.