Skip to content
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.

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

Audio: Prevent null source error in disconnect(). #26597

Merged
merged 1 commit into from Aug 21, 2023

Conversation

takahirox
Copy link
Collaborator

@takahirox takahirox commented Aug 18, 2023

Problem

Audio.disconnect() expects Audio.source is non-null and calls Audio.source.disconnect(). But Audio.source is initialized as null in the constructor and later it is set in .play() or .set*() methods.

So if Audio.disconnect() is called before starting to play or setting source, it can cause an error when attemping to call Audio.source.disconnect()

Solution

In Audio.disconnect() add a guard that checks audio is already connected.

Alternative solutions

  1. If we think it should be managed on user end whether .disconnect() can be called it would be good to add an API to know whether it's connected. Currently users can check with Audio.source !== null but it may not be intuitive. We may add a getter for ._connected.
// in Audio
get connected() {
  return this._connected;
}

// User code
if (audio.connected) {
  audio.disconnect();
}
  1. Add a description in the documents that calling Audio.disconnect() before starting to play or setting source can cause an error.

**Problem**

Audio.disconnect() expects Audio.source is non-null and
calls Audio.source.disconnect(). But Audio.source is
initialized as null in the constructor and later it is
set in .play() or set*() methods.

So if Audio.disconnect() is called before starting to
play or setting source, it can cause an error when
attemping to call Audio.source.disconnect()

**Solution**

Add a guard in Audio.disconnect()
@github-actions
Copy link

📦 Bundle size

Full ESM build, minified and gzipped.

Filesize dev Filesize PR Diff
646.2 kB (160.3 kB) 646.2 kB (160.3 kB) +26 B

🌳 Bundle size after tree-shaking

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

Filesize dev Filesize PR Diff
439.1 kB (106.4 kB) 439.1 kB (106.4 kB) +0 B

@Mugen87 Mugen87 added this to the r156 milestone Aug 18, 2023
@Mugen87 Mugen87 merged commit 6ce2492 into mrdoob:dev Aug 21, 2023
19 checks passed
@Mugen87 Mugen87 changed the title Audio: Prevent null source error in Audio.disconnect() Audio: Prevent null source error in disconnect(). Aug 21, 2023
@takahirox takahirox deleted the AudioDisconnectCheckConnected branch August 21, 2023 15:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants