-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Add Active Object Spatial Cache #14643
base: master
Are you sure you want to change the base?
Add Active Object Spatial Cache #14643
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gave it a quick look
|
||
void SpatialMap::getRelevantObjectIds(const aabb3f &box, const std::function<void(u16 id)> &callback) | ||
{ | ||
if(!m_cached.empty()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Early return would help here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The whole function happens within that if statement, so if it fails: that's early return. Difference in style
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I'm aware. I would consider an earlier return to be better style-wise - as in, if empty, we're done, return - because it avoids the indentation level. This is getting pretty deeply nested.
…e other server operations use the better lookup method (like keeping client SAOs up to date)
…s, or very small entity counts.
d5316f2
to
9659ff0
Compare
9659ff0
to
6f0c224
Compare
::ActiveObjectMgr<ServerActiveObject>::clear(); | ||
m_spatial_map.removeAll(); | ||
} | ||
|
||
void ActiveObjectMgr::clearIf(const std::function<bool(ServerActiveObject *, u16)> &cb) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method directly removes entries from m_active_objects
, side-stepping removeObject
. I believe you need to replace m_active_objects.remove(it.first);
with removeObject(it.first);
to ensure that you don't end up with dead entries in your spatial index data structure. At least that's what I did.
This PR adds a simple cache to server active objects (SAOs) based on position. This PR was made to solve the problem outlined in #14613, hoping to improve by orders of magnitude our ability to handle more entities in Minetest, and to make more performant ways to get objects in a given area/radius.
This directly addresses section 2.4 of the current roadmap for minetest. It specifically calls out the old version of the issue this task tries to address.
The implementation is best described as a single-layer of buckets implemented using unordered_multimap, which is a fancy term for "It's sorts entities into buckets the same size as mapblocks (16x16x16)". When you ask for entities in a given area/radius, this map allows you to filter out the vast majority of irrelevant entities.
Performance
5000 Entities - all getting objects inside radius
Simple Comparison
Relevant lua used for testing:
Hot Spot benchmarks
Master:
Spatial Map Only:
Spatial Map + Performant std::map
Builtin Benchmarking [Pending]
To do
This PR is still pending several features/integrations, but is working as is and Ready for Review. (It could be merged as is, but we'd be missing some further performance gains by using the map in places we currently linear iterate instead, as well as optimizations in the algorithm)
How to test
benchmarking:
Stability/Correctness
I have validated that the benchmarks give the same number of entities when using master and this branch, but have not done enough full runtime on a server testing yet.