crest icon indicating copy to clipboard operation
crest copied to clipboard

Clarification regarding SegmentRegistrar

Open CodeHatchling opened this issue 2 years ago • 2 comments

I've been looking over the code - I've pulled a more recent version (4.15.2) into our project and had to merge code changes. The changes I implemented were:

  • Switching from Vector3 to float3 - this is so the data provided by Crest can more easily be passed to a Burst job.
  • No query count limits - when the limits are exceeded, the buffers are expanded (similar to how List<T> works).

However, I noticed something in the code that wasn't clear, and I just wanted to make sure that the query count limit removal was implemented correctly.

I noticed that SegmentRegistrar._numQueries increments by the size of the segment segment.Value.y - segment.Value.x + 1 when a segment is added. However, when a segment is removed, SegmentRegistrar._numQueries is left unchanged. (QueryBase.cs, line 150, 279) Am I wrong in assuming that when a segment is removed, the _numQueries should decrease similarly?

As SegmentRegistrar._numQueries is what used to determine whether or not the queries should be performed at all (now it resizes the buffers instead of giving errors) I needed to be sure that SegmentRegistrar._numQueries wasn't just growing to infinity as new requests are made.

I assume that the values in SegmentRegistrarRingBuffer.Current are temporary and the SegmentRegistrar gets recycled on the next frame. To be honest, I am not sure I understand the thought process behind these rotating segment registrars. But if I'm not mistaken, the segment registrars themselves contain start/end indices for all of the queries made since the last frame/update, and the ID for each query so the correct data reaches the correct query call point.

(As a side note, I suppose this issue could be taken as a feature request to allow dynamically expanding query buffers. Our game is a physics sandbox, and as such it is undesirable to limit the number of objects a player might wish to place in order to avoid a fixed limit.)

Thanks!

-Steve

CodeHatchling avatar Jun 02 '22 03:06 CodeHatchling

Hello,

Thanks for the detailed notes and suggestions.

Each frame some number of clients each post some number of queries. The results will come back from the GPU some frames later, or not at all. The ring buffer captures the state each frame so that if/when query results return in the future all the data is captured to make sense of them. It's not the simplest system in the world and I think i have an idea to simplify it in the future but it's what is there currently.

_numQueries is set to 0 when a new frame is set up. RemoveRegistrations is not necessary to be called. Queries clean themselves up if a client stops calling the query API. It was added because it was easy to add it but in retrospect it probably causes more confusion than it provides benefit. The future simplification i mentioned will not needed this API and it will be removed, if we get around to it.

On query count limits - these were put in intentionally as too many queries will eventually slow things down, even if this is a big number. I don't think we'd be inclined to automatically grow to any size as they could create performance issues for some users.

huwb avatar Jun 02 '22 20:06 huwb

I suppose it could be a toggle, or if you set the limit to 0 or negative?

Alternatively, if there are more queued queries than what the limit allows, the data could be updated incrementally rather than all at once. In other words, if there are 512 queries but the limit is 256, the first 256 would be updated on frame N, then the second 256 would be updated on frame N+1. We already expect some delay between the query and the data being refreshed as it is - the data that is delayed by some number of frames could just use the latest available data as it currently does. It could even prioritize data based on visibility, the complexity of the wave simulation at the query center, the magnitude of expected change since the last update, etc.

I dunno, to me it seems like there are many better alternatives to having the universe implode when you go over the limit - which is usually what happens.

CodeHatchling avatar Jun 02 '22 22:06 CodeHatchling