cl_visedicts: are duplicate entries OK/expected?

[Johnny Law]

czg07 is the only place where this issue has been observed so far, but reQuiem isn't widely used yet. It's a fundamental enough problem that I wouldn't be surprised to see it manifest in other situations.

Here's an extremely-cut-down version of the issue comments:
=====
skychain is a linked list (through texturechain pointers) of surfaces. Surfaces are appended to skychain during a loop processing cl_visedicts. If there are duplicates in cl_visedicts then loops can be formed in the skychain. If there are loops in the skychain then R_DrawSky will never terminate. (This is all contingent on certain cvars used to choose the sky style, but this is the default behavior.)
=====

It's pretty straightforward to ensure that we don't form loops in skychain even if there are dups in cl_visedicts. The trickier part is doing it efficiently, but there are some possibilities there. I went down that path a little bit last night, but I'm now reconsidering whether I want to just try to fix the underlying issue of dups in cl_visedicts.

The basic problem with the cl_visedicts-modifying code in R_StoreEfrags is that it assumes that it can use r_framecount to determine "have I already added this thing into cl_visedicts since the last cl_visedicts reset". But that's not a good assumption, since r_framecount can be incremented multiple times between cl_visedicts resets (CL_RelinkEntities).

Similarly to the skychain code, one could make R_StoreEfrags a little smarter so that it avoids putting dups in cl_visedicts. The brute-force way would be to search the cl_visedicts array on each add... don't want to do that. One alternative would be to use the visframe field (of the entity) differently: rather than as a counter compared against r_framecount, instead just as a flag that "this entity is currently in cl_visedicts", and make sure to clear out the flags of current cl_visedicts elements when resetting cl_numvisedicts in CL_RelinkEntities. An entity's visframe field isn't used anywhere else in the code I think; if that turns out to really be the case then I could repurpose it freely.

That still feels like a band-aid in a couple of ways:
- There are other spots besides R_StoreEfrags that add stuff into cl_visedicts. I haven't yet investigated which of those other spots might occur multiple times between CL_RelinkEntities invocations.
- It's nice to be able to ignore attempts to put dups in cl_visedicts, but it's a little eyebrow-raising that the overall code flow is making those attempts.

[Spike]

just revert cl_numvisedicts to what it was at the start of the frame, so that ents added via efrags can't build up?

[Baker]

[Johnny Law]

Yeah, it could be better to explicitly say "when it is OK to do this" rather than trying to wall off all the cases of "when is it not OK to do this". I'll try that.

[Johnny Law]

Playtesting a fix now. I didn't quite go as global with the CL_LinkEntity changes as described above, because it looks like the problem that introduces duplicates really is localized to R_StoreEfrags.

https://github.com/SpiritQuaddicted/reQ ... 1620b75c2c

[Baker]

Looks like your issue is solved.

1 week from now, some other feature or bug will be your obsession and all memories of this particular bug will fade.

[Spike]

well, that or it'll just add to the nightmares. to each their own.