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

Shared Announcers get stepped twice #71

Closed
jessegreenberg opened this issue Apr 1, 2022 · 6 comments
Closed

Shared Announcers get stepped twice #71

jessegreenberg opened this issue Apr 1, 2022 · 6 comments
Assignees
Labels

Comments

@jessegreenberg
Copy link
Contributor

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 jessegreenberg self-assigned this Apr 1, 2022
@jessegreenberg jessegreenberg added type:bug Something isn't working priority:2-high labels Apr 1, 2022
@jessegreenberg
Copy link
Contributor Author

This seems important to resolve.

@zepumph
Copy link
Member

zepumph commented Apr 1, 2022

@jessegreenberg and I decided. . . .

  • Remove the queue.length check
  • Remove the need for the queue in step (in the Announcer interface too)
  • run step via its own stepTimer listener. Wahoo!

@jessegreenberg
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Member

zepumph commented Apr 7, 2022

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?

@zepumph zepumph assigned jessegreenberg and unassigned zepumph Apr 7, 2022
@jessegreenberg
Copy link
Contributor Author

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!

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

No branches or pull requests

2 participants