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

Remove audio-params component #4562

Merged
merged 34 commits into from
Aug 25, 2021
Merged

Remove audio-params component #4562

merged 34 commits into from
Aug 25, 2021

Conversation

johnshaughnessy
Copy link
Contributor

@johnshaughnessy johnshaughnessy commented Aug 21, 2021

This PR removes the audio-params aframe component and introduces a simple contract for handling audio nodes and audio settings.

The contract is:

  • When a THREE.Audio is created, we put it in APP.audios and we call updateAudioSettings.
  • We put any state that affects audio settings into various collections (APP.sceneOverrides, APP.zoneOverrides, etc). When state is added or removed from these collections, we call updateAudioSettings.

The updateAudioSettings function reduces over the collections to determine what the audio settings should be and then applies them. When reviewing this PR, I recommend that you start by reading updateAudioSettings: https://github.com/mozilla/hubs/blob/audio-refactor-v1/src/update-audio-settings.js

This fixes a few problems:

  • In the previous code, whichever "layer" of audio settings were set most recently overwrites all previous state. For example, the user might enter an audio zone which affects the gain of an audio node. Then, the user might dismiss a linked copy of that node (via linked-media) and reset the gain to 0.5. The effect from the zone is overwritten (which is not desired). This type of problem is repeated at every "layer".
  • In the previous code, we kept duplicate copies of panner node data in the audio-params component. This meant we had a bunch of book-keeping code to keep data in sync. There were subtle bugs in that book-keeping code that meant the two diverged. This is fixed by getting rid of the indirection and second copies of the "same" data.
  • In the previous code, we had bugs related to the initialization order of aframe components. One component looked for properties of another component that may not have been initialized yet. (For example, Cannot read property 'data' of undefinedCannot read property 'data' of undefined #4456 ). In this PR, whenever we have a new setting for audio, we can confidently store it in our collections (whether the audio for the element in question exists yet or not). If there exists an audio, we update its settings. If not, we know that our settings were stored in the collection and will be used once the audio is created.
  • Similarly, in the previous code, any settings that affects an element's audio node would be lost if we replace that element's audio node with a new one (as in the case of switching from a PositionalAudio to a regular Audio). Although the code paths that handle this kind of replacement need further investigation, it is easy to tell what should happen in these cases: The node should be replaced and the associated settings should be applied to the new node.

The AudioNormalization code is no longer being used and should be re-introduced in a followup PR.

┆Issue is synchronized with this Jira Task

keianhzo and others added 28 commits August 21, 2021 10:51
This is a work-in-progress commit. Hubs "works", but some things need to
be fixed or cleaned up:
- Gain is not properly applied. This will happen in a followup commit.
- The audio params component keeps a separate copy of the "source type" state.
- Audio Zones need to be tested.
- The audio settings system should loop over all audios when the
settings change. (Perhaps this system can just go away.)
- A bunch of calls to `setAttribute("audio-params", { ... })` should
eventually disappear, but not yet.
Some audio layers do not want to affect every possible audio parameter,
so we do not require them to.

-  // TODO: The DEBUG PANEL only wants to set a few settings at a time
-  // TODO: Perhaps AUDIO ZONEs should be allowed to affect only a few settings at a time
Obviously, we are removing this component. But even if we were not,
having multiple of these components on the same entity would absolutely
break things.
@johnshaughnessy johnshaughnessy merged commit a35c28b into master Aug 25, 2021
@johnshaughnessy johnshaughnessy deleted the audio-refactor-v1 branch August 25, 2021 16:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants