Skip to content

Fix material readiness to gate on light texture readiness #18255

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.

Sign up for GitHub

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

Draft
bghgary wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Fix material readiness to gate on light texture readiness #18255

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions packages/dev/core/src/Lights/light.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -945,6 +945,15 @@ export abstract class Light extends Node implements ISortableLight {
this.getScene().sortLightsByPriority();
}

/**
* Returns true when all texture resources used by this light are ready (e.g. projection textures).
* Override in subclasses that use texture resources.
* @returns true if all light textures are ready
*/
public areLightTexturesReady(): boolean {
return true;
}

/**
* Prepares the list of defines specific to the light type.
* @param defines the list of defines
Expand Down
11 changes: 11 additions & 0 deletions packages/dev/core/src/Lights/spotLight.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -511,6 +511,17 @@ export class SpotLight extends ShadowLight {
return engine.useReverseDepthBuffer && engine.isNDCHalfZRange ? 0 : maxZ;
}

/** @override */
public override areLightTexturesReady(): boolean {
if (this._projectionTexture && !this._projectionTexture.isReady()) {
return false;
}
if (this._iesProfileTexture && !this._iesProfileTexture.isReady()) {
Comment on lines +516 to +519
Copy link

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This readiness check uses isReady(), which ignores BaseTexture.isBlocking semantics used elsewhere in material readiness (via isReadyOrNotBlocking()). To avoid unexpectedly blocking scene.executeWhenReady() for explicitly non-blocking projection/IES textures, consider using isReadyOrNotBlocking() here (or otherwise documenting/justifying why light textures should always be blocking).

Suggested change
if (this._projectionTexture && !this._projectionTexture.isReady()) {
return false;
}
if (this._iesProfileTexture && !this._iesProfileTexture.isReady()) {
if (this._projectionTexture && !this._projectionTexture.isReadyOrNotBlocking()) {
return false;
}
if (this._iesProfileTexture && !this._iesProfileTexture.isReadyOrNotBlocking()) {

Copilot uses AI. Check for mistakes.
return false;
}
return true;
}

/**
* Prepares the list of defines specific to the light type.
* @param defines the list of defines
Expand Down
4 changes: 4 additions & 0 deletions packages/dev/core/src/Materials/Background/backgroundMaterial.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -606,6 +606,10 @@ export class BackgroundMaterial extends BackgroundMaterialBase {
PrepareDefinesForLights(scene, mesh, defines, false, this._maxSimultaneousLights);
defines._needNormals = true;

if (defines._areLightTexturesReady === false) {
return false;
}

// Multiview
PrepareDefinesForMultiview(scene, defines);

Expand Down
3 changes: 3 additions & 0 deletions packages/dev/core/src/Materials/Node/Blocks/Dual/lightBlock.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -243,13 +243,16 @@ export class LightBlock extends NodeMaterialBlock {
lightmapMode: false,
shadowEnabled: false,
specularEnabled: false,
lightTexturesReady: true,
};

PrepareDefinesForLight(scene, mesh, this.light, this._lightId, defines, true, state);

if (state.needRebuild) {
defines.rebuild();
}

defines._areLightTexturesReady = state.lightTexturesReady;
Copy link

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the single-light path, this assignment overwrites defines._areLightTexturesReady each time prepareDefines runs. If multiple LightBlocks are used (each bound to a different light), a later ready light can flip the flag back to true, masking an earlier not-ready light texture. Accumulate the readiness across blocks (eg keep it false once any light reports not ready).

Suggested change
defines._areLightTexturesReady = state.lightTexturesReady;
defines._areLightTexturesReady = defines._areLightTexturesReady !== false && state.lightTexturesReady;

Copilot uses AI. Check for mistakes.
}
}

Expand Down
3 changes: 3 additions & 0 deletions packages/dev/core/src/Materials/Node/Blocks/PBR/pbrMetallicRoughnessBlock.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -834,13 +834,16 @@ export class PBRMetallicRoughnessBlock extends NodeMaterialBlock {
lightmapMode: false,
shadowEnabled: false,
specularEnabled: false,
lightTexturesReady: true,
};

PrepareDefinesForLight(scene, mesh, this.light, this._lightId, defines, true, state);

if (state.needRebuild) {
defines.rebuild();
}

defines._areLightTexturesReady = state.lightTexturesReady;
Copy link

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same overwrite issue as LightBlock: defines._areLightTexturesReady is set from a per-light state, so with multiple instances/blocks the last evaluated light wins. This can incorrectly report the material as ready while some light textures are still loading. Consider AND-ing with the existing flag (defaulting to true) so readiness is aggregated across all light blocks.

Suggested change
defines._areLightTexturesReady = state.lightTexturesReady;
defines._areLightTexturesReady = (defines._areLightTexturesReady ?? true) && state.lightTexturesReady;

Copilot uses AI. Check for mistakes.
}
}

Expand Down
4 changes: 4 additions & 0 deletions packages/dev/core/src/Materials/Node/nodeMaterial.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1665,6 +1665,10 @@ export class NodeMaterial extends NodeMaterialBase {

const result = this._processDefines(defines, mesh, useInstances, subMesh);

if (defines._areLightTexturesReady === false) {
return false;
}

if (result) {
const previousEffect = subMesh.effect;
// Compilation
Expand Down
4 changes: 4 additions & 0 deletions packages/dev/core/src/Materials/PBR/openpbrMaterial.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2241,6 +2241,10 @@ export class OpenPBRMaterial extends OpenPBRMaterialBase {
const lightDisposed = defines._areLightsDisposed;
const effect = this._prepareEffect(mesh, subMesh.getRenderingMesh(), defines, this.onCompiled, this.onError, useInstances, null);

if (defines._areLightTexturesReady === false) {
return false;
}

let forceWasNotReadyPreviously = false;

if (effect) {
Expand Down
4 changes: 4 additions & 0 deletions packages/dev/core/src/Materials/PBR/pbrBaseMaterial.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1194,6 +1194,10 @@ export abstract class PBRBaseMaterial extends PBRBaseMaterialBase {
const lightDisposed = defines._areLightsDisposed;
const effect = this._prepareEffect(mesh, subMesh.getRenderingMesh(), defines, this.onCompiled, this.onError, useInstances, null);

if (defines._areLightTexturesReady === false) {
return false;
}

Comment on lines 1195 to +1200
Copy link

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The _areLightTexturesReady gate happens after _prepareEffect(), which can compile/cache an effect with projected/IES defines disabled while textures are still loading. Consider checking _areLightTexturesReady before calling _prepareEffect() to avoid unnecessary compilation and caching of an effect that will never be used once the textures become ready.

Suggested change
const effect = this._prepareEffect(mesh, subMesh.getRenderingMesh(), defines, this.onCompiled, this.onError, useInstances, null);
if (defines._areLightTexturesReady === false) {
return false;
}
if (defines._areLightTexturesReady === false) {
return false;
}
const effect = this._prepareEffect(mesh, subMesh.getRenderingMesh(), defines, this.onCompiled, this.onError, useInstances, null);

Copilot uses AI. Check for mistakes.
let forceWasNotReadyPreviously = false;

if (effect) {
Expand Down
7 changes: 7 additions & 0 deletions packages/dev/core/src/Materials/materialHelper.functions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -667,6 +667,7 @@ export function PrepareDefinesForLights(scene: Scene, mesh: AbstractMesh, define
lightmapMode: false,
shadowEnabled: false,
specularEnabled: false,
lightTexturesReady: true,
};

if (scene.lightsEnabled && !disableLighting) {
Expand All @@ -682,6 +683,7 @@ export function PrepareDefinesForLights(scene: Scene, mesh: AbstractMesh, define

defines["SPECULARTERM"] = state.specularEnabled;
defines["SHADOWS"] = state.shadowEnabled;
defines._areLightTexturesReady = state.lightTexturesReady;

// Resetting all other lights if any
const maxLightCount = Math.max(maxSimultaneousLights, defines["MAXLIGHTCOUNT"] || 0);
Expand Down Expand Up @@ -899,6 +901,7 @@ export function PrepareDefinesForLight(
shadowEnabled: boolean;
specularEnabled: boolean;
lightmapMode: boolean;
lightTexturesReady: boolean;
}
) {
state.needNormals = true;
Expand All @@ -918,6 +921,10 @@ export function PrepareDefinesForLight(

light.prepareLightSpecificDefines(defines, lightIndex);

if (!light.areLightTexturesReady()) {
state.lightTexturesReady = false;
}
Comment on lines 921 to +926
Copy link

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change introduces new readiness behavior (materials are not ready until light textures are ready), but there are no automated tests guarding it. Adding a unit test (NullEngine) that sets a SpotLight projection/IES texture to a not-yet-ready texture and asserts material.isReadyForSubMesh(...)/scene.isReady() remains false until the texture becomes ready would help prevent regressions.

Copilot generated this review using guidance from repository custom instructions.

// FallOff.
defines["LIGHT_FALLOFF_PHYSICAL" + lightIndex] = false;
defines["LIGHT_FALLOFF_GLTF" + lightIndex] = false;
Expand Down
2 changes: 2 additions & 0 deletions packages/dev/core/src/Materials/materialHelper.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,6 +216,7 @@ export class MaterialHelper {
* @param state.shadowEnabled
* @param state.specularEnabled
* @param state.lightmapMode
* @param state.lightTexturesReady
*/
public static PrepareDefinesForLight: (
scene: Scene,
Expand All @@ -230,6 +231,7 @@ export class MaterialHelper {
shadowEnabled: boolean;
specularEnabled: boolean;
lightmapMode: boolean;
lightTexturesReady: boolean;
}
) => void = PrepareDefinesForLight;

Expand Down
4 changes: 4 additions & 0 deletions packages/dev/core/src/Materials/standardMaterial.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -745,6 +745,10 @@ export class StandardMaterial extends StandardMaterialBase {
// Lights
defines._needNormals = PrepareDefinesForLights(scene, mesh, defines, true, this._maxSimultaneousLights, this._disableLighting);

if (defines._areLightTexturesReady === false) {
return false;
}

// Multiview
PrepareDefinesForMultiview(scene, defines);

Expand Down
Loading