phetsims / utterance-queue

Alerting library powered by aria-live
http://scenerystack.org/
MIT License
1 stars 2 forks source link

Shared Announcers get stepped twice #71

Closed jessegreenberg closed 2 years ago

jessegreenberg commented 2 years ago

If an Announcer is shared between two utteranceQueues it gets stepped multiple times per animation frame. The utteranceQueue steps the Announcer, so if the Announcer is assigned to two UtteranceQueues it will get stepped in each. This may impact timing variables in the step function.

I found this while working on #60 where I noticed that the code related to ENGINE_WAKE_INTERVAL was called twice as fast as I expected. The same is probably true for

timeSinceUtteranceEnd timeSincePendingUtterance timeSinceWakingEngine

If we fix this we should be careful to test each of these with since the will be called half as often, or half the timing constants associated with each.

jessegreenberg commented 2 years ago

This seems important to resolve.

zepumph commented 2 years ago

@jessegreenberg and I decided. . . .

jessegreenberg commented 2 years ago

Changes made in the above commits and its working. Unit tests are passing after the change. I am now seeing timing of things in the step function match their controlling timing variables.

So the remaining question is what to do about the timing constants since we are now effectively doubling the amount of time for each. There are three. I think ENGINE_WAKE_INTERVAL and VOICING_UTTERANCE_INTERVAL should be halved, these have lived for a long time and have always had this problem. I don't want to change PENDING_UTTERANCE_DELAY because it was added recently and tested in the context of number-play where there was only one utterance-queue in the sim.

jessegreenberg commented 2 years ago

Timing variable changes made in the above commit. I tested on a Chromebook and verified that the ENGINE_WAKE_INTERVAL workaround still works and the PENDING_UTTERANCE_DELAY still works. I tested on Safari and it seems that all end events are coming through which indicates that VOICING_UTTERANCE_INTERVAL workaround is working.

And unit tests with ?manualInput are passing.

I think this is ready for review. Is there anything else we should check @zepumph?

zepumph commented 2 years ago

Commits look great. I feel like I remember having a conversation with you last Friday about how it would be nice to not have this run/step when there is no voicing, and I see that it is all hidden behind initialize(). I like that!

Anything else?

jessegreenberg commented 2 years ago

I forgot about that, thanks. I think that is good for this issue, but I remember discussing that we shouldn't step if the Announcer is not enabled, maybe Ill make a new issue for that since it doesn't seem related.

Thanks for reviewing!