Closed zepumph closed 2 years ago
We found that there is a sarafi workaround built into the async queue in voicingManager at this time, bug report in https://github.com/phetsims/john-travoltage/issues/435. @jessegreenberg and I think we can fix this by setting the announcer.isReady flag to false for some time after a cancel()
call.
@jessegreenberg, please let me know if I can assist here more, and bring this up at our Friday meeting if you'd like.
This was done in the above commits and voicingManager no longer has its own queue. The original plan in https://github.com/phetsims/utterance-queue/issues/37#issue-1031574580 worked very well.
The hardest part of this change was giving voicingManager (an Announcer) full control over the UtteranceQueue queue
, which was necessary because voicingManager had full control over its queue to add/remove/prioritize/clear.
Summarizing the changes while they are fresh: UtteranceQueue/Announcer:
readyToSpeak
was added to Announcer. utteranceQueue
checks this before moving to the next item in the queue.clearEmitter
was added to Announcer. This is emitted from the Announcer and the utteranceQueue
clears itself in response to this. This replaces the voicingManager
clearing its internal queue.prioritizeUtterances
is an abstract method that was added to Announcer. This is called by utteranceQueue
on addToBack
. It takes the added utterance and the utteranceQueue
's queue. It uses the original priority system of voicingManager to organize the utterances and modify the queue. This is probably the weirdest part of this change, but it allows the voicingManager to prioritize the utteranceQueue instead of its internal queue.step
is an abstract method that was added to Announcer. Similar to the above point, it lets us do the timing portions that are specific to the Announcer but depend on the queue
of the UtteranceQueue.voicingManager:
timeSinceUtteranceEnd
in step
.removeFromQueue
now modifies the actual queue of the UtteranceQueue. As mentioned above we have access to the queue from prioritizeUtterances
.alertNow
now uses speech synthesis synchronously, without waiting for minTimeInQueue
.onSpeechSynthesisUtteranceEnd
was removed. Instead of using the SpeechSynthesisUtterance end
event to trigger the next speech, we use the step function to set poll and set the new readyToSpeak
property when it is time. I am OK with this because the end
event has historically been inconsistent on some platforms anyway.I tested several sims with Voicing, including Friction which uses the voicingManager priority system a lot, comparing them to versions before the change and they sounded the same. I also did some dedicated testing queing up Utterances with the console using combinations of cancelSelf
, cancelOther
, and priority
and everything was as I expect. FInally, unit tests in utterance-queue and scenery are passing.
@zepumph can we review this list of changes together tomorrow during meeting?
readyToSpeak
was added to Announcer.utteranceQueue
checks this before moving to the next item in the queue.
announceImmediately
will completely lose an utterance if the Announcer is not ready to speak. Do we want different behavior for that function instead of a Noop?
Looking forward to more conversation.
One thing we talked about together today was that if we are using polling more anyway to decide when readyToSpeak
can be set to true
, why not use the step function entirely to replace start
and end
event listeners on the SpeechSynthesisUtterance. These events are buggy on multiple platforms and we have workarounds in place to catch when they fail. We could go all in on the step function and have a more consistent and simple implementation.
If we do this we could get rid of VOICING_UTTERANCE_INTERVAL
, we can get rid of safariWorkaroundUtterancePairs
, because those are only needed to get the start
and end
events working better.
As for https://github.com/phetsims/utterance-queue/issues/37#issuecomment-967291483, we decided to implement a behavior where attemptToAnnounce will add an Utterance to the back of the queue if the Announcer is not ready. attemptToAnnounce does not prioritize the Utterances with the Announcer or collapse the same Utterances, but Utterances added in this way are read in last-in-first-out order. We did some testing of this function by adding Utterances to the queue in varying combinations and everything came out as we expect.
Some more things we discussed:
cancelEmitter
allows bi-directional communication between Announcer and UtteranceQueue. Can we replace the cancelEmitter
with calls to cancel the UtteranceQueue instead? It seems very possible if the UtteranceQueue can also cancel anything being spoken by the Announcer. Ill give this a try.alertNow
can be inlined into requestSpeech
.cancelSelf
and cancelOther
? Not necessarily an action item here, but something to keep in mind.I investigated what it would look like to replace the cancelEmitter
with cancelling the UtteranceQueue instead, and the following patch is what I came up with. I added an abstract cancel
to Announcer which cancels the synth in voicingManager. This way utteranceQueue.clear()
also cancels any ongoing speech. The hardest thing to replace is the boundHandleCanSpeakChange
in voicingManager.initialize
, and the best I could come up with are the changes shown in audioManager
.
I think clearEmitter
is more consolidated personally, but I also don't feel too strongly, we can review this together next we discuss.
Up to you. I appreciate the investigation and totally trust whatever direction you would like to take this.
The improvement discussed in UtteranceQueue should ask if announcer is ready for next utterance #37 (comment)
Made progress on this that I will need to return to. It is working well but I am hitting an assertion in requestSpeech
for "We should never request speech while we are already speaking" and need to figure out why.
I am unsure if this issue is on the list, or just out of date at this point. @jessegreenberg, can you reflect on the progress of this issue? Haven't we completed it in other issues?
For example from https://github.com/phetsims/utterance-queue/issues/37#issuecomment-967508508, we don't have the cancelEmitter anymore.
I read through comments in this issue looking for loose ends.
https://github.com/phetsims/utterance-queue/issues/37#issuecomment-967439694
We tried this today in https://github.com/phetsims/scenery/issues/1344 but it ended up being difficult and we decided it was not worth the effort/destabilizing at this time.
For example from #37 (comment), we don't have the cancelEmitter anymore.
Indeed!
You are right, I don't think there is anything else. The remaining work for this issue happened while we worked on making priorityProperty observable (among other things). Im going to close.
From https://github.com/phetsims/scenery/issues/1288, @jessegreenberg and I don't like that there is duplicated code with a queue in utterancequeue and also in voicingManager. These can be combined. The way to do this is just to ask the announcer before announcing. AriaLiveAnnouncer will always say yes, but voicingManager will want to wait until the current element in the synth is done.
We came about this from https://github.com/phetsims/scenery/issues/1300, because that bug will be substantially easier to manage after we have done this refactor.
Scrap notes: