diff --git a/src/core/CoreNode.test.ts b/src/core/CoreNode.test.ts index 6ed1235..167fa5e 100644 --- a/src/core/CoreNode.test.ts +++ b/src/core/CoreNode.test.ts @@ -1712,4 +1712,169 @@ describe('set color()', () => { expect(node.isRenderable).toBe(true); }); }); + + describe('texture ownership cache', () => { + // Same shape as the placeholderColor texture fake: a real EventEmitter so + // loadTextureTask subscribes and we can drive freed/loaded by emitting. + function emittingTexture(state: string): ImageTexture & { + emit: (event: string, data?: unknown) => void; + } { + return Object.assign(new EventEmitter(), { + state, + preventCleanup: false, + retryCount: 0, + maxRetryCount: 1, + dimensions: { w: 100, h: 100 }, + setRenderableOwner: vi.fn(), + }) as unknown as ImageTexture & { + emit: (event: string, data?: unknown) => void; + }; + } + + function visibleNode(): CoreNode { + const parent = new CoreNode(stage, defaultProps()); + parent.globalTransform = Matrix3d.identity(); + parent.worldAlpha = 1; + + const node = new CoreNode(stage, defaultProps({ parent })); + node.alpha = 1; + node.x = 0; + node.y = 0; + node.w = 100; + node.h = 100; + return node; + } + + const flushMicrotasks = () => Promise.resolve(); + + it('repeated updates with unchanged state call setRenderableOwner once', () => { + const node = visibleNode(); + const texture = emittingTexture('loaded'); + node.texture = texture; + node.textureLoaded = true; + + // The texture setter registers ownership with the node's current + // isRenderable (false here), so the cache starts false. + expect(texture.setRenderableOwner).toHaveBeenCalledTimes(1); + expect(texture.setRenderableOwner).toHaveBeenLastCalledWith( + expect.anything(), + false, + ); + + node.update(0, clippingRect); + expect(texture.setRenderableOwner).toHaveBeenCalledTimes(2); + expect(texture.setRenderableOwner).toHaveBeenLastCalledWith( + expect.anything(), + true, + ); + + // Steady-state scroll: ownership unchanged, no further calls. + node.update(1, clippingRect); + node.update(2, clippingRect); + expect(texture.setRenderableOwner).toHaveBeenCalledTimes(2); + }); + + it('moving out of bounds releases ownership once, returning re-adds', () => { + const node = visibleNode(); + const texture = emittingTexture('loaded'); + node.texture = texture; + node.textureLoaded = true; + + node.update(0, clippingRect); + expect(texture.setRenderableOwner).toHaveBeenLastCalledWith( + expect.anything(), + true, + ); + const callsAfterFirstUpdate = ( + texture.setRenderableOwner as ReturnType + ).mock.calls.length; + + // Out of the 200x200 stage bounds entirely. + node.x = 1000; + node.update(1, clippingRect); + expect(texture.setRenderableOwner).toHaveBeenCalledTimes( + callsAfterFirstUpdate + 1, + ); + expect(texture.setRenderableOwner).toHaveBeenLastCalledWith( + expect.anything(), + false, + ); + + // Still out of bounds: no repeated release. + node.update(2, clippingRect); + expect(texture.setRenderableOwner).toHaveBeenCalledTimes( + callsAfterFirstUpdate + 1, + ); + + // Back in view: re-registered exactly once. + node.x = 0; + node.update(3, clippingRect); + expect(texture.setRenderableOwner).toHaveBeenCalledTimes( + callsAfterFirstUpdate + 2, + ); + expect(texture.setRenderableOwner).toHaveBeenLastCalledWith( + expect.anything(), + true, + ); + }); + + it('swapping textures releases the old owner and registers the new one', () => { + const node = visibleNode(); + const textureA = emittingTexture('loaded'); + node.texture = textureA; + node.textureLoaded = true; + node.update(0, clippingRect); + expect(textureA.setRenderableOwner).toHaveBeenLastCalledWith( + expect.anything(), + true, + ); + + const textureB = emittingTexture('loaded'); + node.texture = textureB; + + // A is released via unloadTexture. + expect(textureA.setRenderableOwner).toHaveBeenLastCalledWith( + expect.anything(), + false, + ); + // B is registered with the node's current renderable state (true), + // proving the cache reset on swap — a stale cache would skip this. + expect(textureB.setRenderableOwner).toHaveBeenCalledWith( + expect.anything(), + true, + ); + }); + + it('freed texture is re-registered on the next update (reload trigger)', async () => { + const node = visibleNode(); + const texture = emittingTexture('initial'); + node.texture = texture; + node.update(0, clippingRect); + + await flushMicrotasks(); + (texture as { state: string }).state = 'loaded'; + texture.emit('loaded', { w: 100, h: 100 }); + node.update(1, clippingRect); + expect(texture.setRenderableOwner).toHaveBeenLastCalledWith( + expect.anything(), + true, + ); + + // Memory manager frees the texture: the node must drop ownership... + (texture as { state: string }).state = 'freed'; + texture.emit('freed'); + expect(texture.setRenderableOwner).toHaveBeenLastCalledWith( + expect.anything(), + false, + ); + + // ...and the next update pass re-adds it, which is what triggers + // Texture.load() for the reload. A stale cache would skip this call. + node.update(2, clippingRect); + expect(texture.setRenderableOwner).toHaveBeenLastCalledWith( + expect.anything(), + true, + ); + }); + }); }); diff --git a/src/core/CoreNode.ts b/src/core/CoreNode.ts index 946a5d1..4d5e2f5 100644 --- a/src/core/CoreNode.ts +++ b/src/core/CoreNode.ts @@ -829,6 +829,14 @@ export class CoreNode extends EventEmitter { private hasColorProps = false; public textureLoaded = false; + /** + * Last ownership value sent to the current texture via + * {@link updateTextureOwnership}. Per (node, texture) pair — must reset to + * `false` whenever the texture is swapped or released, or a stale `true` + * would skip the re-registration that triggers `Texture.load()`. + */ + private textureOwnership = false; + /** * True while this node should render its `placeholderColor` instead of its * texture: `placeholderColor` is non-zero, a texture is set, and that @@ -1085,6 +1093,7 @@ export class CoreNode extends EventEmitter { texture.off('failed', this.onTextureFailed); texture.off('freed', this.onTextureFreed); texture.setRenderableOwner(this._id, false); + this.textureOwnership = false; } protected onTextureLoaded: TextureLoadedEventHandler = (_, dimensions) => { @@ -1918,6 +1927,10 @@ export class CoreNode extends EventEmitter { * Changes the renderable state of the node. */ updateTextureOwnership(isRenderable: boolean) { + if (this.textureOwnership === isRenderable) { + return; + } + this.textureOwnership = isRenderable; this.texture?.setRenderableOwner(this._id, isRenderable); } @@ -3100,6 +3113,7 @@ export class CoreNode extends EventEmitter { this.autosizer.setMode(AutosizeMode.Texture); // Set to texture size mode } value.setRenderableOwner(this._id, this.isRenderable); + this.textureOwnership = this.isRenderable; this.loadTexture(); } diff --git a/src/core/CoreTextNode.ts b/src/core/CoreTextNode.ts index ba3e4c0..4d33db9 100644 --- a/src/core/CoreTextNode.ts +++ b/src/core/CoreTextNode.ts @@ -266,8 +266,9 @@ export class CoreTextNode extends CoreNode implements CoreTextNodeProps { this.setRenderable(false); if (this.renderState > CoreNodeRenderState.OutOfBounds) { - // We do want the texture to load immediately - this.texture.setRenderableOwner(this._id, true); + // We do want the texture to load immediately. Routed through + // updateTextureOwnership so the ownership cache stays in sync. + this.updateTextureOwnership(true); } } } diff --git a/src/core/TextureMemoryManager.test.ts b/src/core/TextureMemoryManager.test.ts index f5692de..72af373 100644 --- a/src/core/TextureMemoryManager.test.ts +++ b/src/core/TextureMemoryManager.test.ts @@ -175,7 +175,7 @@ function freedCachedTexture(cacheKey: string): Texture & { state: 'freed', type: TextureType.image, preventCleanup: false, - renderableOwners: [], + renderableOwners: new Set(), cacheKey, free: vi.fn(), destroy: vi.fn(), @@ -206,7 +206,7 @@ describe('TextureMemoryManager — orphaned freed texture eviction', () => { it('keeps a freed texture that still has renderable owners', () => { const { mgr, keyCache } = makeManager(); const texture = freedCachedTexture('img:poster.png'); - (texture.renderableOwners as unknown[]).push(1); + texture.renderableOwners.add(1); keyCache.set('img:poster.png', texture); mgr.cleanup(); diff --git a/src/core/TextureMemoryManager.ts b/src/core/TextureMemoryManager.ts index 7f95f89..078f3df 100644 --- a/src/core/TextureMemoryManager.ts +++ b/src/core/TextureMemoryManager.ts @@ -349,7 +349,7 @@ export class TextureMemoryManager { if ( evictable === true && texture.preventCleanup === false && - texture.renderableOwners.length === 0 && + texture.renderableOwners.size === 0 && texture.hasListeners() === false ) { this.destroyTexture(texture); diff --git a/src/core/textures/SubTexture.ts b/src/core/textures/SubTexture.ts index ec0ffbf..a5b546a 100644 --- a/src/core/textures/SubTexture.ts +++ b/src/core/textures/SubTexture.ts @@ -79,7 +79,7 @@ export class SubTexture extends Texture { // Resolve parent texture from cache or fallback to provided texture this.parentTexture = txManager.resolveParentTexture(this.props.texture); - if (this.renderableOwners.length > 0) { + if (this.renderableOwners.size > 0) { this.parentTexture.setRenderableOwner(this.subtextureId, true); } diff --git a/src/core/textures/Texture.test.ts b/src/core/textures/Texture.test.ts new file mode 100644 index 0000000..4200ccc --- /dev/null +++ b/src/core/textures/Texture.test.ts @@ -0,0 +1,124 @@ +import { describe, it, expect, vi } from 'vitest'; +import { Texture, type TextureData } from './Texture.js'; +import type { CoreTextureManager } from '../CoreTextureManager.js'; + +class TestTexture extends Texture { + override async getTextureSource(): Promise { + return { data: null }; + } +} + +function makeTexture(): { + texture: TestTexture; + loadTexture: ReturnType; + onChangeIsRenderable: ReturnType; +} { + const loadTexture = vi.fn(); + const txManager = { + maxRetryCount: 3, + loadTexture, + } as unknown as CoreTextureManager; + const texture = new TestTexture(txManager); + const onChangeIsRenderable = vi.fn(); + texture.onChangeIsRenderable = onChangeIsRenderable; + return { texture, loadTexture, onChangeIsRenderable }; +} + +describe('Texture.setRenderableOwner', () => { + it('adding an owner fires the 0→1 transition exactly once and loads', () => { + const { texture, loadTexture, onChangeIsRenderable } = makeTexture(); + + texture.setRenderableOwner(1, true); + + expect(texture.renderableOwners.size).toBe(1); + expect(texture.renderable).toBe(true); + expect(onChangeIsRenderable).toHaveBeenCalledTimes(1); + expect(onChangeIsRenderable).toHaveBeenCalledWith(true); + expect(loadTexture).toHaveBeenCalledTimes(1); + }); + + it('double-add of the same owner is a no-op (size stays 1, one event)', () => { + const { texture, loadTexture, onChangeIsRenderable } = makeTexture(); + + texture.setRenderableOwner(1, true); + texture.setRenderableOwner(1, true); + + expect(texture.renderableOwners.size).toBe(1); + expect(onChangeIsRenderable).toHaveBeenCalledTimes(1); + expect(loadTexture).toHaveBeenCalledTimes(1); + }); + + it('a second distinct owner does not re-fire the transition', () => { + const { texture, loadTexture, onChangeIsRenderable } = makeTexture(); + + texture.setRenderableOwner(1, true); + texture.setRenderableOwner(2, true); + + expect(texture.renderableOwners.size).toBe(2); + expect(onChangeIsRenderable).toHaveBeenCalledTimes(1); + expect(loadTexture).toHaveBeenCalledTimes(1); + }); + + it('removing a non-member owner does not fire a transition', () => { + const { texture, onChangeIsRenderable } = makeTexture(); + + texture.setRenderableOwner(1, false); + + expect(texture.renderableOwners.size).toBe(0); + expect(texture.renderable).toBe(false); + expect(onChangeIsRenderable).not.toHaveBeenCalled(); + }); + + it('1→0 fires onChangeIsRenderable(false) exactly once', () => { + const { texture, onChangeIsRenderable } = makeTexture(); + + texture.setRenderableOwner(1, true); + texture.setRenderableOwner(1, false); + texture.setRenderableOwner(1, false); + + expect(texture.renderableOwners.size).toBe(0); + expect(texture.renderable).toBe(false); + expect(onChangeIsRenderable).toHaveBeenCalledTimes(2); + expect(onChangeIsRenderable).toHaveBeenLastCalledWith(false); + }); + + it('re-adding an owner after 1→0 triggers load again (freed→reload cycle)', () => { + const { texture, loadTexture } = makeTexture(); + + texture.setRenderableOwner(1, true); + texture.setRenderableOwner(1, false); + texture.setRenderableOwner(1, true); + + expect(texture.renderable).toBe(true); + expect(loadTexture).toHaveBeenCalledTimes(2); + }); + + it('string and number owners coexist (node ids vs subtexture/font keys)', () => { + const { texture } = makeTexture(); + + texture.setRenderableOwner(7, true); + texture.setRenderableOwner('subtexture-7', true); + + expect(texture.renderableOwners.size).toBe(2); + + texture.setRenderableOwner(7, false); + expect(texture.renderableOwners.size).toBe(1); + expect(texture.renderable).toBe(true); + + texture.setRenderableOwner('subtexture-7', false); + expect(texture.renderableOwners.size).toBe(0); + expect(texture.renderable).toBe(false); + }); + + it('canBeCleanedUp respects remaining owners', () => { + const { texture } = makeTexture(); + // Skip the startup grace period so owners are the deciding factor. + texture.isWithinStartupGracePeriod = () => false; + + texture.setRenderableOwner(1, true); + expect(texture.canBeCleanedUp()).toBe(false); + + texture.setRenderableOwner(1, false); + expect(texture.canBeCleanedUp()).toBe(true); + }); +}); diff --git a/src/core/textures/Texture.ts b/src/core/textures/Texture.ts index dcfd5f7..3f85b0f 100644 --- a/src/core/textures/Texture.ts +++ b/src/core/textures/Texture.ts @@ -141,7 +141,7 @@ export abstract class Texture extends EventEmitter { // aggregate state public state: TextureState = 'initial'; - readonly renderableOwners: any[] = []; + readonly renderableOwners: Set = new Set(); readonly renderable: boolean = false; @@ -251,7 +251,7 @@ export abstract class Texture extends EventEmitter { } // Don't cleanup if there are still renderable owners - if (this.renderableOwners.length > 0) { + if (this.renderableOwners.size > 0) { return false; } @@ -274,28 +274,25 @@ export abstract class Texture extends EventEmitter { * @param renderable */ setRenderableOwner(owner: string | number, renderable: boolean): void { - const oldSize = this.renderableOwners.length; - const hasOwnerIndex = this.renderableOwners.indexOf(owner); + const owners = this.renderableOwners; if (renderable === true) { - if (hasOwnerIndex === -1) { - // Add the owner to the set - this.renderableOwners.push(owner); + if (owners.has(owner) === true) { + return; } - const newSize = this.renderableOwners.length; - if (oldSize !== newSize && newSize === 1) { + owners.add(owner); + if (owners.size === 1) { (this.renderable as boolean) = true; this.onChangeIsRenderable?.(true); this.load(); } } else { - if (hasOwnerIndex !== -1) { - this.renderableOwners.splice(hasOwnerIndex, 1); + if (owners.delete(owner) === false) { + return; } - const newSize = this.renderableOwners.length; - if (oldSize !== newSize && newSize === 0) { + if (owners.size === 0) { (this.renderable as boolean) = false; this.onChangeIsRenderable?.(false); diff --git a/src/main-api/Inspector.ts b/src/main-api/Inspector.ts index 652915a..b8aae5a 100644 --- a/src/main-api/Inspector.ts +++ b/src/main-api/Inspector.ts @@ -1104,7 +1104,7 @@ export class Inspector { // Update renderable owners count div.setAttribute( 'data-texture-owners', - String(texture.renderableOwners.length), + String(texture.renderableOwners.size), ); // Update retry count