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

Reduce Citizens ticking #2110

Closed
wants to merge 10 commits into from
Closed

Reduce Citizens ticking #2110

wants to merge 10 commits into from

Conversation

Cryptite
Copy link
Contributor

@Cryptite Cryptite commented Apr 16, 2020

This is only the first pass at a change to ticking Citizens and is likely incomplete so this PR seeks review and feedback.

What this PR aims to solve for is the quantity of ticking done to unmoving Citizens. Your average NPC is likely static and unmoving. Even if they have the LookClose trait, they still spend the vast majority of their time not looking at anybody or if they are, often that player might not be moving.

This PR reduces ticks by up to 1/20th the time as updates about static NPCs are largely unnecessary and the "Update at least once a second" paradigm is pretty common throughout tick-reduction PRs throughout Minecraft.

In effect, this PR will tick EntityHumanNPC under the following conditions:

  • The NPC has LookClose, it will evaluate whether the Player the NPC is looking at has moved, and only issue updates then to ensure the smooth head-turning aspect of the Trait.
  • Otherwise, It has been 20 ticks since the last update.

It is possible ticking could be reduce further, perhaps behind a config option as even 1s is probably too often to update NPCs who might never move, pathfind, have potion effects, change armor, etc.

One thing this doesn't account for and probably should is the Gravity trait.

On Loka with 30 players this has reduced the overall performance hit of the PlayerUpdateTask from 5% of the server tick to around or less than 1%.

As an unrelated addition this PR also fixes a problem where hasInvalidTarget was not checking the squared range and so was invalidating its target each tick.

@SXRWahrheit
Copy link

It is generally considered good etiquette to discuss whether opening a PR is appropriate on Discord before submitting one at all.

@mcmonkey4eva mcmonkey4eva marked this pull request as draft April 16, 2020 17:57
@mcmonkey4eva
Copy link
Member

This is functionally unpullable until it either
A: Accounts for all potentially relevant factors (Gravity trait being a good example)
or B: Is configurable and defaults to off (ie no change in ticking compared to the current way of things).

Until one of those is met, this has a very high risk of causing issues on any servers that update to a build containing this PR.

This could probably benefit from being configurable on a per-NPC basis in addition to a config.yml entry (for default/new NPCs).

I'd advise B (configurable and default off) as the way to go and be safe, but an attempt at A to account for more likely issues would be nice (but can be delayed for later commits or PRs if B is already in place, as it would be limitations of an option rather than fundamentally breaking existing things).


Side note, but I'd encourage you to check over your git or editor settings to avoid recommitting entire files with changed line endings. Not super important but would be nice.

@fullwall
Copy link
Member

fullwall commented Apr 16, 2020

I'd probably split this PR into the lookclose changes and the shouldTick changes. Currently the shouldTick changes are really broad. With the lookclose change, I'd probably a) use the Entity#getLocation(Location) method and b) use distanceSquared > some minimum delta instead of equals. Have you profiled just the lastKnownLocation change in isolation?

Also, please fix the line endings.

@mcmonkey4eva
Copy link
Member

@Cryptite do you plan to revise this PR per the comments above?

@Cryptite
Copy link
Contributor Author

It may be some time until I can return to this. Busy updating my server to 1.15 among some other things. If there's a desire to close this in favor of a more focused PR (or two) in the future that is fine by me.

@Rincewind34

This comment has been minimized.

@CLAassistant
Copy link

CLAassistant commented Aug 2, 2020

CLA assistant check
All committers have signed the CLA.

# Conflicts:
#	main/src/main/java/net/citizensnpcs/npc/CitizensNPC.java
# Conflicts:
#	main/src/main/java/net/citizensnpcs/npc/CitizensNPC.java
#	main/src/main/java/net/citizensnpcs/trait/LookClose.java
@HexedHero
Copy link

Any news on this or is this dead?

@Cryptite
Copy link
Contributor Author

I'm going to close this as I don't plan to revisit this in the near future. I believe some of the changes to LookClose were implemented quite awhile ago in Citizens, and my ticking changes aren't stable or well thought-out enough to handle other cases.

If this were revisited, it should likely be a fresh PR.

@Cryptite Cryptite closed this Nov 16, 2020
@fullwall
Copy link
Member

Thanks for the PR anyway, it definitely raises some points.
@HexedHero do you have a new performance issue?

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

Successfully merging this pull request may close these issues.

7 participants