Skip to content
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.

Sign up for GitHub

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

fix-count-entities #4356

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

fix-count-entities #4356

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a5de34d
fix-count-entities
ch4ika Feb 22, 2024
9c6078c
Simplify array assignment
ch4ika Feb 23, 2024
f8c7124
Fix entity filtering in chunk to match plot boundaries
ch4ika Feb 23, 2024
a120e49
Fix asynchronous loading issue causing premature count return
ch4ika Feb 23, 2024
9720a91
Implement asynchronous entity check
ch4ika Feb 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Bukkit/src/main/java/com/plotsquared/bukkit/listener/EntityEventListener.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -195,7 +195,7 @@ public void creatureSpawnEvent(CreatureSpawnEvent event) {
}
return;
}
if (BukkitEntityUtil.checkEntity(entity, plot.getBasePlot(false))) {
if (BukkitEntityUtil.checkEntity(entity, plot)) {
event.setCancelled(true);
}
}
Expand Down
72 changes: 12 additions & 60 deletions Bukkit/src/main/java/com/plotsquared/bukkit/util/BukkitRegionManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,19 +41,15 @@
import com.sk89q.worldedit.regions.CuboidRegion;
import com.sk89q.worldedit.world.block.BaseBlock;
import com.sk89q.worldedit.world.block.BlockTypes;
import io.papermc.lib.PaperLib;
import org.bukkit.Bukkit;
import org.bukkit.Chunk;
import org.bukkit.World;
import org.bukkit.entity.Entity;
import org.bukkit.entity.Player;
import org.checkerframework.checker.nullness.qual.NonNull;
import org.checkerframework.checker.nullness.qual.Nullable;

import java.util.ArrayList;
import java.util.HashSet;
import java.util.List;
import java.util.Set;

import static com.plotsquared.core.util.entity.EntityCategories.CAP_ANIMAL;
import static com.plotsquared.core.util.entity.EntityCategories.CAP_ENTITY;
Expand Down Expand Up @@ -88,73 +84,29 @@ public boolean handleClear(

@Override
public int[] countEntities(@NonNull Plot plot) {
int[] count = new int[6];
int[] existing = (int[]) plot.getMeta("EntityCount");
if (existing != null && (System.currentTimeMillis() - (long) plot.getMeta("EntityCountTime") < 1000)) {
return existing;
}
PlotArea area = plot.getArea();
World world = BukkitUtil.getWorld(area.getWorldName());
Location bot = plot.getBottomAbs();
Location top = plot.getTopAbs();
int bx = bot.getX() >> 4;
int bz = bot.getZ() >> 4;

int tx = top.getX() >> 4;
int tz = top.getZ() >> 4;

int size = tx - bx << 4;

Set<Chunk> chunks = new HashSet<>();
for (int X = bx; X <= tx; X++) {
for (int Z = bz; Z <= tz; Z++) {
if (world.isChunkLoaded(X, Z)) {
chunks.add(world.getChunkAt(X, Z));
}
}
}

boolean doWhole = false;
List<Entity> entities = null;
if (size > 200 && chunks.size() > 200) {
entities = world.getEntities();
if (entities.size() < 16 + size / 8) {
doWhole = true;
}
if(world == null) {
return count;
}

int[] count = new int[6];
if (doWhole) {
for (Entity entity : entities) {
org.bukkit.Location location = entity.getLocation();
PaperLib.getChunkAtAsync(location).thenAccept(chunk -> {
if (chunks.contains(chunk)) {
int X = chunk.getX();
int Z = chunk.getZ();
if (X > bx && X < tx && Z > bz && Z < tz) {
count(count, entity);
} else {
Plot other = area.getPlot(BukkitUtil.adapt(location));
if (plot.equals(other)) {
count(count, entity);
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 -> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@ch4ika ch4ika Feb 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I created an issue to keep track of it: #4362

final Entity[] entities = chunk.getEntities();
ch4ika marked this conversation as resolved.
Show resolved Hide resolved
for (Entity entity : entities) {
if (entity instanceof Player) {
continue;
}
}
}
});
}
} else {
for (Chunk chunk : chunks) {
int X = chunk.getX();
int Z = chunk.getZ();
Entity[] entities1 = chunk.getEntities();
for (Entity entity : entities1) {
if (X == bx || X == tx || Z == bz || Z == tz) {
Plot other = area.getPlot(BukkitUtil.adapt(entity.getLocation()));
if (plot.equals(other)) {
count(count, entity);
}
} else {
count(count, entity);
}
});
}
}
}
Expand Down
16 changes: 7 additions & 9 deletions Core/src/main/java/com/plotsquared/core/plot/Plot.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1220,15 +1220,13 @@ public boolean removeFlag(final @NonNull PlotFlag<?, ?> flag) {
*/
public int[] countEntities() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should probably deprecate the "old" sync methods if moving to async

int[] count = new int[6];
for (Plot current : this.getConnectedPlots()) {
int[] result = this.regionManager.countEntities(current);
count[CAP_ENTITY] += result[CAP_ENTITY];
count[CAP_ANIMAL] += result[CAP_ANIMAL];
count[CAP_MONSTER] += result[CAP_MONSTER];
count[CAP_MOB] += result[CAP_MOB];
count[CAP_VEHICLE] += result[CAP_VEHICLE];
count[CAP_MISC] += result[CAP_MISC];
}
int[] result = this.regionManager.countEntities(this);
count[CAP_ENTITY] += result[CAP_ENTITY];
count[CAP_ANIMAL] += result[CAP_ANIMAL];
count[CAP_MONSTER] += result[CAP_MONSTER];
count[CAP_MOB] += result[CAP_MOB];
count[CAP_VEHICLE] += result[CAP_VEHICLE];
count[CAP_MISC] += result[CAP_MISC];
ch4ika marked this conversation as resolved.
Show resolved Hide resolved
return count;
}

Expand Down
Loading