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

Hand control is not paused when a dialog is open #509

Closed
KatieWoe opened this issue Sep 6, 2022 · 6 comments
Closed

Hand control is not paused when a dialog is open #509

KatieWoe opened this issue Sep 6, 2022 · 6 comments

Comments

@KatieWoe
Copy link
Contributor

KatieWoe commented Sep 6, 2022

Test device
Dell
Operating System
Win 11
Browser
Chrome
Problem description
For phetsims/qa#831.
When a dialog, such as the preferences menu, is open, typically the controls to the sim behind it pause. With the hand controls, this is not the case. This can lead to some odd seeming behavior. In particular, I had voicing for the sim occur as I was trying to change the voice type, since my hands were briefly on screen.
Steps to reproduce

  1. Go to the sim with hand controls enabled, and enter either screen.
  2. Open the preferences dialog
  3. Move hands around

Visuals
stillcontrol

Troubleshooting information:

!!!!! DO NOT EDIT !!!!!
Name: ‪Ratio and Proportion‬
URL: https://phet-dev.colorado.edu/html/ratio-and-proportion/1.2.0-dev.31/phet/ratio-and-proportion_all_phet.html?cameraInput=hands&showVideo
Version: 1.2.0-dev.31 2022-08-24 16:50:40 UTC
Features missing: applicationcache, applicationcache, touch
Flags: pixelRatioScaling
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/104.0.0.0 Safari/537.36
Language: en-US
Window: 1280x649
Pixel Ratio: 1.5/1
WebGL: WebGL 1.0 (OpenGL ES 2.0 Chromium)
GLSL: WebGL GLSL ES 1.0 (OpenGL ES GLSL ES 1.0 Chromium)
Vendor: WebKit (WebKit WebGL)
Vertex: attribs: 16 varying: 30 uniform: 4096
Texture: size: 16384 imageUnits: 16 (vertex: 16, combined: 32)
Max viewport: 32767x32767
OES_texture_float: true
Dependencies JSON: {}

@KatieWoe KatieWoe added the type:bug Something isn't working label Sep 6, 2022
@zepumph
Copy link
Member

zepumph commented Sep 9, 2022

It would be nice to brainstorm some suggestions about how to hand this generally. Perhaps with @jessegreenberg.

@jessegreenberg
Copy link
Contributor

I think it is OK that hands continue to control the sim while the dialog is open - sims generally continue to animate while dialogs are open. If we decide to change this I think that will be a larger decision for PhET.

I had voicing for the sim occur as I was trying to change the voice type, since my hands were briefly on screen.

I expected sim voicing to stop while the dialog is opened, maybe there is a loose end in joist about this. If this part is fixed that might be enough for this issue.

@zepumph
Copy link
Member

zepumph commented Oct 5, 2022

I thought that this code would ensure that dialogs silence screen view voicing, but perhaps we need to check on that.

// So that this Utterance does not announce unless the ScreenView is visible and voicingVisible.
Voicing.registerUtteranceToNode( mediaPipeVoicingEndDragUtterance, this );
Voicing.registerUtteranceToNode( mediaPipeVoicingDragUtterance, this );

I'll investigate.

@zepumph
Copy link
Member

zepumph commented Oct 5, 2022

This patch works, but I believe we should discuss before I commit. @jessegreenberg, we were using voicingVisible for toggling "sim voicing", but not with dialogs.

Seems potentially like we forgot to get here in phetsims/scenery#1300 even though that was part of that work (not really sure, I can't remember). Anyways! What do you think about this? It should apply to all modal entities, even though we just have dialogs at this point.

Index: js/Sim.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/Sim.ts b/js/Sim.ts
--- a/js/Sim.ts	(revision 0f2668684a76091bc771b646ff70faf66643f965)
+++ b/js/Sim.ts	(date 1664991281019)
@@ -820,6 +820,7 @@
     if ( isModal ) {
       this.rootNode.interruptSubtreeInput();
       this.modalNodeStack.push( popup );
+      this.setNonModelVoicingVisible( false );
     }
     if ( popup.layout ) {
       popup.layout( this.screenBoundsProperty.value! );
@@ -836,6 +837,7 @@
     assert && assert( popup && this.modalNodeStack.includes( popup ) );
     assert && assert( this.topLayer.hasChild( popup ), 'popup was not shown' );
     if ( isModal ) {
+      this.setNonModelVoicingVisible( true );
       this.modalNodeStack.remove( popup );
     }
     this.topLayer.removeChild( popup );
@@ -1041,13 +1043,21 @@
    * only Toolbar content is announced.
    */
   public setSimVoicingVisible( visible: boolean ): void {
+    this.setNonModelVoicingVisible( visible );
+    this.topLayer && this.topLayer.setVoicingVisible( visible );
+  }
+
+  /**
+   * Sets voicingVisible on all elements "behind" the modal node stack. In this case, voicing should not work for those
+   * components when set to false.
+   * @param visible
+   */
+  public setNonModelVoicingVisible( visible: boolean ): void {
     for ( let i = 0; i < this.screens.length; i++ ) {
-      this.screens[ i ].view.voicingVisible = visible;
+      this.screens[ i ].view.voicingVisible = visible; // home screen is the first item, if created
     }
 
     this.navigationBar.voicingVisible = visible;
-    this.topLayer && this.topLayer.setVoicingVisible( visible );
-    this.homeScreen && this.homeScreen.view.setVoicingVisible( visible );
   }
 }
 

Also, if this works, do we need to do this also for the phet menu or combo box list being open?

@zepumph
Copy link
Member

zepumph commented Oct 7, 2022

@KatieWoe, we have fixed this now. Please confirm on master and let me know how it goes!

@KatieWoe
Copy link
Contributor Author

KatieWoe commented Oct 7, 2022

Voicing playing when moving hands while the dialog is open no longer occurs on master. Movement itself does still happen. If this is the desired behavior I believe this is fixed.

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

3 participants