phetsims / friction

"Friction" is an educational simulation in HTML5, by PhET Interactive Simulations.
http://phet.colorado.edu/en/simulation/friction
GNU General Public License v3.0
4 stars 6 forks source link

Voicing on/off utterance sometimes repeated in State wrapper #284

Closed Nancy-Salpepi closed 1 year ago

Nancy-Salpepi commented 2 years ago

Test device MacBook Air (m1 chip)

Operating System 12.3

Browser Safari and Chrome

Problem description For https://github.com/phetsims/qa/issues/791 in the State wrapper--When I set state and then turn the Voicing Feature on/off in the downstream sim--it will repeat the "Voicing On/Off" utterance the number of times that matches the number of times I pressed 'Set State'. So, if it was the second time I am setting the state, it will say "Voicing On" twice, if it was the third time then it will repeat it 3 times.

That seemed to be the only thing that was repeated.

Steps to reproduce This is how I have been able to consistently reproduce this problem:

  1. In the State wrapper, set state value = 0
  2. In the upstream sim, move the zoomed in book and then press 'Set State Now'
  3. In the downstream sim, turn voicing on (it will say it once) and then turn it off.
  4. In the upstream sim, move the zoomed in book and then press 'Set State Now'
  5. In the downstream sim, turn voicing on and then turn it off (it will be repeated twice)

Visuals

https://user-images.githubusercontent.com/87318828/161133845-e96db9ff-f261-4b79-9681-34611a2bc096.mov

Nancy-Salpepi commented 2 years ago

also seen with Win10 + Firefox

zepumph commented 2 years ago

When I reproduce these steps, I get an assertion:

Assertion failed: must be connected to a display to use UtteranceQueue features

It looks like the VoicingPanelSection is not a connected Node at this time. Thanks for the bug report. I'll investigate.

zepumph commented 2 years ago

Simple case to reproduce what I see:

zepumph commented 2 years ago

Ahha! I know the problem. We aren't disposing much of the voicing code, so when setting state, which destroys the old preferences dialog, it still keeps listeners linked to the audioModel Properties that will trigger voicing/description when those Properties changes. This explains why you heard voicing repeat (once for each dialog that had been created each time state was set), and why I had an assertion hit first, since description failed to trigger on the dangling dialog (since it was disposed and no longer connected).

The fix is just to fully dispose the voicing section Node.

I would like @jessegreenberg to review this, so that he can be aware how finicky PhET-iO is here. Sorry about this, PhetioCapsule definitely adds a fair bit of complexity here. I wonder if there are other models that we will want to clean this up with. I'll look at other audio features and in the input tab too to see if there is trouble.

zepumph commented 2 years ago

I looked through all usages of link in the preferences code and didn't see any other listeners conducting voicing/description or other outside listening that were not getting unlinked in dispose calls. Over to you @jessegreenberg.

zepumph commented 2 years ago

Ping here as we move towards a dev version in the next few days.

jessegreenberg commented 1 year ago

Thank you for explaining in https://github.com/phetsims/friction/issues/284#issuecomment-1179401719, I understand. Disposal code looks good. I found several other places under joist/Preferences that I verified were leaking memory in the state wrapper (Usages of soundManager, Multilinks, other Properties that were not owned by the file). They were patched in https://github.com/phetsims/joist/commit/23b9d634f1615a5df0958dea0f6a0bc37b54eb4d.

I think this is ready to close.