Audio: Copy filters array in setFilters

[takahirox]

Problem this PR fixes

Audio.setFilters() stores a reference to passed array to Audio.filters.

setFilters( value ) {

	if ( ! value ) value = [];

	if ( this._connected === true ) {

		this.disconnect();
		this.filters = value;  // stores reference
		this.connect();

	} else {

		this.filters = value;  // stores reference

	}

	return this;

}

Then for example in the following example, an operation to filters array can have an effect to audio.filters even out of Audio. I don't think this is expected behavior for users.

const audio = new Audio(listener);
const filters = [audio.context.createGain()];
audio.setFilters(filters);
filters.pop(); // This operation has an effect to audio.filters

How this PR fixes

I'd like to suggest copying an array in Audio.setFilters() instead of storing a reference. In the above example an operation to filters won't have an effect to audio.filters. I think it would be better capsulation.

[Mugen87]

I think this change makes senes. At other places in the engine (e.g. Box3.set()), object arguments are not assigned but copied to the respective target properties, too.

[mrdoob]

I'd let javascript do its thing:

this.filters = value.slice(); // clone array

[takahirox]

I'd let javascript do its thing:

this.filters = value.slice(); // clone array

If I'm right, mine is more memory efficient than this.filters = value.slice() because mine reuses this.filters array while this.filters = value.slice() throws away old this.filters array and creating a new copy of value array. this.filters = value.slice() would provide more pressure for Garbage Collection than mine.

But I know Audio.setFilters may not be a function called so often. So it may be ok to prioritize readability by sacrificing memory efficiency.

What do you think? If you still prefer this.filters = value.slice(), I would rewrite.

[mrdoob]

Yeah, I suspect Audio.setFilters wouldn't be called that often so readability may be worth it in this case.

[takahirox]

Thanks for the answer. Yeah it would be reasonable.

I noticed one more difference between them.

const audio = new Audio(listener);
audio.setFilters([audio.context.createGain()]);
const filters = audio.getFilters();
audio.setFilters([]); // Whether should this line have an effect to `filters` or not?

Mine reuses Audio.filters then the second audio.setFilters() have an effect to filters in the above example because filters keeps holding Audio.filters.

The current implementation or this.filters = value.slice() throws away an old array and Audio.filters points to a new array, then the second audio.setFilters() doesn't have an effect to filters. filters holds old Audio.filters.

Which one should be right behavior do you think? I don't have strong opinion about it.

[takahirox]

Replaced with this.filters = value.slice()

[Mugen87]

@mrdoob I think this PR is good to go 👍 .

[mrdoob]

Thanks!