Handle default sounds for DragListener and KeyboardDragListener in scenery

pixelzoom commented 11 months ago

Over in https://github.com/phetsims/graphing-lines/issues/137#issuecomment-1861693224, @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 commented 11 months ago

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

jbphet commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

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

pixelzoom commented 11 months ago

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 https://github.com/phetsims/scenery-phet/issues/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 https://github.com/phetsims/scenery/issues/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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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

AgustinVallejo commented 10 months ago

Began working on this, but got blocked by a number of things, listing them here and tagging as help-wanted:

Tried but got stuck to merge the default start functions with the ones that come in providedOptions... It randomly says start() needs arguments, but when declaring the function in DragListenerOptions, it doesn't ask for it.
Somewhere in Kepler's Laws we use associatedViewNode on the soundGenerator, not really sure why or if we should consider it for this implementation.
There are multiple edge cases of this pattern, similar enough to require investigation but different enough not to allow our RichDragListener to be even tried.

In the end, I'm not even sure if pushing through with this would be even worth it, as the instances of 1:1 repeated code are minor (less than 3), whereas many of them differ sufficiently to justify the repeated pattern, but open for dicsussion.

zepumph commented 9 months ago

While working on https://github.com/phetsims/area-model-multiplication/issues/9, I was surprised to see that the shared sound player for grabbing is using grab2_mp3, but RichDragListener uses grab_mp3. Why the discrepancy? I think we should probably be consistent. @jbphet can you speak to if these two grab sounds should be the same?

jbphet commented 9 months ago

These have been around for a few years and I don't recall their origin stories at this point. The GitHub history isn't that helpful either. So I can't directly answer the question of "why the discrepancy", but I just listened to both and I think grab2 sounds better. I would recommend that we use grab2 everywhere and delete grab. Feel free to double check that with @Ashton-Morris if you think it is necessary.

If you'd like me to do the consolidation, let me know and I will.

zepumph commented 9 months ago

Thanks. That sounds great. Over in https://github.com/phetsims/tambo/issues/185 I'll work on removing grab_mp3 and most likely renaming grab2_mp3.

zepumph commented 9 months ago

Over in https://github.com/phetsims/area-model-multiplication/issues/9 I ran into a bug trying to use RichDragListener because it doesn't yet have the code that @AgustinVallejo and I worked on to overwrite the start/end callbacks when they are provided. I thought there may be a patch to continue on, but I don't see one above so I'll see what I can do.

Working on this now.

zepumph commented 9 months ago

Ok. I made enough commits to proceed with https://github.com/phetsims/area-model-multiplication/issues/9, but I know that much of what @AgustinVallejo and I want to get done has to do with other usages which I didn't touch. @AgustinVallejo I'll go ahead and see if the above commits unblocked you enough to keep going. I didn't include the soundGeneratorOptions that we were working on last time, but I think we found a good path forward for that. Please review my changes and let me know if you run into trouble.

pixelzoom commented 9 months ago

I'm having problems with this API in FEL. @AgustinVallejo let's discuss.

(1) Why was this integrated into sims without a code review?

(2) Entirely different code for RichDragListener vis RichKeyboardDragListener. I'd expect the implementations to be identical, since the DragListener and KeyboardDragListener APIs for wiring in sound are identical. Why are the implementations different?

(3) The doc for RichDragListener says:

RichDragListener extends DragListener to integrate PhET-specific designed features that should be broadly applied to DragListener instances in phetsims. This includes the grab and release sound clips.

The doc for RichKeyboardDragListener says:

RichKeyboardDragListener extends KeyboardDragListener to integrate the drag and drop sound clips.

So the motivations are different?

(4) If additional PhET-specific features are indeed added to RichDragListener (and hopefully RichKeyboardDragListener) and I want to substitute custom sounds for grab/release, but use the other PhET-specific features? There's no way to do that. I expected to see these options, with defaults:

grabSound: WrappedAudioBuffer;
releaseSound: WrappedAudioBuffer;

(5) Why are the APIs different for RichDragListener vs RichKeyboardDragListener? For example, RichKeyboardDragListener does not have option addGrabReleaseSounds.

(6) We have dragClipOptions, with no way to (for example) set independent output levels for grab vs release. Is that OK?

(7) If I provide options start or end to RichKeyboardDragListener, no sound will play. It blows away the code that wires the sounds into KeyboardDragListener, instead of wrapping the start and end functions as RichDragListener.

(8) Where is the test harness for this in sun?

(9) No dispose and no isDisposable: false.

AgustinVallejo commented 9 months ago

Hi @pixelzoom, sorry for the inconveniences.

I think the only problem was pushing changes to FEL. This feature is currently under development by @zepumph and I, and as you noted, there's much left for the implementation to be clean and ready for use.

We just don't have it in high priorities so progress is slow.

Will undo the push in FEL shortly

pixelzoom commented 9 months ago

Will undo the push in FEL shortly

No need. I'm working on RichDragListener and RichKeyboardDragListener.

pixelzoom commented 9 months ago

@AgustinVallejo and I paired on this today. RichDragListener vis RichKeyboardDragListener are now in much better shape, and I'm unblocked for FEL.

There are TODOs in the code for remaining work. The most important one is that RichDragListener.ts and RichKeyboardDragListener.ts are now identifical implementations, which is unforuntate and undesirable.

pixelzoom commented 9 months ago

One more thing... RichDragListener and RichKeyboardDragListener currently live in sun, which is a repo for general UI components, not PhET-specific. Since the documented purpose of these new classes is "to integrate PhET-specific designed features", they should be moved to scenery-phet, which is the repo for PhET-specific components.

jonathanolson commented 9 months ago

@Luisav1 noted a memory leak in AMM/AMA, and after a bit I tracked it down to RichDragListener not ever removing sound generators. Fixed above.

I see this was noted in https://github.com/phetsims/scenery-phet/issues/849, seems good to make other improvements soon.

pixelzoom commented 9 months ago

Problems with https://github.com/phetsims/sun/commit/c33c9b9267aa00bd07db9b02dcf7b5562f2624ba:

You changed RichDragListener, but did not change RichKeybordDragListener. So less than 1 hour after I addressed the divergence in implementations, the divergence has been re-introduced.
You left isDisposable: false in the default options for RichDragListener, so it will presumably fail when you call dispose.

I fixed these problems in https://github.com/phetsims/sun/commit/55f1190c795a3962c66558e5bad3d1d0ae84d95a. I also fixed a major problem in RichDragListener, where the clips were wired into start and end after calling super( options ).

@jonathanolson please review and verify that the leak in AMM/AMA is still addressed.

jonathanolson commented 9 months ago

Looks good to me.

pixelzoom commented 9 months ago

@AgustinVallejo is leading further work on this, so unassigning myself. Let me know if you'd like feedback or assistance.

AgustinVallejo commented 9 months ago

TODOs after talking with @zepumph

[x] Move this to scenery-phet with perennial/bin/copy-history-to-different-repo.sh because the drag listeners in Sun are not good candidates for this, they already have their own sound implemented.
[x] Create test harnesses

Also, we're not going to merge the implementation of the two RichDragListeners since DragListener and KeyboardDragListener are by nature really different and that's a bigger problem, which we're not solving in this issue.

pixelzoom commented 8 months ago

I'm getting worried about how these classes are proliferating, and the fact that I've now needed to add them to multiple sims. At the very least, the code should be moved from sun to scenery-phet. Adding this to the developer meeting agenda.

zepumph commented 8 months ago

Hello, and we are in agreement here. We have not been communicating well in this issue at all. I feel this issue is on hold at the moment. @jessegreenberg has been getting more time allocated to him about alt input issues, and https://github.com/phetsims/scenery/issues/1614 is on his list for this iteration. I'd be interested in seeing if that would change our approach for this issue, since we would likely only have a single instance of this that handled keyboard and mouse/touch. Perhaps let's hold off on discussion unless there is something specific to discuss.

zepumph commented 8 months ago

Discussed during dev meeting today. It would be good to finish this up as one step and then having the combining step as a separate task. @AgustinVallejo and I will take it on! Note that the API likely doesn't support all current usages of grab/release sounds, so if you are using this for non-simple cases, you may need to talk with us.

zepumph commented 8 months ago

TODO:

[x] review all items in https://github.com/phetsims/scenery-phet/issues/849
[x] Move to scenery phet (https://github.com/phetsims/scenery-phet/issues/849)
[x] Test in demos (https://github.com/phetsims/scenery-phet/issues/849)
[x] Self code review

zepumph commented 8 months ago

Looking into other usages of grab/release playing:

BASE/BalloonNode.js: Not a good candidate because drag/keyboard-drag share a release sound, and grab happens on GrabDragInteraction.onGrab.
We converted Graphing Lines and it went well. As part of this we were able to simplify the sim-specific code, and also then were able to build out the parametric implementation for RichDragListener. At this time KeyboardDragListener isn't parameterized to need the type of the instance in the start/end callbacks, so we didn't do this work in RichKeyboardDragListener (perhaps a paper trail in https://github.com/phetsims/scenery/issues/1238).
Density and Buoyancy: The grabbing logic is handled via Three.js and not using a normal DragListener. There's also a case where we use KeyboardDragListener but we're making the sound on focus() instead of start(), also not a good candidate.

TODO:

[x] TODOs in the code for this issue
[x] Usages of grab\w+\.play\(\) will help us find the usages that may want to be converted to this class. The main purpose of converting is to make sure that the Rich(Keyboard)DragListener API is supportive to the kinds of usages we will have in the future (less about being perfectly converted).
[ ] It will likely take a discussion with a couple other member of the team to get this item closed out. Questions for meeting:
- [ ] What about the shared sound player called grabSoundPlayer() https://github.com/phetsims/scenery-phet/issues/849
- [x] Can we take another look at the API, is this the best for our cases? I know it is flexible, but more times we don't need custom options to pass to grab and release separately. https://github.com/phetsims/solar-system-common/commit/eeec48e9518678059bd80fd05af0dd9c0b9b6aa7#diff-3a1af2bd927e954fa155eddff3653dc12a3d4a95e9209e8b509ee54e8df7b219R176-R194.
- [ ] The API doesn't support passing in a soundclip. i.e. RatioHalf.ts. I'm not sure we should support this, but just wanted to note it.
- [ ] Sometimes the sound can be on focus/blur instead of start/end. Should we support that?
- [ ] RichKeyboardDragListener and RichDragListener.js are identical except for the string 'Keyboard' (likely fixed by https://github.com/phetsims/scenery/issues/1614).
[x] Items in https://github.com/phetsims/scenery-phet/issues/849
[x] soundManagerOptions: { categoryName: 'user-interface' } are used by grabSoundPlayer. Should we have this in our defaults?
[x] Check the usages of MeasuringTapeNode for duplicate sounds
[x] Add text to the scenery-phet demo Nodes for these classes.

zepumph commented 8 months ago

@AgustinVallejo and I believe that we are done with the work for this issue. Next step is to have a reviewer look over the work. We recommend @jbphet take the review with an eye for how the tambo-related code is implemented.

Focus for reviewer:

Please look through the implementations of RichDragListener and RichKeyboardDragListener. How do you like the API, and do you think it is complete?
Please look into the usages we have so far. Not all grab/release sounds were converted, but enough that we feel that the API supports needed cases moving forward.

Questions for reviewer (to think about while working on the above)

What about the shared sound player called grabSoundPlayer() Handle default sounds for DragListener and KeyboardDragListener in scenery #849
The API doesn't support passing in a SoundClip. i.e. RatioHalf.ts. I'm not sure we should support this, but just wanted to note it.
Sometimes the sound can be on focus/blur instead of start/end. Should we support that? I believe we may get to a point where ~50% of keyboard drag listeners want that behavior, depending on the sim-specific context.
Note that RichKeyboardDragListener and RichDragListener.js are identical except for the string 'Keyboard' (likely fixed by Combine DragListener and KeyboardDragListener into a single class scenery#1614).

pixelzoom commented 7 months ago

These drag listeners are not behaving properly when they've been interrupted (eg by calling interruptSubtreeInput). They should not play releaseSound if the drag cycle was interrupted. See https://github.com/phetsims/graphing-lines/issues/156.

In the case of RichDragListener, I suspect that this patch will fix the problem. But I do not have a multitouch device availabe, so I cannot test.

Subject: [PATCH] interruptSubtreeInput when resetAllButton is pressed, https://github.com/phetsims/graphing-lines/issues/156
---
Index: js/RichDragListener.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/RichDragListener.ts b/js/RichDragListener.ts
--- a/js/RichDragListener.ts    (revision 69f356768c26e0d2af3ca23e272f118cc16f3ef2)
+++ b/js/RichDragListener.ts    (date 1713281208977)
@@ -87,7 +87,7 @@
       const previousEnd = options.end;
       options.end = ( ...args ) => {
         previousEnd && previousEnd( ...args );
-        releaseClip.play();
+        !this.interrupted && releaseClip.play();
       };
     }

In the case of RichKeyboardDragListener, I don't see how to address this. KeyboardDragListener has no interrrupted flag -- another problematic case where it's API differs from DragListener.

Back to @zepumph and @AgustinVallejo with high priority. Graphing Lines is in RC testing, and any change here will need to be cherry picked.

jbphet commented 7 months ago

Unassigning myself until the next review is needed.

zepumph commented 7 months ago

I believe first we should update the KeyboardDragListener API to support the interrupted flag. Then this will be a simple patch, as @pixelzoom mentioned. I'll touched base with @jessegreenberg about his current KeyboardDragListener work. I will just patch things in main, since Graphing Lines is already in RC (making for an easier cherry pick).

zepumph commented 7 months ago

Alright. @pixelzoom, can you please see if this fixed your problem for Graphing Lines.

To Cherry pick:

https://github.com/phetsims/scenery-phet/commit/1af656ae5c66725be4f8480df346bf4c6960446c https://github.com/phetsims/scenery/commit/5ee1b7c567172433daecaf1e90d4620601c3a4dd

pixelzoom commented 7 months ago

Alright. @pixelzoom, can you please see if this fixed your problem for Graphing Lines.

I don't have a multitouch device available for testing. So I've asked @Nancy-Salpepi to verify in https://github.com/phetsims/graphing-lines/issues/156#issuecomment-2064895893.

pixelzoom commented 7 months ago

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 https://github.com/phetsims/scenery-phet/issues/849#issuecomment-2021595684.

pixelzoom commented 6 months ago

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

jessegreenberg commented 6 months ago

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 commented 4 months ago

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.

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?
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?

phetsims / scenery-phet

Handle default sounds for DragListener and KeyboardDragListener in scenery #849