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

Skip sync chunk load for MoveToBlock (fixes #6045) #6218

Closed
wants to merge 3 commits into from

Conversation

PaulBGD
Copy link
Contributor

@PaulBGD PaulBGD commented Jul 17, 2021

Any goal which extends this does a sync chunk/block lookup, which for
mobs close to the loaded chunk border will repeatedly lag the main
thread loading these.

This has been tested on production servers for a few weeks without any issues.

Any goal which extends this does a sync chunk/block lookup, which for
mobs close to the loaded chunk border will repeatedly lag the main
thread loading these.
@PaulBGD PaulBGD requested review from a team as code owners July 17, 2021 22:26
@PaulBGD
Copy link
Contributor Author

PaulBGD commented Jul 19, 2021

Bump!

Copy link
Contributor

@Phoenix616 Phoenix616 left a comment

Choose a reason for hiding this comment

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

Looks good to me. The only thing that might be an issue at some point is that Level#hasChunk(BlockPos) is deprecated but that shouldn't be hard to replace if it really gets removed at some point.

@jpenilla
Copy link
Member

I tested this locally, adding a printout when !this.mob.level.hasChunkAt(mutableBlockPos) evaluated to true, and after flying around for quite a while and then letting the server idle for a while longer, I didn't see any printouts at all in console.

I'm not sure if maybe the issue here is an edge case, or if certain settings are needed to reproduce it? I was testing with everything completely default.

If someone can provide some test results different to mine, or if someone having the issue can enable sync load debug and show some evidence this is sync loading chunks, I'm happy to merge.

@PaulBGD
Copy link
Contributor Author

PaulBGD commented Jul 21, 2021

Testing this out, I came to the same conclusion. I was wondering if the issue could be replicated by moving quickly, so that a chunk next to a fox wouldn't be loaded and it would sync load it. But I couldn't get that to reproduce either.

My best guess is with entities and chunks being decoupled in 1.17 that the issue this "fixed" occurred from an entity being ticked in a non-ticking chunk. Given that the MoveToBlockGoal always runs immediately on the 1st tick a mob is ticked for, it's likely that it's either this edge case or the moving fast/warping edge case with the nearby chunk not loaded yet.

This might still be a good fix knowing that these edge cases exist, although personally I think it'd be better to fix the core issue.

@electronicboy
Copy link
Member

A good chunk of this stuff is generally solved by the ticketing system in general, the extra decoupling may also help with the tracking aspects of this too, generally there are some edge cases where an entity is ticked when we don't expect it to, e.g. entities nearing the "loaded" boundary is always iffy

@PaulBGD
Copy link
Contributor Author

PaulBGD commented Jul 21, 2021

e.g. entities nearing the "loaded" boundary is always iffy

Especially on a busy server where chunks might be loading slower it's probably more of an issue. One potential "fix" for this which might be more natural is to just not tick MoveToBlockGoal on the initial mob tick, have it initially delayed by 10 ticks or something. I can imagine a lot of edge cases where looking up a 12x12 blocks around the mob on initial spawn might be a little sketchy.

@Machine-Maker Machine-Maker linked an issue Jul 22, 2021 that may be closed by this pull request
3 tasks
@stale
Copy link

stale bot commented Sep 19, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@PaulBGD
Copy link
Contributor Author

PaulBGD commented Sep 19, 2021

Probably an edge case, if I ever see another report of this I'll consider updating this then.

@PaulBGD PaulBGD closed this Sep 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fox pathfinding loads chunks
4 participants