Alright, hopefully everyone is happy now

@md_5 the villager change is good. However the EAR change is not. You still have the problem that you may conditionally return false when flag is false, which is not allowed.

when flag is false, you absolutely must not return false, as it will skip the chunk registration step.

EAR logic needs to be reverted back to before it ever got changed. The Villager change solves the underlying problem and is all that was needed (though, I think it should probably only skip the door portion of the tick)

Ideally ,we should have a flag that we are ticking entities, and ignore flipping the boolean for getChunk during an entity tick and that would solve it across the board.

Thanks md_5. Your change looks like a reasonable solution to the problem

And villagers as discussed:
https://hub.spigotmc.org/stash/projects/SPIGOT/repos/spigot/commits/73ea0a853a3c721ab69705d9481b3108845b8a5c

I think that fixes all issues brought up in this ticket

And the previous behaviour was broken from a Vanilla perspective, so blanket reverting all these commits is also not the correct choice.

I've added this commit https://hub.spigotmc.org/stash/projects/SPIGOT/repos/spigot/commits/48c4baf7e9b27bfd029c9cfc32e2a5a3ad801bcd which should address the primary complaint about skipping ticking in a proper manner.

Further code proposals are welcome as PRs.

Apologies. I edited the comment so it should make more sense now.

I think you are right that other inactiveTick should be looked at to make sure they aren't preventing chunks from being unloaded. Luckily the number of things which are done in the inactive tick are relatively small and are contained to a single patch: https://hub.spigotmc.org/stash/projects/SPIGOT/repos/spigot/browse/CraftBukkit-Patches/0014-Entity-Activation-Range.patch

• EntityAgeable - looks like it makes the entity still age, looks like it may end up looking up chunks only if the entity changes size (i.e. grows up)
• EntityArrow - still ticks respawn counter, doesn't touch chunks
• EntityFireworks - still ticks flown counter, doesn't touch chunks
• EntityItem - still ticks despawn counter, doesn't touch chunks
• EntityLiving - only updates ticksFarFromPlayer, doesn't touch chunks
• EntityVillager - Calls logic in M() if tickInactiveVillagers is enabled. This does touch chunks to refresh doors

Villagers seem to be the only source of persistent chunk lookups as an ageable entity only grows up once. Villagers only lookup chunks if the tickInactiveVillagers option is enabled

I think you linked something wrong, as that commit is nearly 2 years old, those issues are recent.

Villagers are not going to be the only source of this.

Would need to make sure any inactiveTick method uses getChunkIfLoaded to avoid tripping active flag.

Just to be clear, the changes in https://hub.spigotmc.org/stash/projects/SPIGOT/repos/craftbukkit/commits/fb423b8f48335d0dc6c88cd8a24ee07d82ae09a8 were made after the reports (SPIGOT-3874) that the fix for SPIGOT-3846 caused chunks to remain permanently loaded. A configuration option was added to workaround this problem but this ticket was the actual root cause of the bug. I think we can have everyones bugfixes without compromising on performance for large servers on spigot

no, that is bad. inactive ticks are by designed to fix bugs in mojangs logic to maintain counters

If that villager inactive tick is causing harm, then only the villager inactive tick code should be modified to account for "are we on the edge"

Other entities are mandatory to run the inactive tick code.

Ah i managed to find the specific bugfix you are referring to: https://hub.spigotmc.org/stash/projects/SPIGOT/repos/spigot/commits/7127db5a842375b7e1b452e260ff6a6cb2537ddc and what you are describing about the behaviour makes sense however reverting another bug fix doesn't seem like the solution. The line could simply have the flag check reinstated.

The pseudo-code would be like this:

if (flag && !(in non-lazy chunk) {
    return;
}
if (flag && !(in activation range) {
    do inactive tick
} else {
    do active tick
}

heh  I actually found it

https://hub.spigotmc.org/stash/projects/SPIGOT/repos/spigot/commits/7127db5a842375b7e1b452e260ff6a6cb2537ddc

And I understand that the intent of that commit was to allow 'permaloaders' to work, but that in essence is a problem in that it keeps chunks held open that need to be unloaded.

That change was made in such a drastic way that lots of individual actions are now holding the chunks open.

That kind of behavior is not scalable for larger servers. People who want to run small servers with those kind of tiny vanilla details really should be using vanilla.

Also the said bug was a bug in EAR, so applicable to Spigot only.

I previously had the check just as it has now been made, which was the issue. Entities get stuck in incorrect chunks when you skip on flag=false

There's no reasonable way to find that commit.

when an entity is being ticked due to their routine world entity tick, flag is true.

this is the only time you should skip ticking.

When flag is false, is part of special operations such as teleporting, and the tick method runs in a way that doesnt run the full tick process, only the chunk registration process runs.

This operation should not ever be skipped... For one, it may be moving the entity OUT of a "lazy chunk".

Spigot was already handling it similar to Vanilla, in that when flag is true, and the entity is in an edge chunk, to skip ticking the entity. The only difference is we have a 1 chunk radius instead of 2, as many servers run with a 4 view distance and 2 is very devastating to entity ticking.

See the bottom of the checkIfActive method for this logic. 

This commit actually deviated it from vanilla and I highly doubt it even fixed the supposed issue.

I think I found the commit you are referring to: https://hub.spigotmc.org/stash/projects/SPIGOT/repos/craftbukkit/commits/1953f52da1ece0feb56dea20592c1b86616b31a5 It is the only one I could find which seems to be about chunk load and unload events not being triggered when a chunk loads or unloads. The change in the commit you referenced does not affect these events being triggered at the appropriate times.

Is the 'bug you fixed years ago' you are referring to a bug in Spigot/CraftBukkit or a bug in vanilla? The change in the commit referenced fixed unloading inconsistencies with vanilla. That is, it made CraftBukkit (and spigot) behave how vanilla would behave. Similarly this bug fix aligns spigot closer to how vanilla works. Entities receive no update if they are in lazy chunks in vanilla, Spigot then adds additional functionality which means that even within entity processing chunks (non-lazy ones) only the inactive tick portion would be invoked if the entity isn't within an activation range. Is there a reason why Spigot would not use the vanilla behaviour for lazy chunks and not process entities in them?

Could link the bug which you originally fixed? Perhaps we can explore fixing it a different way which doesn't require reintroducing bugs which we also being fixed.

Your proposed solution is incorrect and introduces bugs I fixed years ago.

Vanilla only skips ticking when flag = true, check the craftbukkit patch...

you ABSOLUTELY MUST NOT skip ticking if flag  = false, as this is the case where entities have moved chunks, and this will break chunk registration (the bug fixed years ago)

The real source of the bug is https://hub.spigotmc.org/stash/projects/SPIGOT/repos/craftbukkit/commits/fb423b8f48335d0dc6c88cd8a24ee07d82ae09a8 

that commit should be reverted.