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

Problems with categoryName option to SoundManager #132

Open
pixelzoom opened this issue Feb 10, 2021 · 3 comments
Open

Problems with categoryName option to SoundManager #132

pixelzoom opened this issue Feb 10, 2021 · 3 comments
Assignees

Comments

@pixelzoom
Copy link
Contributor

pixelzoom commented Feb 10, 2021

SoundManager supports the ability to change output level by "category" via this option:

      // {string} - category name for this sound, which can be used to group sounds together an control them as a group
      categoryName: null

Problems:

(1) The doc is incomplete. It says nothing about the value. Is there a set of valid values? Is there a set of values that's in common use? (I've discovered 'user-interface', how do I discover others? Where are their semantics documented?) Apparently you can create new categories by using new strings (which is a problem if you misspelled the string), but the documentation doesn't say that - it just leaves you wondering.

(2) 'user-interface' is duplicated 32 times in the code base. Spell it wrong and there's nothing to help you, SoundManager will merrily create a new category with the misspelled name. (Possible the cause of phetsims/joist#680, but I haven't investigated.)

(3) The value should be an enumeration, not a string. Values can be documented to indicate where/how they are typically used. If you need a new category, add an enumeration value. And SoundManager should verify that options.categoryName is included in the enumeration.

So recommendations:

  1. Create a SoundCategory enumeration.
  2. Convert strings like 'user-interface' to SoundCategory.USER_INTERFACE
  3. Document the values - what they mean, where they should be used
  4. Rename categoryName to soundCategory
  5. Add validation in SoundManager - assert( SoundCategory.includes( options.soundCategory )
  6. Convert SoundManager gainNodesForCategories to a Map, so gain can be looked up by SoundCategory value

EDIT (@jbphet): Made the list numbered instead of bulleted for easier reference in subsequent comments.

@jbphet
Copy link
Contributor

jbphet commented Jul 27, 2021

These all seem like good suggestions, but I want to be careful to continue to support one use case that I'm not sure is being considered. We want the client to be able to create and use their own categories.

How about I do most of what is described above, but instead of validating the categories as described item 5 above, the sound manager keeps a list of categories in an array, and we validate against that list. I'll add a method called addSoundCategory, and the user has to use it and name the category before being allowed to add sounds with that category. I'd still use the enumeration, but the string value of the enumeration would be in the list of valid sound categories.

Back to @pixelzoom for his response. We can set up a time and discuss if that would be easier.

@jbphet jbphet assigned pixelzoom and unassigned jbphet Jul 27, 2021
@pixelzoom
Copy link
Contributor Author

We want the client to be able to create and use their own categories.

Which client is this? PhET-iO? Something else?

I'll add a method called addSoundCategory, and the user has to use it and name the category before being allowed to add sounds with that category. I'd still use the enumeration, but the string value of the enumeration would be in the list of valid sound categories.

That sounds reasonable for the standard set of categories.

@pixelzoom pixelzoom assigned jbphet and unassigned pixelzoom Jul 28, 2021
@jbphet
Copy link
Contributor

jbphet commented Aug 7, 2023

@pixelzoom asked:

Which client is this? PhET-iO? Something else?

I'm referring to client code here, so I'm essentially saying that we simulation developers may have a need to create sim-specific categories so that sounds can be grouped and controlled according to the needs of that sim.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants