Mechanical sound support part 3

[arthurkehrwald]

Picking up where linojon left off in #466. I merged his changes into my fork and rebased the feature branch onto the upstream master branch. Here are the problems I have identified so far:

Problems

Usability

Add mech sound button situation ✅

• Initial problem: Unity does not respect default volume when adding new mech sound using the + button in the inspector list view
• Attempted solution: linojon added another button as a workaround
• New problem
  • The + button is still there
  • There are now two ways to do the same thing
  • Only one is correct, but there is no obvious way for a user to tell which
  • Choosing the wrong one will result in no audio being played, because the volume is zero by default

Audio playback in editor ✅

SoundAssets have a button to play back their audio in the editor. Pressing this button creates a new game object with an audio source in the currently open scene, if such an object doesn't already exist. This behavior is an unexpected and unwelcome side-effect, because it permanently alters the currently open scene.

Sounds must be added manually ✅

Users must manually add sounds to each game object (these should be included in prefabs instead).

Architecture

Inheritance for the sake of type constraints ✅

DropTargetBankApi needed a reference to the associated DropTargetBankComponent to call its EmitSound method. In other xApi classes the associated xComponent instance is referenced using the MainComponent property defined in the abstract base class ItemApi . DropTargetBankApipreviously did not inherit from ItemApi because ItemApi has a type parameter for the component type and a constraint that requires that type parameter (in this case DropTargetBankComponent) to inherit from the abstract base class MainComponent, which DropTargetBankComponent did not. linojon solved this by making DropTargetBankComponent inherit from MainComponentpurely to satisfy the constraint of ItemApi without actually implementing any of the abstract methods of MainComponent (and instead throwing an Exception when they are called). This is bad because it leads to unexpected results when calling those methods

Diligence

• Questions in code comments ✅
• Commented out code ✅
• SerializableDictionary with NonSerialized attribute in MechSoundsComponent seems a bit pointless, but I have not yet checked whether it actually is ✅
• Redundant use of Serialized and NonSerialized attributes ✅
• public fields all over ✅
• Fading logic is absolutely bizarre ✅
• Use of end-of-life IMGUI API for editor code ✅

Bugs

• Sound fading doesn’t work if fade duration is shorter than one second ✅
• MechSound.volume is not applied ✅

Incomplete

• Score motor sounds ✅
• Ball collision sounds

[freezy]

Awesome!

This will take a little bit of time to review, trying my best for this week.

[arthurkehrwald]

Committed a fix for the zero-volume-by-default problem when adding MechSound instances to the list in the inspector. It applies the default value the first time an instance is serialized based on the value of a private flag. The downside is that it only works for the first item added to the list, because Unity clones the field values of each subsequent instance (including the flag) from the previous item in the list, but that is arguably ok because any unreasonable values that are cloned from the previous instance are the user's responsibility since they must have changed the defaults at some point.

[arthurkehrwald]

Ok but what about the prefabs in the main repo, like the flipper? Should I move those into the pipeline repos?

[freezy]

Yes, most of the prefabs are already moved on my branch.

[arthurkehrwald]

Got it. One more thing: The prefabs will need to reference my new sound code, so is it ok to introduce a dependency on the main repo in the asset library?

[freezy]

Not sure I understand - The sound code is in the main repo, right? If a component that is implemented in the main repo sits on a prefab in the asset lib, that shouldn't be a problem, as long as they are in the same project.

[arthurkehrwald]

Yea I guess so. It's just a formal thing. You could install the asset library without the main package technically and then you'd get missing scripts in the prefabs. But then again who does that.

[arthurkehrwald]

Ready to merge!
VisualPinball/VisualPinball.Unity.AssetLibrary#8
VisualPinball/VisualPinball.Unity.Urp#19
VisualPinball/VisualPinball.Unity.Hdrp#22

[freezy]

Awesome! I'll go through this hopefully this weekend!

[freezy]

I've now rebased this onto my collider branch, since I felt more comfortable resolving your commits than mine. ;)

I'll do the testing now and if everything is good then I'll merge my branch first and then this one.

[freezy]

Okay, in case my comments weren't clear in the other repo. I've now rebased and pushed the sound code:

• https://github.com/freezy/VisualPinball.Engine/tree/feature/sounds-rebased
• https://github.com/VisualPinball/VisualPinball.Unity.Hdrp/tree/feature/sounds-rebased
• https://github.com/VisualPinball/VisualPinball.Unity.AssetLibrary/tree/feature/sounds-rebased

Can you:

1. Check those out
2. Apply the changes you added in the HDRP repo for bumper and gate to the Asset Library repo:
   • Bumper
   • Gate
3. Run a quick test if the sound stuff you've added is still working

That'd be awesome, then I'll can do the review and merge this relatively quickly.

[freezy]

I've got a few compilation errors, too (the logs below are from your branch, not the rebase branch, although they occur on the rebase branch as well):

VisualPinball.Engine\VisualPinball.Unity\VisualPinball.Unity.Editor\Sound\CoilSoundComponentInspector.cs(26,45): error CS0246: The type or namespace name 'SoundComponentInspector' could not be found (are you missing a using directive or an assembly reference?)

VisualPinball.Engine\VisualPinball.Unity\VisualPinball.Unity.Editor\Sound\CoilSoundComponentInspector.cs(31,33): error CS0115: 'CoilSoundComponentInspector.CreateInspectorGUI()': no suitable method found to override

VisualPinball.Engine\VisualPinball.Unity\VisualPinball.Unity.Editor\Sound\SwitchSoundComponentInspector.cs(26,47): error CS0246: The type or namespace name 'SoundComponentInspector' could not be found (are you missing a using directive or an assembly reference?)

VisualPinball.Engine\VisualPinball.Unity\VisualPinball.Unity.Editor\Sound\SwitchSoundComponentInspector.cs(31,33): error CS0115: 'SwitchSoundComponentInspector.CreateInspectorGUI()': no suitable method found to override

[arthurkehrwald]

Ok, checking out the rebased branch now

[arthurkehrwald]

Getting a different compilation error: Packages\org.visualpinball.engine.unity\VisualPinball.Unity\VisualPinball.Unity\VPT\Flipper\FlipperComponent.cs(198,18): error CS0103: The name '_startAngle' does not exist in the current context

[freezy]

Yes I got that one too, maybe a relict from the rebase. You can remove the entire method since it's not used.

[arthurkehrwald]

Alright I got it to work I think. It's a bit hard to tell because I get like three frames per second when running HDRP on my laptop.

[freezy]

Awesome, everything seems to be fine too, here. Will do the review tomorrow hopefully. Cheers!

[freezy]

Started testing - This is awesome! 👍

Okay if I rename EnableAfterAwakeMonoBehaviour to EnableAfterAwakeComponent? That's the convention we're using in this project.

I'm also redoing the icons (so my Illustrator file is complete), what do you think about this design? Since the music notes don't really represent music..

[freezy]

Also adding curly brackets around oneliners, if you could do that in the future that'd be awesome.

[arthurkehrwald]

Glad you like it. The changes you suggested are fine by me. I didn't make the icons myself anyways. They are just recolored default icons from Unity.

[freezy]

Thanks a lot for this. I've pushed a commit to the -rebased branch (6abd49f), lemme know what you think. Some other comments below.

[arthurkehrwald]

I reviewed the changes you made on your branch. Formatting and style is too subjective and not important enough to argue about, so that's all good. The one thing I don't agree with is making private fields public so the inspector classes can access their names. If you look at Unity docs, you'll see they don't do this in their examples, and I think that's for a good reason. I know spelling out a variable name as a string kind of sucks for tracing usage in an IDE and breaks when the variable is renamed, but I think it's more important to communicate that this field is not relevant to outside classes and should not be written to from the outside. With the changes you've made, this is no longer clear.

[freezy]

Well, they are prefixed with _, so that seems a pretty obvious hint about visibility to me.

We're in a stage where refactoring is common, and I don't want to run after serialized member names that my IDE cannot handle.

I would agree with you if we were talking about public APIs. There, it would be important to communicate visibility, and you wouldn't want anybody to write to your internals. But it's all internal here, anyway. Visibility is basically determined by the lowest access something needs, and not by whether we decide to offer a functionality to a third party.

[arthurkehrwald]

Ok fair enough

[freezy]

Trying to fix the plunger issue and then I'll merge.