TSL: Fix onDispose listener not being removed in Sampler

[shotamatsuda]

Fixes: #31866

Description

This PR fixes that the event listeners on textures are not removed and grow indefinitely. Heap snapshots of webgpu_cubemap_dynamic no longer contain a large array of onDispose listeners.

[github-actions]

📦 Bundle size

Full ESM build, minified and gzipped.

	Before	After	Diff
WebGL	338.24 79.14	338.24 79.14	+0 B +0 B
WebGPU	579.84 160.2	579.91 160.21	+73 B +10 B
WebGPU Nodes	578.45 159.96	578.52 159.97	+73 B +10 B

🌳 Bundle size after tree-shaking

Minimal build including a renderer, camera, empty scene, and dependencies.

	Before	After	Diff
WebGL	469.88 113.85	469.88 113.85	+0 B +0 B
WebGPU	649.55 175.86	649.62 175.88	+73 B +23 B
WebGPU Nodes	603.66 165.03	603.73 165.04	+73 B +10 B

[Mugen87]

I'm afraid this does not work. When EventDispatcher executes the callback, this will point to the texture, not the Sampler class.

[shotamatsuda]

Oh, that's correct. It needs binding or to be an arrow function.

[Mugen87]

Now the version is restored before #31751, introducing a different bug.

[shotamatsuda]

Does _onTextureDispose have to be recreated when cloned?

[Mugen87]

A sampler binding always has a reference to a texture. Before #31751, disposing textures can lead to sampler bindings which point to a destroyed textures. The texture is of course not rendered correctly and WebGPU warnings are displayed.

#31751 made sure that cloned samplers have correct dispose handlers by creating them inline. The issue is the removal is not implemented correctly since it refers to the wrong dispose handler. I think we can fix this issue by keeping track of the current dispose handler in a sampler property and use it in removeEventListener().

[Mugen87]

I'm not sure anymore if we need to create dispose inline. Can you please make a test with the fiddles from #31747 (comment) and verify if the error pops up again.

Maybe just adding clone() was already sufficient to fix the issue.

[shotamatsuda]

I'm seeing only the following error:

Uncaught TypeError: Cannot read properties of undefined (reading 'call')
    at Texture.dispatchEvent (EventDispatcher.js:118:1)
    at Texture.dispose (Texture.js:644:1)

This should be caused by this.texture = texture in the constructor before populating _onTextureDispose, which needs to be moved before that line.

[shotamatsuda]

Since texture is compared with the strict equality operator in set texture(), we need to initialize it with null before setting it via the setter.

[Mugen87]

Would this version work:

import Binding from './Binding.js';

/**
 * Represents a sampler binding type.
 *
 * @private
 * @augments Binding
 */
class Sampler extends Binding {

	/**
	 * Constructs a new sampler.
	 *
	 * @param {string} name - The samplers's name.
	 * @param {?Texture} texture - The texture this binding is referring to.
	 */
	constructor( name, texture ) {

		super( name );

		/**
		 * The texture the sampler is referring to.
		 *
		 * @type {?Texture}
		 */
		this.texture = texture;

		/**
		 * The binding's version.
		 *
		 * @type {number}
		 */
		this.version = texture ? texture.version : 0;

		/**
		 * The binding's generation which is an additional version
		 * qualifier.
		 *
		 * @type {?number}
		 * @default null
		 */
		this.generation = null;

		/**
		 * This flag can be used for type testing.
		 *
		 * @type {boolean}
		 * @readonly
		 * @default true
		 */
		this.isSampler = true;

		/**
		 * An event listener which is added to {@link texture}'s dispose event.
		 *
		 * @private
		 * @type {Function}
		 */
		this._currentDispose = null;

	}

	/**
	 * Sets the texture of this sampler.
	 * @param {?Texture} value - The texture to set.
	 */
	set texture( value ) {

		if ( this._texture === value ) return;

		if ( this._texture ) {

			this._texture.removeEventListener( 'dispose', this._currentDispose );
			this._currentDispose = null;

		}

		this._texture = value;

		this.generation = null;
		this.version = 0;

		if ( this._texture ) {

			this._currentDispose = () => {

				this._texture = null;
				this.generation = null;
				this.version = 0;

			};

			this._texture.addEventListener( 'dispose', this._currentDispose );

		}

	}

	/**
	 * Gets the texture of this sampler.
	 * @return {?Texture} The texture.
	 */
	get texture() {

		return this._texture;

	}

	/**
	 * Updates the binding.
	 *
	 * @return {boolean} Whether the texture has been updated and must be
	 * uploaded to the GPU.
	 */
	update() {

		const { texture, version } = this;

		if ( version !== texture.version ) {

			this.version = texture.version;

			return true;

		}

		return false;

	}


	clone() {

		const clonedSampler = super.clone();

		// fix dispose handler for cloned instances
		// TODO: Find better solution, see #31747

		clonedSampler._texture = null;
		clonedSampler.texture = this.texture;

		return clonedSampler;

	}

}

export default Sampler;

[shotamatsuda]

Yes, your code does basically the same thing as the changes I made. The only difference is whether to lazily initialize the dispose listener.

But your _currentDispose is recreated on every call (though it removes the previous one correctly). Is it really necessary?

[Mugen87]

No, you version looks cleaner. Let's go with it.

Like mentioned in the comments, the entire module needs some refactoring since the binding/texture handling is quite fragile and hard to follow right now.

[shotamatsuda]

I mean, doesn't it work just lazily initialize the listener like the following:

// Note that the strict equality operator doesn't work here because the assignment 
// via the setter occurs before `_currentDispose` is initialized with `null`.
if ( this._currentDispose == null ) {

	this._currentDispose = () => {

		this._texture = null;
		this.generation = null;
		this.version = 0;

	};

}

[Mugen87]

Oh, did I merge too soon? If so, please make a new PR with your suggestion.

[Mugen87]

I mean, doesn't it work just lazily initialize the listener like the following:

I'm okay with it as long as the use case from #31747 (comment) does not break again.

[shotamatsuda]

Oh, did I merge too soon?

No, our messages crossed. 😓
The code in clone() and the timing issue in the texture setter are hard to notice, though.