From c01a534c2b964f3228de2fdd13f95d07c9abc4f6 Mon Sep 17 00:00:00 2001 From: wouterlucas Date: Thu, 9 Jan 2025 16:52:47 +0100 Subject: [PATCH 1/3] refactor: Simplify renderability checks --- src/core/CoreNode.test.ts | 188 ++++++++++++++---- src/core/CoreNode.ts | 168 +++++++++------- src/core/CoreTextNode.ts | 13 +- src/core/Stage.ts | 6 +- .../SdfTrFontFace/SdfTrFontFace.ts | 2 +- src/core/textures/Texture.ts | 8 +- 6 files changed, 261 insertions(+), 124 deletions(-) diff --git a/src/core/CoreNode.test.ts b/src/core/CoreNode.test.ts index 1d53881a..ce590a86 100644 --- a/src/core/CoreNode.test.ts +++ b/src/core/CoreNode.test.ts @@ -17,55 +17,73 @@ * limitations under the License. */ -import { describe, expect, it } from 'vitest'; +import { describe, expect, it, vi } from 'vitest'; import { CoreNode, type CoreNodeProps, UpdateType } from './CoreNode.js'; import { Stage } from './Stage.js'; import { mock } from 'vitest-mock-extended'; import { type TextureOptions } from './CoreTextureManager.js'; import { type BaseShaderController } from '../main-api/ShaderController'; +import { createBound } from './lib/utils.js'; +import { ImageTexture } from './textures/ImageTexture.js'; -describe('set color()', () => { - const defaultProps: CoreNodeProps = { - alpha: 0, - autosize: false, - clipping: false, - color: 0, - colorBl: 0, - colorBottom: 0, - colorBr: 0, - colorLeft: 0, - colorRight: 0, - colorTl: 0, - colorTop: 0, - colorTr: 0, - height: 0, - mount: 0, - mountX: 0, - mountY: 0, - parent: null, - pivot: 0, - pivotX: 0, - pivotY: 0, - rotation: 0, - rtt: false, - scale: 0, - scaleX: 0, - scaleY: 0, - shader: mock(), - src: '', - texture: null, - textureOptions: {} as TextureOptions, - width: 0, - x: 0, - y: 0, - zIndex: 0, - zIndexLocked: 0, - preventCleanup: false, - strictBounds: false, - }; +const defaultProps: CoreNodeProps = { + alpha: 0, + autosize: false, + clipping: false, + color: 0, + colorBl: 0, + colorBottom: 0, + colorBr: 0, + colorLeft: 0, + colorRight: 0, + colorTl: 0, + colorTop: 0, + colorTr: 0, + height: 0, + mount: 0, + mountX: 0, + mountY: 0, + parent: null, + pivot: 0, + pivotX: 0, + pivotY: 0, + rotation: 0, + rtt: false, + scale: 0, + scaleX: 0, + scaleY: 0, + shader: mock(), + src: '', + texture: null, + textureOptions: {} as TextureOptions, + width: 0, + x: 0, + y: 0, + zIndex: 0, + zIndexLocked: 0, + preventCleanup: false, + strictBounds: false, +}; + +const clippingRect = { + x: 0, + y: 0, + width: 200, + height: 200, + valid: false, +}; + +const stage = mock({ + strictBound: createBound(0, 0, 200, 200), + preloadBound: createBound(0, 0, 200, 200), + defaultTexture: { + state: 'loaded', + }, +}); +describe('set color()', () => { it('should set all color subcomponents.', () => { - const node = new CoreNode(mock(), defaultProps); + const node = new CoreNode(stage, defaultProps); node.colorBl = 0x99aabbff; node.colorBr = 0xaabbccff; node.colorTl = 0xbbcceeff; @@ -85,7 +103,7 @@ describe('set color()', () => { }); it('should set update type.', () => { - const node = new CoreNode(mock(), defaultProps); + const node = new CoreNode(stage, defaultProps); node.updateType = 0; node.color = 0xffffffff; @@ -93,3 +111,89 @@ describe('set color()', () => { expect(node.updateType).toBe(UpdateType.PremultipliedColors); }); }); + +describe('isRenderable checks', () => { + it('should return false if node is not renderable', () => { + const node = new CoreNode(stage, defaultProps); + expect(node.isRenderable).toBe(false); + }); + + it('visible node that is a color texture', () => { + const node = new CoreNode(stage, defaultProps); + node.alpha = 1; + node.x = 0; + node.y = 0; + node.width = 100; + node.height = 100; + node.color = 0xffffffff; + + node.update(0, clippingRect); + expect(node.isRenderable).toBe(true); + }); + + it('visible node that is a texture', () => { + const node = new CoreNode(stage, defaultProps); + node.alpha = 1; + node.x = 0; + node.y = 0; + node.width = 100; + node.height = 100; + node.texture = mock({ + state: 'initial', + }); + + node.update(0, clippingRect); + expect(node.isRenderable).toBe(false); + + node.texture.state = 'loaded'; + node.setUpdateType(UpdateType.IsRenderable); + node.update(1, clippingRect); + + expect(node.isRenderable).toBe(true); + }); + + it('a node with a texture with alpha 0 should not be renderable', () => { + const node = new CoreNode(stage, defaultProps); + node.alpha = 0; + node.x = 0; + node.y = 0; + node.width = 100; + node.height = 100; + node.texture = mock({ + state: 'loaded', + }); + + node.update(0, clippingRect); + expect(node.isRenderable).toBe(false); + }); + + it('a node with a texture that is OutOfBounds should not be renderable', () => { + const node = new CoreNode(stage, defaultProps); + node.alpha = 1; + node.x = 300; + node.y = 300; + node.width = 100; + node.height = 100; + node.texture = mock({ + state: 'loaded', + }); + + node.update(0, clippingRect); + expect(node.isRenderable).toBe(false); + }); + + it('a node with a freed texture should not be renderable', () => { + const node = new CoreNode(stage, defaultProps); + node.alpha = 1; + node.x = 0; + node.y = 0; + node.width = 100; + node.height = 100; + node.texture = mock({ + state: 'freed', + }); + + node.update(0, clippingRect); + expect(node.isRenderable).toBe(false); + }); +}); diff --git a/src/core/CoreNode.ts b/src/core/CoreNode.ts index 64e60a07..cc7df500 100644 --- a/src/core/CoreNode.ts +++ b/src/core/CoreNode.ts @@ -768,7 +768,16 @@ export class CoreNode extends EventEmitter { UpdateType.RenderState, ); - this.createDefaultTexture(); + // if the default texture isn't loaded yet, wait for it to load + // this only happens when the node is created before the stage is ready + if ( + this.stage.defaultTexture && + this.stage.defaultTexture.state !== 'loaded' + ) { + this.stage.defaultTexture.once('loaded', () => { + this.setUpdateType(UpdateType.IsRenderable); + }); + } } //#region Textures @@ -808,18 +817,6 @@ export class CoreNode extends EventEmitter { }); } - createDefaultTexture(): void { - // load default texture if no texture is set - if ( - this.stage.defaultTexture !== null && - this.props.src === null && - this.props.texture === null && - this.props.rtt === false - ) { - this.texture = this.stage.defaultTexture; - } - } - unloadTexture(): void { if (this.texture !== null) { this.texture.off('loaded', this.onTextureLoaded); @@ -1222,46 +1219,6 @@ export class CoreNode extends EventEmitter { } } - //check if CoreNode is renderable based on props - hasRenderableProperties(): boolean { - if (this.texture !== null) { - return true; - } - - if (!this.props.width || !this.props.height) { - return false; - } - - if (this.props.shader !== this.stage.defShaderCtr) { - return true; - } - - if (this.props.clipping === true) { - return true; - } - - if (this.props.color !== 0) { - return true; - } - - // Consider removing these checks and just using the color property check above. - // Maybe add a forceRender prop for nodes that should always render. - if ( - this.props.colorTop !== 0 || - this.props.colorBottom !== 0 || - this.props.colorLeft !== 0 || - this.props.colorRight !== 0 || - this.props.colorTl !== 0 || - this.props.colorTr !== 0 || - this.props.colorBl !== 0 || - this.props.colorBr !== 0 - ) { - return true; - } - - return false; - } - checkRenderBounds(): CoreNodeRenderState { assertTruthy(this.renderBound); assertTruthy(this.strictBound); @@ -1391,28 +1348,100 @@ export class CoreNode extends EventEmitter { } /** - * This function updates the `isRenderable` property based on certain conditions. - * - * @returns + * Updates the `isRenderable` property based on various conditions. */ updateIsRenderable() { - let newIsRenderable: boolean; - if (this.worldAlpha === 0 || this.hasRenderableProperties() === false) { - newIsRenderable = false; + let newIsRenderable = false; + let needsTextureOwnership = false; + + // If the node is out of bounds, has no dimensions or has an alpha of 0, it is not renderable + if (this.checkBasicRenderability() === false) { + this.updateTextureOwnership(false); + this.setRenderable(false); + return; + } + + if (this.texture !== null) { + needsTextureOwnership = true; + + // we're only renderable if the texture state is loaded + newIsRenderable = this.texture.state === 'loaded'; + } else if ( + this.hasColorProperties() === true && + this.hasDimensions() === true + ) { + // This mean we have dimensions and a color set, so we can render a ColorTexture + if ( + this.stage.defaultTexture && + this.stage.defaultTexture.state === 'loaded' + ) { + newIsRenderable = true; + } + } + + this.updateTextureOwnership(needsTextureOwnership); + this.setRenderable(newIsRenderable); + } + + /** + * Checks if the node is renderable based on world alpha, dimensions and out of bounds status. + */ + checkBasicRenderability(): boolean { + if (this.worldAlpha === 0 || this.isOutOfBounds() === true) { + return false; } else { - newIsRenderable = this.renderState > CoreNodeRenderState.OutOfBounds; + return true; } + } - if (this.isRenderable !== newIsRenderable) { - this.isRenderable = newIsRenderable; - this.onChangeIsRenderable(newIsRenderable); + /** + * Sets the renderable state and triggers changes if necessary. + * @param isRenderable - The new renderable state + */ + setRenderable(isRenderable: boolean) { + if (this.isRenderable !== isRenderable) { + this.isRenderable = isRenderable; } } - onChangeIsRenderable(isRenderable: boolean) { + /** + * Changes the renderable state of the node. + */ + updateTextureOwnership(isRenderable: boolean) { this.texture?.setRenderableOwner(this, isRenderable); } + /** + * Checks if the node is out of the viewport bounds. + */ + isOutOfBounds(): boolean { + return this.renderState <= CoreNodeRenderState.OutOfBounds; + } + + /** + * Checks if the node has dimensions (width/height) + */ + hasDimensions(): boolean { + return this.props.width !== 0 && this.props.height !== 0; + } + + /** + * Checks if the node has any color properties set. + */ + hasColorProperties(): boolean { + return ( + this.props.color !== 0 || + this.props.colorTop !== 0 || + this.props.colorBottom !== 0 || + this.props.colorLeft !== 0 || + this.props.colorRight !== 0 || + this.props.colorTl !== 0 || + this.props.colorTr !== 0 || + this.props.colorBl !== 0 || + this.props.colorBr !== 0 + ); + } + calculateRenderCoords() { const { width, height, globalTransform: transform } = this; assertTruthy(transform); @@ -1553,7 +1582,6 @@ export class CoreNode extends EventEmitter { assertTruthy(this.globalTransform); assertTruthy(this.renderCoords); - assertTruthy(this.texture); // add to list of renderables to be sorted before rendering renderer.addQuad({ @@ -1563,7 +1591,9 @@ export class CoreNode extends EventEmitter { colorTr: this.premultipliedColorTr, colorBl: this.premultipliedColorBl, colorBr: this.premultipliedColorBr, - texture: this.texture, + // if we do not have a texture, use the default texture + // this assumes any renderable node is either a distinct texture or a ColorTexture + texture: this.texture || this.stage.defaultTexture, textureOptions: this.textureOptions, zIndex: this.zIndex, shader: this.shader.shader, @@ -2049,6 +2079,7 @@ export class CoreNode extends EventEmitter { this.stage.txManager.loadTexture(this.texture, true); this.texture.once('loaded', () => { this.stage.renderer?.renderToTexture(this); // Only this RTT node + this.setUpdateType(UpdateType.IsRenderable); }); } @@ -2229,11 +2260,8 @@ export class CoreNode extends EventEmitter { this.props.texture = value; if (value !== null) { - value.setRenderableOwner(this, this.isRenderable); + value.setRenderableOwner(this, this.isRenderable); // WVB TODO: check if this is correct this.loadTexture(); - } else { - // If the texture is null, create a default texture - this.createDefaultTexture(); } this.setUpdateType(UpdateType.IsRenderable); diff --git a/src/core/CoreTextNode.ts b/src/core/CoreTextNode.ts index db6368a8..3c7f0a15 100644 --- a/src/core/CoreTextNode.ts +++ b/src/core/CoreTextNode.ts @@ -368,15 +368,20 @@ export class CoreTextNode extends CoreNode implements CoreTextNodeProps { this.textRenderer.set.y(this.trState, this.globalTransform.ty); } - override hasRenderableProperties(): boolean { + override checkBasicRenderability() { + if (this.worldAlpha === 0 || this.isOutOfBounds() === true) { + return false; + } + if (this.trState && this.trState.props.text !== '') { return true; } - return super.hasRenderableProperties(); + + return false; } - override onChangeIsRenderable(isRenderable: boolean) { - super.onChangeIsRenderable(isRenderable); + override setRenderable(isRenderable: boolean) { + super.setRenderable(isRenderable); this.textRenderer.setIsRenderable(this.trState, isRenderable); } diff --git a/src/core/Stage.ts b/src/core/Stage.ts index bfeb1955..6eb5693d 100644 --- a/src/core/Stage.ts +++ b/src/core/Stage.ts @@ -455,11 +455,7 @@ export class Stage { assertTruthy(this.renderer); // If the node is renderable and has a loaded texture, render it - if ( - node.isRenderable === true && - node.texture !== null && - node.texture.state === 'loaded' - ) { + if (node.isRenderable === true) { node.renderQuads(this.renderer); } diff --git a/src/core/text-rendering/font-face-types/SdfTrFontFace/SdfTrFontFace.ts b/src/core/text-rendering/font-face-types/SdfTrFontFace/SdfTrFontFace.ts index 5ae9fe31..af1b8a95 100644 --- a/src/core/text-rendering/font-face-types/SdfTrFontFace/SdfTrFontFace.ts +++ b/src/core/text-rendering/font-face-types/SdfTrFontFace/SdfTrFontFace.ts @@ -89,7 +89,7 @@ export class SdfTrFontFace< // Load the texture stage.txManager.loadTexture(this.texture, true); - this.texture.on('loaded', () => { + this.texture.once('loaded', () => { this.checkLoaded(); // Make sure we mark the stage for a re-render (in case the font's texture was freed and reloaded) stage.requestRender(); diff --git a/src/core/textures/Texture.ts b/src/core/textures/Texture.ts index 282f152d..627819af 100644 --- a/src/core/textures/Texture.ts +++ b/src/core/textures/Texture.ts @@ -200,10 +200,14 @@ export abstract class Texture extends EventEmitter { */ setRenderableOwner(owner: unknown, renderable: boolean): void { const oldSize = this.renderableOwners.size; + if (renderable === true) { - this.renderableOwners.add(owner); - const newSize = this.renderableOwners.size; + if (this.renderableOwners.has(owner) === false) { + // Add the owner to the set + this.renderableOwners.add(owner); + } + const newSize = this.renderableOwners.size; if (newSize > oldSize && newSize === 1) { (this.renderable as boolean) = true; (this.lastRenderableChangeTime as number) = this.txManager.frameTime; From 1bdb75e865be8dbadb6d8cd1383d6bb831718773 Mon Sep 17 00:00:00 2001 From: wouterlucas Date: Thu, 9 Jan 2025 20:42:41 +0100 Subject: [PATCH 2/3] chore: cleanup leftover comment --- src/core/CoreNode.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/core/CoreNode.ts b/src/core/CoreNode.ts index cc7df500..c7ee866e 100644 --- a/src/core/CoreNode.ts +++ b/src/core/CoreNode.ts @@ -1354,7 +1354,7 @@ export class CoreNode extends EventEmitter { let newIsRenderable = false; let needsTextureOwnership = false; - // If the node is out of bounds, has no dimensions or has an alpha of 0, it is not renderable + // If the node is out of bounds or has an alpha of 0, it is not renderable if (this.checkBasicRenderability() === false) { this.updateTextureOwnership(false); this.setRenderable(false); From a168da49da1c12a2adf74245417f5c800e1481d7 Mon Sep 17 00:00:00 2001 From: wouterlucas Date: Fri, 10 Jan 2025 12:02:27 +0100 Subject: [PATCH 3/3] chore: remove no longer needed If check --- src/core/CoreNode.ts | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/core/CoreNode.ts b/src/core/CoreNode.ts index c7ee866e..d33dee9b 100644 --- a/src/core/CoreNode.ts +++ b/src/core/CoreNode.ts @@ -1399,9 +1399,7 @@ export class CoreNode extends EventEmitter { * @param isRenderable - The new renderable state */ setRenderable(isRenderable: boolean) { - if (this.isRenderable !== isRenderable) { - this.isRenderable = isRenderable; - } + this.isRenderable = isRenderable; } /**