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

Optimise collision box checks by caching some basic info #6606

Open
wants to merge 8 commits into
base: minor-next
Choose a base branch
from

Conversation

dktapps
Copy link
Member

@dktapps dktapps commented Jan 27, 2025

This PR significantly improves performance of entity movement calculation.

Previous attempts to optimise this were ineffective, as they used a cache to mitigate the cost of recomputing AABBs. Entities tend to move around randomly, so the non-cached pathway really needed to be optimized.

This change improves performance on multiple fronts:

  1. avoiding Block allocations for blocks with 1x1x1 AABBs and with no AABBs (the most common)
  2. avoiding Block allocations and overlapping intersection checks unless a stateID is specifically known to potentially exceed its cell boundaries (like fences)
  3. avoiding overlapping AABB checks when overlaps can't make any difference anyway (cubes)

Together, these changes improve the performance of item entity movement (non-cached) by about 200%. We may be able to get further performance gains, but this is a good start.

Related issues & PRs

Changes

API changes

Added public RuntimeBlockStateRegistry->collisionInfo and related constants

Behavioural changes

This change is built on the assumption that Block->recalculateCollisionBoxes() and its overrides don't interact with any world. This is technically possible due to the crappy design of the Block architecture, but should be avoided. As a world is not available during RuntimeBlockStateRegistry initialization, attempting to interact with a world during recalculateCollisionBoxes() will now cause a crash.

This turned out to be a problem for ChorusPlant, which was fixed by 70fb9bb. The correct solution in this case was to use dynamic states similar to how we currently deal with fence connections.

As a result of this change, it's possible plugins implementing custom blocks may break if their blocks' AABB calculations depend on $this->position. However, I don't expect this to be an issue, and the problem will become quickly apparent during startup anyway.

Given the massive performance advantage on offer from this change, I think this is an OK risk to take.

Backwards compatibility

See above section on behavioural changes.

Follow-up

We may be able to get further improvements by tracking more info about oversized blocks, but this is a good enough start.

In the future, it might be worth requiring blocks to explicitly declare something like Block->mayExceedCellBounds() : bool when their AABBs could go outside the cell bounds, and have this enforced, throwing errors if recalculateCollisionBoxes() ever generates out-of-bounds AABBs. However, this would be tricky to implement due to the headaches presented by getModelPositionOffset(), which can currently offset an AABB outside of the block's cell.

We also really, really need to implement #6551. It should never have been possible to implement position-dependent logic in recalculateCollisionBoxes. This problem has appeared in a variety of issues by now.

Tests

Tested in-game with item entities. My local server started to lag without the change at around 400 dropped items; with the change, around 1200.

This change improves performance on 3 fronts:
1) avoiding Block allocations for blocks with 1x1x1 AABBs and with no AABBs (the most common)
2) avoiding Block allocations and intersection checks unless a stateID is specifically known to potentially exceed its cell boundaries

Together, these changes improve the performance of item entity movement (non-cached) by about 200%.
We may be able to get further performance gains, but this is a good start.
@dktapps dktapps requested a review from a team as a code owner January 27, 2025 22:23
@dktapps dktapps changed the title Specialize collision box checks by caching some basic info Optimise collision box checks by caching some basic info Jan 27, 2025
pmmp-admin-bot[bot]
pmmp-admin-bot bot previously approved these changes Jan 27, 2025
pmmp-admin-bot[bot]
pmmp-admin-bot bot previously approved these changes Jan 27, 2025
pmmp-admin-bot[bot]
pmmp-admin-bot bot previously approved these changes Jan 27, 2025
we don't need this var for cubes anyway
pmmp-admin-bot[bot]
pmmp-admin-bot bot previously approved these changes Jan 27, 2025
@dktapps dktapps added Category: Core Related to internal functionality Type: Enhancement Contributes features or other improvements to PocketMine-MP Performance labels Jan 27, 2025
@dktapps
Copy link
Member Author

dktapps commented Jan 28, 2025

Linking #6605 since dynamic states are also a confounding factor - anything which lazy-updates on read may also have unusual AABBs

@dktapps dktapps added Status: Blocked Depends on other changes which are yet to be completed Status: Incomplete Work in progress and removed Status: Blocked Depends on other changes which are yet to be completed labels Jan 28, 2025
pmmp-admin-bot[bot]
pmmp-admin-bot bot previously approved these changes Feb 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category: Core Related to internal functionality Performance Status: Incomplete Work in progress Type: Enhancement Contributes features or other improvements to PocketMine-MP
Projects
Development

Successfully merging this pull request may close these issues.

1 participant