openvibers/openvibe.games Commits

Guard texture updates after scene disposal (#328)

OP OpenVibers committed 4 days ago · 87dd5e98ba · 6 files, +240 −5

Changes

ddiff --git a/apps/client/src/render/ktx2.test.ts b/apps/client/src/render/ktx2.test.ts
iindex 1416d48..ee7989f 100644
---- a/apps/client/src/render/ktx2.test.ts
++++ b/apps/client/src/render/ktx2.test.ts
@@@ -1,7 +1,52 @@
import { afterEach, describe, expect, it, vi } from 'vitest'
--import { builtinTexSource, ktx2Enabled, resetKtx2ForTests } from './ktx2'
++import type { Scene } from '@babylonjs/core/scene.js'
++import { builtinTexSource, ktx2Enabled, loadTexture, resetKtx2ForTests } from './ktx2'
import { resolveTexInfo } from './mapStyle'
++/**
++ * A Texture that does not touch the GPU: it records the error callback Babylon
++ * would call when the KTX2 load fails, so the fallback path can be driven
++ * directly. `getScene()` and `dispose()` follow Babylon's contract (dispose
++ * clears the scene).
++ */
++const textureMock = vi.hoisted(() => ({
++ scene: null as unknown,
++ updateURL: vi.fn(),
++ onError: null as null | (() => void),
++ failInConstructor: false,
++}))
++
++vi.mock('@babylonjs/core/Materials/Textures/texture.js', () => ({
++ Texture: class MockTexture {
++ static readonly TRILLINEAR_SAMPLINGMODE = 3
++ url: string
++ constructor(
++ url: string,
++ scene: unknown,
++ _noMipmap?: boolean,
++ _invertY?: boolean,
++ _samplingMode?: number,
++ _onLoad?: unknown,
++ onError?: () => void,
++ ) {
++ this.url = url
++ textureMock.scene = scene
++ textureMock.onError = onError ?? null
++ if (textureMock.failInConstructor) onError?.()
++ }
++ getScene(): unknown {
++ return textureMock.scene
++ }
++ updateURL(url: string): void {
++ textureMock.updateURL(url)
++ this.url = url
++ }
++ dispose(): void {
++ textureMock.scene = null
++ }
++ },
++}))
++
describe('KTX2 material textures (Games M6)', () => {
afterEach(() => {
resetKtx2ForTests(null)
@@@ -10,8 +55,14 @@ describe('KTX2 material textures (Games M6)', () => {
it('a built-in texture loads its KTX2 with the JPEG as the fallback', () => {
resetKtx2ForTests(true)
-- expect(builtinTexSource('red_brick')).toEqual({ url: '/assets/tex/red_brick.ktx2', fallback: '/assets/tex/red_brick.jpg' })
-- expect(resolveTexInfo('leafy_grass')).toEqual({ url: '/assets/tex/leafy_grass.ktx2', fallback: '/assets/tex/leafy_grass.jpg' })
++ expect(builtinTexSource('red_brick')).toEqual({
++ url: '/assets/tex/red_brick.ktx2',
++ fallback: '/assets/tex/red_brick.jpg',
++ })
++ expect(resolveTexInfo('leafy_grass')).toEqual({
++ url: '/assets/tex/leafy_grass.ktx2',
++ fallback: '/assets/tex/leafy_grass.jpg',
++ })
})
it('without KTX2 the JPEG alone', () => {
@@@ -35,3 +86,58 @@ describe('KTX2 material textures (Games M6)', () => {
expect(ktx2Enabled()).toBe(false)
})
})
++
++describe('loadTexture KTX2 fallback', () => {
++ const liveScene = (): { isDisposed: boolean } => ({ isDisposed: false })
++ const asScene = (scene: { isDisposed: boolean }): Scene => scene as unknown as Scene
++
++ afterEach(() => {
++ textureMock.updateURL.mockClear()
++ textureMock.onError = null
++ textureMock.failInConstructor = false
++ textureMock.scene = null
++ })
++
++ it('switches to the JPEG when the KTX2 fails', () => {
++ const scene = liveScene()
++ loadTexture(
++ { url: '/assets/tex/red_brick.ktx2', fallback: '/assets/tex/red_brick.jpg' },
++ asScene(scene),
++ )
++ expect(textureMock.onError).toBeTypeOf('function')
++ textureMock.onError!()
++ expect(textureMock.updateURL).toHaveBeenCalledWith('/assets/tex/red_brick.jpg')
++ })
++
++ it('does not retry the fallback after the texture was disposed', () => {
++ const scene = liveScene()
++ const tx = loadTexture(
++ { url: '/assets/tex/red_brick.ktx2', fallback: '/assets/tex/red_brick.jpg' },
++ asScene(scene),
++ )
++ tx.dispose()
++ textureMock.onError!()
++ expect(textureMock.updateURL).not.toHaveBeenCalled()
++ })
++
++ it('does not retry the fallback after the scene was disposed', () => {
++ const scene = liveScene()
++ loadTexture(
++ { url: '/assets/tex/red_brick.ktx2', fallback: '/assets/tex/red_brick.jpg' },
++ asScene(scene),
++ )
++ scene.isDisposed = true
++ textureMock.onError!()
++ expect(textureMock.updateURL).not.toHaveBeenCalled()
++ })
++
++ it('still falls back when the decoder fails inside the constructor', () => {
++ textureMock.failInConstructor = true
++ const scene = liveScene()
++ loadTexture(
++ { url: '/assets/tex/red_brick.ktx2', fallback: '/assets/tex/red_brick.jpg' },
++ asScene(scene),
++ )
++ expect(textureMock.updateURL).toHaveBeenCalledWith('/assets/tex/red_brick.jpg')
++ })
++})
ddiff --git a/apps/client/src/render/ktx2.ts b/apps/client/src/render/ktx2.ts
iindex a2a31ed..6a8abb2 100644
---- a/apps/client/src/render/ktx2.ts
++++ b/apps/client/src/render/ktx2.ts
@@@ -8,6 +8,7 @@ import { KhronosTextureContainer2 } from '@babylonjs/core/Misc/khronosTextureCon
import '@babylonjs/core/Materials/Textures/Loaders/ktxTextureLoader.js'
import { Texture } from '@babylonjs/core/Materials/Textures/texture.js'
import type { Scene } from '@babylonjs/core/scene.js'
++import { updateTextureUrl } from './textureUrl.js'
const BASE = '/ktx2/'
@@@ -72,11 +73,20 @@ export function loadTexture(src: TexSource, scene: Scene): Texture {
if (!src.fallback || !src.url.endsWith('.ktx2')) return new Texture(src.url, scene)
configure()
let tx: Texture | null = null
++ // A decoder can fail synchronously inside the constructor, before `tx` is
++ // assigned; remember it and apply the JPEG once there is a texture to update.
++ let failedBeforeAssign = false
const fallBack = (): void => {
++ if (!src.fallback) return
++ if (!tx) {
++ failedBeforeAssign = true
++ return
++ }
// KTX2 decoding can fail after a material has replaced and disposed this
-- // texture. Babylon clears its scene on dispose; updateURL needs that scene.
-- if (tx?.getScene() === scene && !scene.isDisposed && src.fallback) tx.updateURL(src.fallback)
++ // texture (or after the scene went away). updateTextureUrl needs a scene.
++ updateTextureUrl(tx, src.fallback, scene)
}
tx = new Texture(src.url, scene, false, true, Texture.TRILINEAR_SAMPLINGMODE, null, fallBack)
++ if (failedBeforeAssign) updateTextureUrl(tx, src.fallback, scene)
return tx
}
ddiff --git a/apps/client/src/render/modelCache.ts b/apps/client/src/render/modelCache.ts
iindex b095c44..41d2570 100644
---- a/apps/client/src/render/modelCache.ts
++++ b/apps/client/src/render/modelCache.ts
@@@ -12,6 +12,7 @@
import type { AssetContainer } from '@babylonjs/core/assetContainer.js'
import type { Scene } from '@babylonjs/core/scene.js'
import type { TransformNode } from '@babylonjs/core/Meshes/transformNode.js'
++import { guardTextureUpdates } from './textureUrl.js'
export interface ModelInstance {
/** Root of the instantiated hierarchy, ready to parent. */
@@@ -32,13 +33,20 @@ export class ModelCache {
private load(id: string, glb: string): Promise<AssetContainer | null> {
const existing = this.containers.get(id)
if (existing) return existing
++ if (this.scene.isDisposed) return Promise.resolve(null)
const p = (async (): Promise<AssetContainer | null> => {
++ // The glTF loader updates each image texture when its bytes arrive, which
++ // can be after the scene went away (the model's view was deleted or the
++ // editor navigated). Guard the textures it creates during this load.
++ const unguard = guardTextureUpdates(this.scene)
try {
const { LoadAssetContainerAsync } = await import('@babylonjs/core/Loading/sceneLoader.js')
await import('@babylonjs/loaders/glTF/2.0/glTFLoader.js')
return await LoadAssetContainerAsync(glb, this.scene, { pluginExtension: '.glb' })
} catch {
return null
++ } finally {
++ unguard()
}
})()
this.containers.set(id, p)
@@@ -52,6 +60,10 @@ export class ModelCache {
async instantiate(id: string, glb: string): Promise<ModelInstance | null> {
const container = await this.load(id, glb)
if (!container) return null
++ if (this.scene.isDisposed) {
++ container.dispose()
++ return null
++ }
const entries = container.instantiateModelsToScene(undefined, false, {
doNotInstantiate: false,
})
ddiff --git a/apps/client/src/render/textureUrl.test.ts b/apps/client/src/render/textureUrl.test.ts
nnew file mode 100644
iindex 0000000..d785849
---- /dev/null
++++ b/apps/client/src/render/textureUrl.test.ts
@@@ -0,0 +1,58 @@
++import { afterEach, beforeEach, describe, expect, it } from 'vitest'
++import { NullEngine } from '@babylonjs/core/Engines/nullEngine.js'
++import { Scene } from '@babylonjs/core/scene.js'
++import { Texture } from '@babylonjs/core/Materials/Textures/texture.js'
++import { updateTextureUrl, guardTextureUpdates } from './textureUrl.js'
++
++describe('updateTextureUrl', () => {
++ let engine: NullEngine
++ let scene: Scene
++
++ beforeEach(() => {
++ engine = new NullEngine()
++ scene = new Scene(engine)
++ })
++
++ afterEach(() => {
++ if (!scene.isDisposed) scene.dispose()
++ engine.dispose()
++ })
++
++ it('skips a texture whose scene was disposed (the editor E2E pageerror)', () => {
++ const tx = new Texture('/assets/tex/red_brick.ktx2', scene)
++ scene.dispose()
++ expect(tx.getScene()).toBeNull()
++ // The unguarded call is what the browser reports as a page error.
++ expect(() => tx.updateURL('/assets/tex/red_brick.jpg')).toThrow(/markAllMaterialsAsDirty/)
++ expect(updateTextureUrl(tx, '/assets/tex/red_brick.jpg', scene)).toBe(false)
++ })
++
++ it('skips a texture disposed with its material while the scene lives', () => {
++ const tx = new Texture('/assets/tex/red_brick.ktx2', scene)
++ tx.dispose()
++ expect(updateTextureUrl(tx, '/assets/tex/red_brick.jpg', scene)).toBe(false)
++ })
++
++ it('updates a live texture', () => {
++ const tx = new Texture('/assets/tex/red_brick.ktx2', scene)
++ expect(updateTextureUrl(tx, '/assets/tex/red_brick.jpg', scene)).toBe(true)
++ expect(tx.url).toBe('/assets/tex/red_brick.jpg')
++ })
++
++ it('guards every texture a loader creates, even after the scene goes away', () => {
++ const unguard = guardTextureUpdates(scene)
++ const tx = new Texture('/assets/model/image.png', scene)
++ scene.dispose()
++ expect(() => tx.updateURL('/assets/model/other.png')).not.toThrow()
++ expect(tx.url).toBe('/assets/model/image.png')
++ unguard()
++ })
++
++ it('lets a live texture created under the guard update normally', () => {
++ const unguard = guardTextureUpdates(scene)
++ const tx = new Texture('/assets/model/image.png', scene)
++ tx.updateURL('/assets/model/other.png')
++ expect(tx.url).toBe('/assets/model/other.png')
++ unguard()
++ })
++})
ddiff --git a/apps/client/src/render/textureUrl.ts b/apps/client/src/render/textureUrl.ts
nnew file mode 100644
iindex 0000000..d59132b
---- /dev/null
++++ b/apps/client/src/render/textureUrl.ts
@@@ -0,0 +1,46 @@
++/**
++ * Keep Babylon texture URL updates away from textures and scenes that are gone.
++ *
++ * Babylon's Texture.updateURL starts with `this.getScene().markAllMaterialsAsDirty(...)`.
++ * Disposing a texture clears its scene, and disposing a scene is no place to
++ * refresh materials either. An asynchronous load that loses the race — the KTX2
++ * JPEG fallback after the editor replaced the material, or the glTF loader
++ * finishing a model texture after the scene went away — reaches that line with
++ * a null scene and throws "Cannot read properties of null (reading
++ * 'markAllMaterialsAsDirty')". Skip the update instead.
++ */
++import { Texture } from '@babylonjs/core/Materials/Textures/texture.js'
++import type { Scene } from '@babylonjs/core/scene.js'
++
++/** True when the texture was updated; false when it no longer has a live scene. */
++export function updateTextureUrl(tx: Texture, url: string, scene: Scene): boolean {
++ if (tx.getScene() !== scene || scene.isDisposed) return false
++ tx.updateURL(url)
++ return true
++}
++
++const alreadyGuarded = new WeakSet<Texture>()
++
++/**
++ * Guard every texture created until the returned function is called. The glTF
++ * loader is not ours to change, but it creates a Texture for each image and
++ * only calls updateURL once the bytes arrive, so a scene disposed in between
++ * makes that call throw. Wrapping the textures it creates turns the race into a
++ * skipped update instead of a page error.
++ */
++export function guardTextureUpdates(scene: Scene): () => void {
++ const observer = scene.onNewTextureAddedObservable.add((texture) => {
++ if (!(texture instanceof Texture) || alreadyGuarded.has(texture)) return
++ alreadyGuarded.add(texture)
++ const updateUrl = texture.updateURL
++ texture.updateURL = function (url, buffer, onLoad, forcedExtension) {
++ // Same rule as updateTextureUrl: no scene means the texture was disposed
++ // with its material or its scene, so the material refresh would throw.
++ // The scene can also be disposed while the loader is still parsing new
++ // textures for it; those are no more useful than a disposed texture.
++ if (!this.getScene() || scene.isDisposed) return
++ updateUrl.call(this, url, buffer, onLoad, forcedExtension)
++ }
++ })
++ return () => scene.onNewTextureAddedObservable.remove(observer)
++}
ddiff --git a/test/apps__client__src__render__textureUrl.test.js b/test/apps__client__src__render__textureUrl.test.js
nnew file mode 100644
iindex 0000000..fc46c38
---- /dev/null
++++ b/test/apps__client__src__render__textureUrl.test.js
@@@ -0,0 +1,3 @@
++// file: 'apps/client/src/render/textureUrl.test.ts'
++// covers: 'apps/client/src/render'
++// Generated by node test/run.js --index; do not edit.