-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
dktapps
wants to merge
8
commits into
minor-next
Choose a base branch
from
collision-box-specialization
base: minor-next
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+156
−23
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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
changed the title
Specialize collision box checks by caching some basic info
Optimise collision box checks by caching some basic info
Jan 27, 2025
we don't need this var for cubes anyway
dktapps
added
Category: Core
Related to internal functionality
Type: Enhancement
Contributes features or other improvements to PocketMine-MP
Performance
labels
Jan 27, 2025
Linking #6605 since dynamic states are also a confounding factor - anything which lazy-updates on read may also have unusual AABBs |
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
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
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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:
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 constantsBehavioural 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 theBlock
architecture, but should be avoided. As a world is not available duringRuntimeBlockStateRegistry
initialization, attempting to interact with a world duringrecalculateCollisionBoxes()
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 ifrecalculateCollisionBoxes()
ever generates out-of-bounds AABBs. However, this would be tricky to implement due to the headaches presented bygetModelPositionOffset()
, 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.