RobustToolbox icon indicating copy to clipboard operation
RobustToolbox copied to clipboard

Try improve recursive component lookups

Open ElectroJr opened this issue 3 months ago • 0 comments

Marked as a draft because I'm pretty sure its currently broken, and I need to re-read this rambling mess of a PR description.

About

This PR tries to improve recursive component lookup systems, so that they could actually be used on the server. I'm not at all sure that this is the best solution, but I don't think it'd be that bad for performance and probably even makes the client-side lookups faster, though I haven't benchmarked it.

The issue

Component lookup systems exist to allow for area lookups for entities with some specific component, and should generally be much faster than using a generic entity lookup and then filtering based on components. However, one of the main restrictions is that they are largely limited to only being used by entities that are always parented directly to either the grid or map. This is because then you can use normal MoveEvents to update the entity positions.

If you instead want to support entities that are further up the transform hierarchy, you need a "recursive" component lookup system. These are currently quite expensive, as the presence of even a single such system requires RecursiveMoveSystem to start recursively iterating over all children whenever any MoveEvent is raised, and then trying to fetch a component for each entity & tree. This is obviously quite bad, especially when you have players moving around as they are basically just a large Matryoshka doll of storage containers & organs.

So as a result, we currently don't want to enable any recursive lookups on the server, though a few of them are used on the client (sprites & lights).

Potential Fix

This PR tries to fix this by changing how RecursiveMoveSystem notifies the lookup systems that positions need to be updated. Instead of recursively iterating through children, it now tracks any entities that need to handle move events in a transform hierarchy tree (EntityTree, unrelated to the area lookup "trees"). As before, the functionality is only enabled if there is at least one recursive lookup system. While enabled it adds some overhead to to EntParentChangedMessage.

If any entity that is in the tree moves, RecursiveMoveSystem will give any recursive lookup systems a ReadonlySpan<EntityUid> that consists only of entities that actually need to get their position updated. So for example, if the server did get given a recursive light lookup tree and a player with a pda & flashlight moves, the light tree system should just be given a span with 2 entities and the move event shouldn't be noticeably more expensive than it is without light lookups enabled.

EntityTree

The hierarchy tree is a subset of the full transform hierarchy that only has entities that either need to receive events or who have any children that need to be on the tree. It is meant to allow for fast enumeration of relevant entities at the cost of more complex insertion/removal. When you want to enumerate over children in a node, the tree basically flattens each that node to a cached List<EntityUid> that only gets updated when needed.

This should support things like having a player moving off & onto grids without actually needing to update any cached lists, and when something like a backpack is picked up or dropped, it should only ever have to update the player's own cached list.

Metadata flags

This PR adds two new metadata flags that are used to tag any entity that has components that are tracked by lookup system. The non-recursive lookup updating has also moved been moved from a MoveEvent event subscription to using the same C# move event handler that the recursive trees use along with a metadata flag check.

Caveats

The span that RecursiveMoveSystem gives to recursive lookup systems contains all entity with any component that is tracked by a recursive tree, not just the entities relevant to that tree. So in the previous example where two lights were passed to the server-side light system, on the client that would instead also include any entities with sprites, which the light tree would still have to filter through. The end result should still be quite a bit faster than it was before, but having something commonplace like sprites in a recursive tree will still slow things down a bit and should be avoided on the server.

This could be avoided by giving each recursive comp lookup tree its own own entity transform tree, but would add more overhead to handling parent-change events.

Benchmarks

Testing using a content benchmark, where the client is moving around a player containing ~70 entities. Its basically the same as the engine's RecursiveMoveBenchmark, but running on the client and with a more realistic player entity.

Master

Method Mean Error StdDev Ratio
MoveSameParent 6.992 us 0.0084 us 0.0075 us 1.00
MoveNewParent 13.430 us 0.0124 us 0.0116 us 1.92

This PR

Method Mean Error StdDev Ratio
MoveSameParent 3.469 us 0.0039 us 0.0036 us 1.00
MoveNewParent 9.390 us 0.0300 us 0.0266 us 2.71

This PR + net10 RC 1 because why not

Method Mean Error StdDev Ratio
MoveSameParent 2.713 us 0.0120 us 0.0112 us 1.00
MoveNewParent 8.461 us 0.0386 us 0.0343 us 3.12

I love free performance

TODO

  • fix TODO comments
  • breaking changes (comp subscription conflicts, better handlle sprite & light init)

ElectroJr avatar Sep 19 '25 16:09 ElectroJr