-
Notifications
You must be signed in to change notification settings - Fork 315
fix(renderer): track render target usage before cleaning them up, to fix prematurely unloading textures #2639
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
base: master
Are you sure you want to change the base?
Conversation
…ely unloading textures
|
Could you give an example to reproduce the issue? |
Desplandis
left a comment
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.
Hi and thanks for the PR!
I think the fix introduces overly complex global state handling and leaks implementation detail of TiledGeometryLayer, TileMesh and LayeredMaterial to MainLoop. I think the real issue is that the rtCache was made a global cache rather than a cache local to a given TiledGeometryLayer.
If the cache was scoped to the layer, you could use the same mechanism to dispose all render targets in TiledGeometyLayer#postUpdate (called after all updates).
Here a suggested approach that could fix your issue:
class TiledGeometryLayer {
rtCache: LRUCache<string, THREE.WebGLArrayRenderTarget>;
convert() {
const tilemesh = convertToTile(...); // same as current code
convertToTile.material.cache = rtCache;
return tilemesh;
}
}It would suppose that we also have a property cache in LayeredMaterial which would be set by the layer (whenever you're initializing a new TileMesh). As such all material within a layer would share the same cache and the implementation would not leak into the MainLoop.
Let me know if you'd like further clarification!
The issue can be observed in basic examples like |
| this.colorLayersOrder = getColorLayersIdOrderedBySequence(context.colorLayers); | ||
|
|
||
| // Dispose render targets that are queued for disposal and not used by this view | ||
| if (this.usedRts.length) { // important: only clean up if last loop did rendering |
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.
Previously this cleanup was done after each render, and this clear was done before each render. For it to work with the new refactoring (moving it to preUpdate), I had to add this condition, that looks a bit hacky but is there a better solution?
|
@ftoromanoff This PR is ready for review again |
Desplandis
left a comment
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.
Thanks for you PR! However, I have a few remarks:
- I think you could simplify the code by building a dedicated cache class (= cache + disposal cache + used set)
- I don't think you need to guard against the first render
- You could directly marked render target as used, instead of the code of
trackCurrentRenderTargetUsage
| } | ||
|
|
||
| this.rtCache?.set(textureSetId, rt); | ||
| rt.texture.userData = { textureSetId }; |
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.
I suppose this is marking this render target as used for this frame. Why not adding it directly to the set of used render targets (i.e. usedRts)?
| let cachedRT = this.rtCache?.get(textureSetId); | ||
|
|
||
| if (!cachedRT) { | ||
| cachedRT = this.pendingRtDisposal?.get(textureSetId); | ||
| if (cachedRT) { | ||
| this.rtCache!.set(textureSetId, cachedRT); | ||
| this.pendingRtDisposal!.delete(textureSetId); | ||
| } | ||
| } |
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.
From what I infer from this code:
- it fetches a render target from the cache
- if not present, it checks whether it was marked as disposed and in such case cache it again
I think this code would benefit to be factored within a class (within a get: string -> RenderTarget method?). Moreover, if in cache it should be marked as used.
| this.rtCache?.set(textureSetId, rt); | ||
| rt.texture.userData = { textureSetId }; |
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.
Same here, this code would benefit to be factored within a class (within a set: string -> RenderTarget -> void method?).
| * @param type - Layer set identifier: 'c' for color, 'e' for elevation. | ||
| * @param renderer - The renderer used to render textures. | ||
| */ | ||
| private _updateLayersUniformsForType<Type extends 'c' | 'e'>( |
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.
Could you just add the corresponding cache parameter instead of moving the function to a new method, this breaks the diff of the code?
| this.rtCache!.set(textureSetId, cachedRT); | ||
| this.pendingRtDisposal!.delete(textureSetId); |
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.
A good case for factoring the two caches & marked targets, you will not have to use the ! operator!
| // Dispose render targets that are queued for disposal and not used by this view | ||
| if (this.usedRts.size) { // important: only clean up if last loop did rendering | ||
| for (const [id, renderTarget] of this.pendingRtDisposal) { | ||
| if (this.usedRts.has(id)) { continue; } | ||
| renderTarget.dispose(); | ||
| this.pendingRtDisposal.delete(id); | ||
| } | ||
|
|
||
| // initialize render target usage tracking for next render | ||
| this.usedRts.clear(); | ||
| } | ||
|
|
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.
A good case for refactoring into a class. A cleanup method maybe?
| this.colorLayersOrder = getColorLayersIdOrderedBySequence(context.colorLayers); | ||
|
|
||
| // Dispose render targets that are queued for disposal and not used by this view | ||
| if (this.usedRts.size) { // important: only clean up if last loop did rendering |
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.
If done correctly, we should not need this no?
I know that we don't have a method called after rendering in the layer (maybe we should add a postRender method?) but this should clean the render target of the previous render.
| /** | ||
| * @type {Map<string, THREE.WebGLArrayRenderTarget>} | ||
| */ | ||
| this.pendingRtDisposal = new Map(); | ||
| /** | ||
| * @type {Set<string>} | ||
| */ | ||
| this.usedRts = new Set(); | ||
|
|
||
| /** | ||
| * @type {LRUCache<string, THREE.WebGLArrayRenderTarget>} | ||
| */ | ||
| this.rtCache = new LRUCache({ | ||
| max: 200, | ||
| dispose: (rt, key) => { | ||
| this.pendingRtDisposal.set(key, rt); | ||
| }, | ||
| }); |
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.
I think you should have a dedicated class for all caches, I could think of a class like:
class RenderTargetCache {
private cache: LRUCache<string, THREE.WebGLArrayRenderTarget>;
private pendingDisposal = new Map<string, THREE.WebGLArrayRenderTarget>();
private used = new Set<string>();
constructor(maxSize = 200) {
this.rtCache = new LRUCache({
max: maxSize,
dispose: (rt, key) => this.pendingDisposal.set(key, rt),
});
}
get(id: string): THREE.WebGLArrayRenderTarget | undefined { ... }
set(id: string, rt: THREE.WebGLArrayRenderTarget): void { ... }
cleanup(): void { ... }
dispose(): void { ... }
}
Description
Track which array render targets are actually used when rendering each view.
Then, when they would normally be disposed of because the cache managing them reaches its limit, we keep them in another map to only dispose of them once they are not used for rendering in any view.
Motivation and Context
When rendering array textures for new tiles, some tiles' textures were sometimes prematurely unloaded, leaving holes in the terrain where those tiles should be.
Screenshots
Screencast.from.19-11-2025.09.34.36.webm