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

Voicing: Don't hear "Sim Voicing Off" bug #846

Closed
zepumph opened this issue Aug 26, 2022 · 9 comments
Closed

Voicing: Don't hear "Sim Voicing Off" bug #846

zepumph opened this issue Aug 26, 2022 · 9 comments

Comments

@zepumph
Copy link
Member

zepumph commented Aug 26, 2022

To reproduce:

  • Open a sim with voicing
  • Enable voicing
  • Press the "details" button with the mouse
  • Immediately press the sim voicing toggle switch.

Expected Behavior: details button is interrupted then you hear "sim voicing off"

Current Behavior: details button is interrupted

@jessegreenberg
Copy link
Contributor

jessegreenberg commented Aug 29, 2022

Confirmed. I am not hearing any content when interrupting a "Quick Info" button (for example, pressing Reset All).

@jessegreenberg
Copy link
Contributor

Caused by this line:

// clear the voicingUtteranceQueue because stale alerts may have collected while the quick info button announced
voicingUtteranceQueue.clear();

jessegreenberg added a commit that referenced this issue Aug 29, 2022
@jessegreenberg
Copy link
Contributor

We added this line because we don't want to hear stale Utterances that are sitting in the queue waiting for the toolbar button responses to finish. I think that is correct. But if we interrupt the the toolbar button responses we do want to hear the next response.

I pushed a proposed fix in the above commit and added a line of doc about where it falls short. But I think it should be acceptable. @zepumph can you please review and verify that this is fixed?

@zepumph
Copy link
Member Author

zepumph commented Aug 31, 2022

I am worried about the comment you said. What about in friction, the temperature continues to get lower, and alerts pile up without any interaction. I also think this is quite possible in Greenhouse/MAL because of background sim state being described. I think next steps here are to discuss with @jessegreenberg.

@jessegreenberg
Copy link
Contributor

Thinking about this more I think it is less of a shortcoming and more the correct behavior. I changed my doc language to describe why. I thought it was correct because

  1. The queue is still cleared when toolbar button content stops speaking.
  2. If there is some user input, we won't have a build-up of alerts. As soon as the user taps on the screen, the toolbar button content becomes lowest priority so anything waiting in the queue should be spoken immediately and interrupt toolbar button content.

I think generally the sim cases you mentioned should still work well. Still happy to discuss but back to @zepumph.

@jessegreenberg jessegreenberg removed their assignment Aug 31, 2022
@zepumph
Copy link
Member Author

zepumph commented Aug 31, 2022

@jessegreenberg and I discussed this, we are in agreement that the current implementation isn't ideal because if you interrupt a readme button by grabbing the book, it may say 10 "ambient" utterances waiting in the queue before saying "grabbed".

We discussed new priorities, and separating a priority for "AMBIENT" voicing vs "INTERACTIVE":

AMBIENT_HIGH
AMBIENT_LOW
README_AFTER
INTERACTIVE_HIGH
INTERACTIVE_LOW
README_START

But didn't love it.
We will keep thinking about it.

@jessegreenberg
Copy link
Contributor

jessegreenberg commented Aug 31, 2022

Ideas that are moderately to extremely yucky.

  • What if we clear the queue in a listener on the Pointer that presses the button?
  • What if we added a set of "pre-dispatch" input listeners to to display so that in Input.dispatchEvent we could
    // Notify input listeners before all others to handle global events before pointers
    this.dispatchToListeners<DOMEvent>( pointer, this.display.getPreDispatchInputListeners(), type, inputEvent );
  • Add a listener to the document with useCapture that would fire before scenery input event dispatch as a workaround.
  • Utterances have an ambient field separate from Priority. UtteranceQueue has a clearAmbientUtterances().
    • I actually don't mind this one too much. I am not excited about going through and inspecting every Utterance in the project to decide if it should be ambient.

@zepumph
Copy link
Member Author

zepumph commented Oct 5, 2022

Please note this issue is blocking the publication of Friction. I don't think @jessegreenberg and I have a direction forward just yet.

zepumph added a commit that referenced this issue Oct 7, 2022
This reverts commit 5f37d77.
zepumph added a commit that referenced this issue Oct 7, 2022
@zepumph
Copy link
Member Author

zepumph commented Oct 7, 2022

@jessegreenberg and I realized that more central/powerful fixes were overkill, so we just made a workaround in the voicing toolbar code. The bug is fixed, and life is good. Closing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants