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.

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

Conversation

ch4ika
Copy link
Contributor

@ch4ika ch4ika commented Feb 22, 2024

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.

@ch4ika ch4ika requested a review from a team as a code owner February 22, 2024 23:25
@github-actions github-actions bot added the Bugfix This PR fixes a bug label Feb 22, 2024
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

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

Core/src/main/java/com/plotsquared/core/plot/Plot.java Outdated Show resolved Hide resolved
}

BukkitEntityUtil.checkEntityAsync(entity, plot.getBasePlot(false)).thenAcceptAsync(toRemove -> {
if(toRemove) {
Copy link
Member

Choose a reason for hiding this comment

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

code style

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);
Copy link
Member

Choose a reason for hiding this comment

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

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() {
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

Copy link

Please take a moment and address the merge conflicts of your pull request. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Plot limit can be overwritten.
4 participants