Repository: lightning-js/renderer (ts)

Issue:
All nodes are renderable
I was looking to fix: https://github.com/lightning-js/renderer/issues/463 and noticed it wasn't happening any longer in main. This is because all nodes are now renderable due to 
https://github.com/lightning-js/renderer/blob/main/src/core/CoreNode.ts#L771

Every node is being given a texture making it renderable. I assume you don't want that to be the case as every container node is now being rendered.

For instance:
```js
const yellowRect = renderer.createNode({
    x: 300,
    y: 0,
    width: 100,
    height: 100,
    color: 0x00000000,
    parent: testRoot,
  });
```
Gets the defaultTexture, even though it shouldn't be rendered.

Pull request:
Refactor isRenderable detection
✨ What changed? ✨ 

* Decouple **isRenderable** from **hasRenderableOwner** as textures can be loaded asynchronously.
* Refactored the `isRenderable` detection:
  - If a node has `alpha = 0` or is off screen its not renderable
  - If a node has dimensions and color properties, its marked renderable and we assume its a `ColorTexture`.
  - If a node has a texture, it will be marked as a renderable owner. Which will trigger loading of the texture (if its the first owner)
    - If the texture is loaded, it is marked renderable
* On `renderQuads` we assume everything has a texture if not we set the defaultTexture for 1x1 sampling. This is implicit but matches the old behaviour.
* `CoreTextNode` overwrites behaviour and checks for text !== ''. 
* Also fixed a bug where the default texture was still loading but already referenced.
* Added a `CoreNode` unit test to validate this more rapidly. 

🔧 Addresses the following
Fixes #477 



Patch:
diff --git a/src/core/CoreNode.ts b/src/core/CoreNode.ts
index 64e60a0..d33dee9 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,98 @@ 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;
-    } else {
-      newIsRenderable = this.renderState > CoreNodeRenderState.OutOfBounds;
+    let newIsRenderable = false;
+    let needsTextureOwnership = false;
+
+    // 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);
+      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;
+      }
     }
 
-    if (this.isRenderable !== newIsRenderable) {
-      this.isRenderable = newIsRenderable;
-      this.onChangeIsRenderable(newIsRenderable);
+    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 {
+      return true;
     }
   }
 
-  onChangeIsRenderable(isRenderable: boolean) {
+  /**
+   * Sets the renderable state and triggers changes if necessary.
+   * @param isRenderable - The new renderable state
+   */
+  setRenderable(isRenderable: boolean) {
+    this.isRenderable = isRenderable;
+  }
+
+  /**
+   * 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 +1580,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 +1589,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 +2077,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 +2258,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 db6368a..3c7f0a1 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 bfeb195..6eb5693 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 5ae9fe3..af1b8a9 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 282f152..627819a 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;


Test patch:
diff --git a/src/core/CoreNode.test.ts b/src/core/CoreNode.test.ts
index 1d53881..ce590a8 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<BaseShaderController>(),
-    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<BaseShaderController>(),
+  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<Stage>({
+  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<Stage>(), 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<Stage>(), 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<ImageTexture>({
+      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<ImageTexture>({
+      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<ImageTexture>({
+      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<ImageTexture>({
+      state: 'freed',
+    });
+
+    node.update(0, clippingRect);
+    expect(node.isRenderable).toBe(false);
+  });
+});