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.

Sign up for GitHub

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

Jump to bottom

Avatar grunts breaks speech phoneme recognition #3651

Closed
wants to merge 6 commits into from
Closed

Avatar grunts breaks speech phoneme recognition #3651

wants to merge 6 commits into from

Conversation

tcm390
Copy link
Contributor

@tcm390 tcm390 commented Aug 17, 2022 •
edited
Loading

The issue is caused by that both audioWorker and microphoneWorker are setting the value of this.volume

related:
#3645

Result:

Webaverse.-.Google.Chrome.2022-08-17.18-02-11.mp4

@tcm390 tcm390 changed the title disable audio worker setting volume when the volume is too small Avatar grunts breaks speech phoneme recognition Aug 17, 2022
avaer
avaer suggested changes Aug 18, 2022
View reviewed changes
Copy link
Contributor

@avaer avaer left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If volume goes from 1 -> 0 in one frame won't this cause it to get stuck? That's probably an even worse effect than the bug it is fixing.

How about choose which one to emit when it's enabled, not just by chance when it's zero?

@tcm390
Copy link
Contributor Author

tcm390 commented Aug 18, 2022 •
edited
Loading

disable audio worker after grunt end

Can we disable audio worker after the grunt effect is completed?

If volume goes from 1 -> 0 in one frame won't this cause it to get stuck? That's probably an even worse effect than the bug it is fixing.

How about choose which one to emit when it's enabled, not just by chance when it's zero?

@avaer
Copy link
Contributor

avaer commented Aug 19, 2022

Can we disable audio worker after the grunt effect is completed?
No I don't think that's right, creating/destroying workers is a pretty heavy operation to run on every sound effect...

@tcm390
Copy link
Contributor Author

tcm390 commented Aug 19, 2022 •
edited
Loading

Can we disable audio worker after the grunt effect is completed?
No I don't think that's right, creating/destroying workers is a pretty heavy operation to run on every sound effect...

disable audioWorker to set the volume when isGrunting is false

Got it. Is it okay to use isGrunting in avatar.js to decide whether audioWorker is allowed to set volume or not?

@avaer
Copy link
Contributor

avaer commented Aug 20, 2022

Got it. Is it okay to use isGrunting in avatar.js to decide whether audioWorker is allowed to set volume or not?

Probably not, because I think multiple grunts can happen at a time. At the very least I think this would need to be a stack number, not a boolean.

And if this is related to manuallySetMouth, shouldn't both be in the same class? Currently it's spread around so the code is confusing to read.

@tcm390
Copy link
Contributor Author

tcm390 commented Aug 21, 2022

And if this is related to manuallySetMouth, shouldn't both be in the same class? Currently it's spread around so the code is confusing to read.

disable audioworker set volume after grunting

Got it. Renamed characterBehavior.manuallySetMouth to characterBehavior.disableAudioWorkerSetVolume. And used characterBehavior.disableAudioWorkerSetVolume to disable audioWorker after emitting grunt.

avaer
avaer reviewed Aug 23, 2022
View reviewed changes
avatars/avatars.js Outdated
@@ -993,7 +993,7 @@ class Avatar {
this.lastNeedsHeadTarget = false;
this.lastHeadTargetTime = -Infinity;

this.manuallySetMouth=false;
this.disableAudioWorkerSetVolume=false;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not good practice to have double negatives in variable names. The variable name should be the positive version, enable.

avaer
avaer reviewed Aug 23, 2022
View reviewed changes
avatars/avatars.js Outdated
@@ -993,7 +993,7 @@ class Avatar {
this.lastNeedsHeadTarget = false;
this.lastHeadTargetTime = -Infinity;

this.manuallySetMouth=false;
this.disableAudioWorkerSetVolume=false;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default seems to be true when the grunt ends... so why is this false. Probably a consequence of choosing a poor variable name.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it. set disableAudioWorkerSetVolume initial value to true

@avaer
Copy link
Contributor

avaer commented Aug 28, 2022

It looks like this is still using the disable name: https://github.com/webaverse/app/pull/3651/files#diff-fcb2093ac11d9b04a179d3dedbef5c0aae4d14b7f71db90e483becca4170b7ccR2041

@tcm390
Copy link
Contributor Author

tcm390 commented Aug 28, 2022

It looks like this is still using the disable name: https://github.com/webaverse/app/pull/3651/files#diff-fcb2093ac11d9b04a179d3dedbef5c0aae4d14b7f71db90e483becca4170b7ccR2041

Sorry Changed it.
change disableAudioWorkerSetVolume to enableAudioWorkerSetVolume

@tcm390 tcm390 closed this Dec 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@avaer avaer avaer requested changes

Assignees
No one assigned
Labels
passed qa
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@tcm390 @avaer