Handle default sounds for DragListener and KeyboardDragListener in scenery

[pixelzoom]

Over in phetsims/graphing-lines#137 (comment), @amanda-phet and I discovered that DragListener and KeyboardDragListener apparently have an (unofficial?) default design. tambo has sounds to be used for "grab" and "release" interactions.

While it's a default sound design, it's not supported in common code. Sims must implement it, using some variation of this code:

import grab_mp3 from '../../../../tambo/sounds/grab_mp3.js';
import release_mp3 from '../../../../tambo/sounds/release_mp3.js';
...

// Create sound clips.
const dragClipOptions = {
  initialOutputLevel: ... // a value between 0 and 1
};
const grabClip = new SoundClip( grab_mp3, dragClipOptions );
const releaseClip = new SoundClip( release_mp3, dragClipOptions );

// Add the sound clips to the soundManager.
soundManager.addSoundGenerator( grabClip );
soundManager.addSoundGenerator( releaseClip );

// Wire the sounds up to DragListener interactions.
... new DragListener( {

   start: () => {
     grabClip.play();
     ...
   },
   ...
   end: () => {
     releaseClip.play();
     ...
   }
} );

// Wire the sounds up to KeyboardDragListener interactions.
... new KeyboardDragListener( {

   start: () => {
     grabClip.play();
     ...
   },
   ...
   end: () => {
     releaseClip.play();
     ...
   }
} );

There are also variations of the above code that use press: () => grabClip.play() and release: () => releaseClip.play().

This duplication is very undesirable. It's unnecessary work. It results in inconsistent implementation and inconsistent behavior (e.g output levels are all over the place). It's also easy to omit, if the developer and designer are not aware of this pattern (as I wasn't until a couple of hours ago.)

Let's investigate moving the default sound design (and the above code) into scenery, with options to specify an alternative sound design.

[pixelzoom]

@amanda-phet @jbphet Do you have anything to add here design-wise?

[jbphet]

I have two comments/suggestions:

• Default sound features have been added to a number of common code components over the last couple of years, so there should be a number of examples to follow. @pixelzoom was recently involved in fixing some problems with sound in ComboBox. I can't say off the top of my head which would be the best examples to follow, but searching in sun and scenery-phet for soundPlayer would help to locate several. I'd advise not using the buttons, since the complex inheritance hierarchy used there makes it a bit more complicated than most other examples.
• I've had it in the back of my head for a while now that the terms "sound player" and "sound generator" have evolved quite a bit since they first entered the PhET lexicon and their usage is probably not perfectly consistent in the code and documentation. Don't let that throw you and, if you run into code or docs where you think this could be improved, please let me know I'll set up an issue to work on improving it.

Also, I'd be happy to collaborate on this if it would be helpful.

[amanda-phet]

Design-wise, I'm in favor of using a default sound design for DragListener, and vote for the current grab and release sounds that are being copied into multiple sims.

[pixelzoom]

Now that this has been assigned back to me, I'm not sure how to proceed. I guess the process is to leave it unassigned (which seems wrong) and add this to the Developer Meeting agenda (done).

[jbphet]

Seems like a "hot potato" or "not it" sort of issue at the moment. Maybe put it on the list of potential tasks for an upcoming iteration. I'm happy to work on it if the decision makers want to make it a priority for me.

[zepumph]

@jonathanolson, can you comment on how you feel about adding a dependency on tambo in scenery?

[pixelzoom]

This was discussed extensively at 1/4/24 dev meeting. Here are the notes from https://docs.google.com/document/d/11Gt3Ulalj0fCD2fFeCjPT5ni_9mM2WjkGc4ysisQmo8/edit?pli=1:

CM: How to proceed with #849? I.e. DragListeners have to get the drag and drop soundClips manually added every time.
MK: Baking these sounds would require a scenery dependency on tambo.
CM: Some of us didn’t know about this de-facto convention.
JB: Let’s work on this.
MS: We should vote on priority
MK: Side question 🌟: How do devs handle emergent time? This issue fits in that assigned time.
SR: Does it make sense to include it in FEL time?
MS: I usually integrate emergent issues related to my work into my personal time.
MS: I don’t think any of the issues on the Dev Priorities board are doable in a single day, otherwise they’d be done.
MK: The PrB feels like it’s the place where things go to die.
MS: I want to push back on that idea. Really good progress has been made there. ✊
SR: Is this a small thing or not?
CM: It’s hard to estimate that, it involves many dependency and refactoring problems. There’s also the fact that we usually have to create DragListener AND KeyboardDragListener with slightly different APIs – see phetsims/scenery#1238 (which is not on the Dev Priorities board)
JB: We’d have to figure out if it’s okay for scenery to depend on tambo, along other big questions: Probably some initial discussion, planning and implementation.
MS: Maybe let’s wait for JO and bring this back. There is also the possibility of this depending on the complexity. The Region and Culture club could help out here.
Team Decision: We’ll set up a time with JO and JB to estimate the difficulty of it, and if the dependency on tambo is okay. (JO, JB, AV, MS, LV) We’ll set up a slack when this is done.

[AgustinVallejo]

We met on jan-08. Meeting notes:
JB: If we proceed on this, scenery would need to depend on SoundClip and the mp3s. It's something we've tried to avoid but let's hear what JO has to say. Maybe tweak the way we create DragListeners?
JO: No objections to including Tambo as a library, although it would be extra work on the start-up. However, DragListener is not supposed to be "Phet's Drag Listener" but a general tool that any other project could use.
JB: It'd be good to have a "SoundEnabledDragListener" that lives outside scenery. (JO +1)
AV: Where would that component live? Opening threads for each mentioned repo:

• AV: scenery-phet? JO: scenery-phet is a level up from sun, so sun wouldn't be able to use the sonified DragListener.
• JB: tambo? It makes sense to have this component a level up.
• MS: sun? JO: I expect Sun to already rely on tambo. Potentially a good place for this. Do we know the filesize of the sounds we want to include? JB: ~3kb.

JO: After looking at TSoundPlayer, I'm leaning on adding an equivalent type into scenery and not depend on Tambo. (It's just an object that has a play() and stop() methods). However, we'd still need Tambo for the default values.
JO: Alternatively, we could check for default sounds on the loading state.

JO: Do we want this also for PressListeners? Answer: No.
JO: I think a subtype of DragListener in Sun or scenery-phet with the defaults should be good. (JB +1, AV +1)
AV: I can get behind this! What about the name?
MS: This pattern is mostly used in drag N drop scenarios. GrabDragListener? We're making a poll.

[zepumph]

Was there conversation about converting most/all old and new usages to this new, phet-specific drag listener? If the motivation of this issue was, "I always forget to add UI sound to my draggable because I need to do it manually", isn't this the same problem shifted because you have to remember to use the new drag listener? Unless guarded by strong precedent or a lint rule I will most likely continue to accidentally use DragListener until this bites me one too many times.

[zepumph]

It seems like the motivation in the issue is based more on, "PhET's specific dragging implementation", so I wouldn't be surprised if we pack more sim-specific customization in this new class in the future. I would recommend a more general name than something with "Sound" in the class name.

[AgustinVallejo]

@zepumph it was mentioned that we're not interested in replacing all usages of DragListener, only the ones which use the above pattern.

[pixelzoom]

Cherry picking has been completed for graphing-lines 1.4 and graphing-slope-intercept 1.2.

Back to @jbphet to complete the review requested in #849 (comment).

[pixelzoom]

@jbphet ping. Let's get this reviewed and closed.

[jessegreenberg]

Note that RichDragListener has been renamed to RichPointerDragListener. This rename was discussed over slack and will allow RichDragListener to be a listener that combines RichPointerDragListener and RichKeyboardDragListener, supporting both forms of input with a single listener.

However, my rename lost git history so I am going to redo the scenery-phet commits.

[jbphet]

I'm sorry it's taken me so long to start reviewing this, but, well, higher priorities.

I've taken an initial look, and have some big questions before moving on to the next steps.

1. It looks like this has evolved a lot, and the classes that I should now review (since I believe I'm being asked to focus on the implementation for the default sounds) are SoundRichDragListener, SoundKeyboardDragListener, and SoundDragListener. True?
2. I started looking at SoundRichDragListener, and I can see that the approach for sounds is significantly different from the examples that I described how to find above. You're passing wrapped audio buffers and the options for both creating and adding SoundClip instances for the grab and release sounds. This makes for a somewhat 'wider' API than is used in, say, AccordionBox, where TSoundPlayers are provided for the expand and collapse sounds. Also, this approach means that a unique instance of the SoundClip will be created for each instance of SoundRichDragListener. And finally, it's just a different API than is used elsewhere for default sounds. Just to be clear, I'm not saying it's incorrect - it seems perfectly viable - it's just a break from precedent. Was there a reason for doing it differently?