fix-count-entities #4356
fix-count-entities #4356
Conversation
| for (final CuboidRegion region : plot.getRegions()) { | ||
| for (int x = region.getMinimumPoint().getX() >> 4; x <= region.getMaximumPoint().getX() >> 4; x++) { | ||
| for (int z = region.getMinimumPoint().getZ() >> 4; z <= region.getMaximumPoint().getZ() >> 4; z++) { | ||
| world.getChunkAtAsync(x, z).thenAccept(chunk -> { |
There was a problem hiding this comment.
loading chunks async will break things; count is returned before the chunk is loaded (that was flawed before too, I guess). For very large plots, this might be important, but that means we either need to come up with something clever or load all chunks before starting counting.
There was a problem hiding this comment.
I thought about it and played around, but unfortunately I can't really think of a simple idea that would allow for performance-friendly loading for larger plots without making any really big changes.
The whole thing is now in sync and works perfectly, but would cause lag on larger properties.
Unfortunately, the way it is currently being done is not the right solution either.
There was a problem hiding this comment.
The solution would be to query the async and if it overwrites the caps, remove the entity.
There would still be a meta missing, in the time when it hasn't been removed yet, that you can't remove or kill it.
Before I continue with it, the question is whether it makes sense or whether there is a better option.
There was a problem hiding this comment.
I think the short-term best solution is to just ignore unloaded chunks. That will result in the same behavior as the current code if I didn't miss anything. It also isn't a problem for a vast majority of servers I guess.
Long-term, we probably need to rethink the concept or come up with a solution to store the entity count of unloaded chunks somehow.
Removing entities later likely is a bad idea, as we would need to track how the entity was created (i.e. via spawn eggs, you'd need to give the spawn egg back to the player, etc...).
There was a problem hiding this comment.
Thanks for the feedback. As an alternative, I'm also thinking of caching the whole thing somehow.
Because the solution with checkAsync works, but as you already said, too many additional checks would have to be made and the events would not really be cancelable.
The whole thing is breaking my head. If anyone wants to give that, I would be grateful. Otherwise, I would try it again as soon as I have time.
There was a problem hiding this comment.
You could probably just open an issue for it so we can keep track of it. I don't think it's worth to spend too much time on it right now, the fix for merged plots is far more important.
There was a problem hiding this comment.
The problem is I had already made several tickets about it, but @OneLiteFeather kept marking them as invalid because she didn't understand the problem and wasn't open to discussing it.
There was a problem hiding this comment.
We are open for discussions but not mixing up contexts or creating incorrect issues. Also we do no private support including voicechat and discord dms.
There was a problem hiding this comment.
Anyway, you can now be sure that this problem exists and that you can reproduce it without any problems. I'll write another proper ticket in a few days to record it.
There was a problem hiding this comment.
I created an issue to keep track of it: #4362
Bukkit/src/main/java/com/plotsquared/bukkit/util/BukkitRegionManager.java
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| BukkitEntityUtil.checkEntityAsync(entity, plot.getBasePlot(false)).thenAcceptAsync(toRemove -> { | ||
| if(toRemove) { |
| for (final CuboidRegion region : plot.getRegions()) { | ||
| for (int x = region.getMinimumPoint().getX() >> 4; x <= region.getMaximumPoint().getX() >> 4; x++) { | ||
| for (int z = region.getMinimumPoint().getZ() >> 4; z <= region.getMaximumPoint().getZ() >> 4; z++) { | ||
| Chunk chunk = world.getChunkAt(x, z); |
There was a problem hiding this comment.
Can this method actually be called async?
| @@ -1219,17 +1212,11 @@ public boolean removeFlag(final @NonNull PlotFlag<?, ?> flag) { | |||
| * @see RegionManager#countEntities(Plot) | |||
| */ | |||
| public int[] countEntities() { | |||
There was a problem hiding this comment.
Should probably deprecate the "old" sync methods if moving to async
|
Please take a moment and address the merge conflicts of your pull request. Thanks! |
Fixes #4339
Description
I don't know what the code did exactly before, but it definitely didn't work for merged plots.
This change works and ensures that the caps are added up correctly.