Memory leak in the rendering pipline

[flo]

What you were trying to do

Normally playing the game with water reflections set to "sky only" (default setting I assume).

What actually happened

The game needs constantly more memory as the field chunksOpaqueReflection field in RenderQueuesHelper grows constantly in size.

The method renderChunks of WorldRendererImpl is the only method I could find that removed entities from that list. The method renderChunks gets however only called byWorldReflectionNode if the reflectiveWater config option is set.

The list gets filled multiple times with the same chunks again and again.
The expected max size specified by the MAX_LOADABLE_CHUNKS constant of RenderableWorldImpl gets thus easily topped. (Btw. I think it would have been better to not have this constant: 1. It is less code. 2. It can not lead another developer to wrongly assume that the queue has a size limit(was that the intention?). 3. The CPU overhead for growing the list is minimal (in the worst case twice as many assignments to array positions need to be made), while on the other size the list is not larger as it needs to be on systems that have less memory.)

Maybe the queue could be cleared before it gets filled again?

When this issue gets fixed, it should be checked if the other piplines have potentially the same issue.

How to reproduce

1. Start the game from the IDE with"water reflections" set to "sky only"
2. Set a breakpoint and have a look at the size of the chunksOpaqueReflection field in RenderQueuesHelper

[flo]

@emanuele3d @tdgunes it would be great if you could have a look as it is code you wrote.

[emanuele3d]

Thank you @flo for reporting this problem.

The code you are referring to is older than our GSOC-based interventions over the past few months and even older than the refactoring of the rendering engine I have been working on over the past 18-24 months.

The particular chunk list you are referring to and others like it were introduced before my time, either by begla or immortius I imagine.

tdgunes did refactor the renderChunks method somewhat however and it is entirely possible that we introduced a problem without realizing.

In this context it'd be good to verify if this is a recently introduced problem or have been silently there for much longer. And is the issue noticeable only via debugging or does it result into filling the whole memory over a relatively short amount of time?

In any case, we'll look into it. Can we add some appropriate labels to this issue and assign it to me and tdgunes?

[emanuele3d]

@Cervator: I'm not sure this is about 3D-Wizardry: more about chunks management in some kind of collection. But I guess those collections are compiled and maintained for the benefit of rendering so... 😄

[flo]

The game needs more and more memory, I don't know fixing this issue will completely fix it or if it is just contributing to it. I found the issue through a memory snapshot however, so it's not something minor. (view distance moderate = potentially 13*13*13 chunks = about 2000 = chunks, 50 frames per second would give 100000 entries per second. Each entry say 4 bytes would give 400 kbytes per second = 24 mbytes per minute = about 1.4 GB/hour, of coures maybe not all chunks may have reflection but still).

[emanuele3d]

Ok, we'll take a look. Thanks @flo!

[Cervator]

3D wizardly related architecture then? :D

The labels aren't an exact science, better too many than too few :-)

(and thanks for the detailed issue, @flo)

[tdgunes]

Thanks @flo for reporting this issue. As @emanuele3d points out, I think it shouldn't be related with the what we are doing recently in the rendering engine. I briefly check to see what causes it. I sort of validate it with YourKit, but I am very novice in this matter. Let me first post the related parts:

WorldReflectionNode.java

public void process() {
    ...
    if (renderingConfig.isReflectiveWater()) {
        ...
        // by `chunks.poll()`, it's removed from the queue in this method.
        worldRenderer.renderChunks(renderQueues.chunksOpaqueReflection, ...
        ...
    }
}

RenderableWorldImpl.java

/**
 * Updates the currently visible chunks (in sight of the player).
 */
@Override
public int queueVisibleChunks(boolean isFirstRenderingStageForCurrentFrame) {
       ...
       if (isChunkVisibleReflection(chunk)) {
           renderQueues.chunksOpaqueReflection.add(chunk); // Only addition to the queue
       }
       ...
}

I wonder why in RenderableWorldImpl.java addition to the queue is not dependent on the reflective water config. What do you think about that? All other queues are guaranteed to be removed because there are not dependent on any config. But why is this different in the first place?

[Cervator]

This is again something I'm waaay not qualified to talk about, but the one thing that came to mind is an old memory of @begla talking about how reflection is expensive as each layer of reflection ends up needing a whole other rendering pass. Could this be a leftover of some older approach where at one point the reflection piece was "left in" on a first pass just so it could do its thing on a second or later pass? But then we refactored/broke something later and didn't correctly handle or clean up how reflection was set up.

I see a isFirstRenderingStageForCurrentFrame boolean right there, was what made me think of it. And we're bound to have places still where we have a weird mix of old and new code, or even old, really old, and new code

Chance are I'm totally off and should go back to logistics, but I wanted to try throwing an idea in there :D

[emanuele3d]

I just took an in-depth look at the relevant code. In my opinion the quickest solution is as simple as having

if (sceneVisibleInReflections && isChunkVisibleReflection(chunk)) {
    renderQueues.chunksOpaqueReflection.add(chunk);
}

Longer term I'd like the whole RenderableWorldImpl class to become a number of "caching nodes" in the dag, processed before the rendering nodes, although right now I can't quite see how this can be achieved - it's hard to think how RenderableWorldImpl can be split into nodes.

Regarding the question "why is this queue different in the first place": it looks like a straightforward bug that was either there when this code was initially written or that was introduced somewhere along the line. Perhaps it was never detected because Terasology crashes or is shut down before memory consumption becomes an issue. Also, the chunkOpaqueShadowqueue is an example of a queue dependent on the rendering config.

I don't have the time right now to do the fix as I want to go through the steps @flo suggested to make sure the fix does the job. @tdgunes, if you have an itch to deal with this go ahead and do it - just let me know if you do. Otherwise I'll take care of it sometimes this week I hope.