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

Add sound when jumping to first/last launched projectile #285

Closed
Tracked by #1079
Nancy-Salpepi opened this issue Apr 10, 2024 · 6 comments
Closed
Tracked by #1079

Add sound when jumping to first/last launched projectile #285

Nancy-Salpepi opened this issue Apr 10, 2024 · 6 comments

Comments

@Nancy-Salpepi
Copy link

For phetsims/qa#1068, @matthew-blackman and I both agree that a sound should be heard when using home and end keys to jump to the first and last projectile on the field sign.

@samreid
Copy link
Member

samreid commented Apr 11, 2024

I'll take a look at this one.

@samreid samreid removed the dev:help-wanted Extra attention is needed label Apr 11, 2024
@samreid
Copy link
Member

samreid commented Apr 11, 2024

Here is a working patch. I left debugging information in for the review. @matthew-blackman let's take a look together:

Subject: [PATCH] Increase memory available to QuickServer node processes, see https://github.com/phetsims/chipper/issues/1431
---
Index: js/common/view/SelectorNode.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/common/view/SelectorNode.ts b/js/common/view/SelectorNode.ts
--- a/js/common/view/SelectorNode.ts	(revision 346f6024a6d422430030bbc71d8d464e24064114)
+++ b/js/common/view/SelectorNode.ts	(date 1712849481029)
@@ -7,7 +7,7 @@
  * @author Sam Reid (PhET Interactive Simulations)
  */
 
-import { Color, Node, NodeOptions, Path, SceneryConstants } from '../../../../scenery/js/imports.js';
+import { Color, KeyboardListener, Node, NodeOptions, Path, SceneryConstants } from '../../../../scenery/js/imports.js';
 import AccessibleNumberSpinner, { AccessibleNumberSpinnerOptions } from '../../../../sun/js/accessibility/AccessibleNumberSpinner.js';
 import StrictOmit from '../../../../phet-core/js/types/StrictOmit.js';
 import TReadOnlyProperty from '../../../../axon/js/TReadOnlyProperty.js';
@@ -157,6 +157,28 @@
 
     // support for binder documentation, stripped out in builds and only runs when ?binder is specified
     assert && phet.chipper.queryParameters.binder && InstanceRegistry.registerDataURL( 'sun', 'SelectorNode', this );
+
+    let priorValue: number | null = null;
+
+    assert && assert( this.startInput === _.noop, 'start input should be a no-op before we change it' );
+    this.startInput = event => {
+
+      // When input begins, record what the value was before it changes so that when we process a home/end event,
+      // we do not play duplicate sounds if it was already at the home/end
+      priorValue = numberProperty.value;
+    };
+
+    // Add sound effects for pressing the home/end keys. Note this callback is called after the number property was updated.
+    // Do not use this.onInput since this would double some of the sounds when the buttons are interacted with in a certain way.
+    this.addInputListener( new KeyboardListener( {
+      keys: [ 'home', 'end' ] as const,
+      callback: () => {
+        console.log( 'home/end' );
+        if ( numberProperty.value !== priorValue ) {
+          options.playSound( numberProperty.value );
+        }
+      }
+    } ) );
   }
 }
 
Index: js/common/model/ProjectileSound.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/common/model/ProjectileSound.ts b/js/common/model/ProjectileSound.ts
--- a/js/common/model/ProjectileSound.ts	(revision 346f6024a6d422430030bbc71d8d464e24064114)
+++ b/js/common/model/ProjectileSound.ts	(date 1712846026046)
@@ -52,6 +52,7 @@
 
   public static play( projectileType: ProjectileType, x: number, isLanding: boolean ): void {
 
+    console.log('soundplay');
     const config = projectileSoundsConfig.get( projectileType )!;
 
     assert && assert( config, `Projectile type configuration not found for ${projectileType.phetioID}` );

@matthew-blackman
Copy link
Contributor

This should be fixed on main. Nice work @samreid!

@Nancy-Salpepi
Copy link
Author

Yes! Sounds good 👂🏻on main.

@Nancy-Salpepi Nancy-Salpepi removed their assignment Apr 11, 2024
@matthew-blackman matthew-blackman removed their assignment Apr 16, 2024
matthew-blackman added a commit that referenced this issue Apr 22, 2024
matthew-blackman added a commit that referenced this issue Apr 22, 2024
(cherry picked from commit 882cb6e)
matthew-blackman added a commit that referenced this issue Apr 22, 2024
(cherry picked from commit bad1e0c)
matthew-blackman added a commit that referenced this issue Apr 22, 2024
(cherry picked from commit f6284ae)
@matthew-blackman
Copy link
Contributor

Please close after verifying.

@KatieWoe
Copy link

Sounds good in rc.2

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

4 participants