Conversation
jthorborg
left a comment
There was a problem hiding this comment.
Thanks for the early look! Here are my high level notes as I can't run or test this yet (reasons given in comments).
-
Naming. We use CamelCasing everywhere else consistently, so I would highly suggest to review the coding conventions and apply that.
-
Aggregrate non-POD (?) initialization requires C++20, I'm not sure whether we want to impose that.
-
Either here or in a follow-up, I would abstract the sample buffers to channel + sample argument indexers instead, and hide whether it's internally contiguous and/or interleaved (which we're going away from!)
-
Consistent formatting, tried running our formatting tools on this?
-
Extranuous binaries, build artifacts, user settings committed along. Needs to be removed (would probably start from scratch to avoid committing this history).
-
Consider whether we can split this PR up multiple pieces, eg.:
- Move of the legacy stuff
- Introduction of the base SDK and solutions
- The new examples
- Unity setup
It would make it much easier to review and give quality feedback.
Otherwise, it looks sane and modern! Much better.
| Assets/EntityCache | ||
| Assets/EntityCache.meta | ||
| Assets/InitTestScene* | ||
| Assets/GlobalEntitiesDependencies/ | ||
| Assets/GlobalEntitiesDependencies.meta | ||
| Assets/SceneDependencyCache/ | ||
| Assets/SceneDependencyCache.meta | ||
| Assets/StreamingAssets/ | ||
| Assets/StreamingAssets.meta |
There was a problem hiding this comment.
Right, this needs to be investigated.
There was a problem hiding this comment.
Ah that's my bad, copy-pasta leftovers. We duplicated a gitignore from another project and forgot to tailor it.
| upm-ci~/ | ||
|
|
||
| # Overrides | ||
| !*~ |
There was a problem hiding this comment.
| !*~ | |
| !*~ | |
| SDK/VisualStudio/Debug | |
| SDK/VisualStudio/x64 |
These are build artifacts and should not be committed as they are in this PR.
There was a problem hiding this comment.
Not an expert in XCode but looks like a bunch of user settings and build artifacts were committed as well.
There was a problem hiding this comment.
Yep, we need to fix the .gitignore file.
There was a problem hiding this comment.
Aye, more copy-pasta.
|
|
||
| float prewarp(float w, float T) | ||
| { | ||
| return (2.0f / T) * tan((T / 2.0) * w); |
There was a problem hiding this comment.
1>D:\code\repos\NativeAudioPlugins\SDK\Plugin_Attenuator.cpp(46,27): warning C4244: 'return': conversion from 'double' to 'float', possible loss of data
There was a problem hiding this comment.
Yes, 'tan' needs to be replaced with 'tanf'.
| namespace dsp | ||
| { | ||
|
|
||
| static float db_to_ratio(float dB) |
There was a problem hiding this comment.
shouldn't this rather be in a shared utility somewhere?
There was a problem hiding this comment.
I'd like to avoid splitting the example projects into multiple files. Ideally they should be self-contained mini demos that demonstrate the SDK to users in the simplest way possible. Having each example in a single file makes it very easy for users to digest them.
| <ClCompile Include="..\EntryPoints.cpp" /> | ||
| <ClCompile Include="..\Plugin_Attenuator.cpp" /> | ||
| <ClCompile Include="..\Plugin_NoiseGenerator.cpp" /> | ||
| <ClCompile Include="..\Plugin_Reverb.cpp" /> |
There was a problem hiding this comment.
This file doesn't exist and causes build failure in visual studio:
1>Plugin_Reverb.cpp
1>c1xx : fatal error C1083: Cannot open source file: '..\Plugin_Reverb.cpp': No such file or directory
There was a problem hiding this comment.
The visual studio project is still WIP. Waiting for a Windows PC that will arrive next week! :)
|
|
||
| static const size_t entry_points_count = audio::entry_points.size(); | ||
|
|
||
| if (!is_initialised.test_and_set()) |
There was a problem hiding this comment.
No, probably not. But the function might be called multiple times. An atomic_flag is probably overkill -- although it doesn't harm.
| @@ -0,0 +1,6 @@ | |||
| fileFormatVersion: 2 | |||
There was a problem hiding this comment.
I know you want to emphasize the legaciness of this, but I would remove the all caps part of this path.
|
|
||
| Also, right now the 'AudioPluginUtil' component is a frankenstein mash-up of | ||
| essential functions related to the SDK and convenience functions for doing | ||
| complex number algebra (why don't we use std::complex?!), Fourier transforms, |
There was a problem hiding this comment.
we should definitely use std::complex
| returns a vector of parameter descriptors (in the 'EntryPoint' struct) is not | ||
| as userfriendly as it could be. | ||
|
|
||
| 3. Is it ok to use the C++ Standard Library (STL)? |
There was a problem hiding this comment.
You can definitely use the standard library (not the STL though, that's a historical artifact ;) )
But be careful about enforcing usage of these containers in scenarios where memory allocation might need to be controlled by users
There was a problem hiding this comment.
Okay, we need to discuss this.
I'll fix the formatting.
Yep, good point. We should probably stick with C++17 for now.
Very good point. I like this idea!
Good idea. We can do that in the end. |
|
Hi Nis, overall I think this is a pretty nice update. I've been working with this branch a little bit, as it was the basis for our platform audio programmer test, and it's pretty nice to work with. I love how simple and accessible it feels. I see you're getting some good, detailed feedback already from Janus and I don't have too much to add on that level, but I do have two things. First, I think we need to make sure it is possible to implement spatializer plugins and ambisonic decoder plugins, even within this new simplified interface. There isn't too much that's unique about them, but you need to have the option to add the following flags to the effect's definition: UnityAudioEffectDefinitionFlags_IsSpatializer, UnityAudioEffectDefinitionFlags_IsAmbisonicDecoder, and UnityAudioEffectDefinitionFlags_AppliesDistanceAttenuation. The other thing is the effect needs to be able to access UnityAudioEffectState's spatializerdata or ambisonicdata, depending on which kind of effect it is. That's really about it. Optionally, it also might be helpful to include a simple spatializer example, just because it kind of helps explain some of the matrix data that is being passed in that isn't maybe super intuitive. It definitely doesn't need to be the full example spatializer that was in the original plugin repo, but instead it could just implement a traditional 3d panning algorithm or something. I have an example you could take a look at if you're interested. Second, I monkeyed with the programmer test a little bit before I sent it out, so this could have been my mistake, but there might be an issue with the Windows debug build. I was getting crashes when trying to load the debug plugins into the Windows Editor. When you try this out on Windows, definitely check that out, and see if it is all working OK. |
NOTE: This is just an initial draft!
A Revised SDK for Unity Native Audio Plug-ins
This pull-request contains a proposal for an updated Unity Native Audio Plugin SDK.
Why
Working with the original SDK, I found that it is quite convoluted and exposes
an unnecessary amount of the low-level C API to users. Furthermore, it uses the
C macro preprocessor extensively which is fragile and cumbersome to debug.
Moreover, the project is somewhat of a mouthful with almost 30 audio effects and
instruments of varying complexity. A lot of those plug-ins are not
production-ready as they do memory allocation and mutex locking on the audio
thread. This includes the convolution reverb. In my opinion, a leaner project
with a few, simple plug-ins that demonstrate the SDK, is a better approach,
provided our goal is to teach users how the SDK works.
Also, right now the 'AudioPluginUtil' component is a frankenstein mash-up of
essential functions related to the SDK and convenience functions for doing
complex number algebra (why don't we use std::complex?!), Fourier transforms,
mutex locking (users should probably use std::mutex instead), and other stuff.
Finally, the original SDK needs some love anyway, as the Xcode project no longer
compiles out-of-the-box (haven't been able to test the VS project).
Approach
I've created a clean C++17 layer that completely shields the low-level API from
the user. It declares a handful of interfaces that users implement and then the
SDK handles all low-level stuff behind the scenes. All of the SDK that users
need to understand is contained in a single header,
'UnityAudioPluginInterface.hpp', which is about 100 lines of code. Having a
clean and self-contained C++ interface simplifies the mental burden that we put
on our users. Also, getting rid of all the macro-preprocessor non-sense and
weird callbacks such as 'AudioPluginUtil::RegisterParameter' from the original
SDK, makes plug-ins easier to maintain and debug.
On top of the C++ interface, I have implemented two simple plug-ins, an
attenuator and a noise generator. Both are very simple plugins. We could probably
do with an extra and more complicated plug-in with multiple parameters and more
advanced DSP.
I have also created a Unity project, an Xcode project (for macOS) and a Visual
Studio project (for Windows) and tested that the code compiles on both platforms.
On macOS, I have verified that the plug-ins can load and that they work as expected.
I haven't been able to do this on Windows as I don't have a Windows machine yet.
NOTE: I haven't made any changes to the low-level C API. (So far!)
Unclarified Points
We need to discuss what version of Unity / Xcode / Visual Studio we want to
use for the SDK.
The way parameters is exposed in the new SDK via a callback function that
returns a vector of parameter descriptors (in the 'EntryPoint' struct) is not
as userfriendly as it could be.
Is it ok to use the C++ Standard Library (STL)?
Next Steps
We need to implement a sane way to transfer samples from the managed code to
a plugin. The way it is done now in the convolution reverb example is a bit of
a hack and uses locks and memory allocation on the audio thread.
Better functionality for sending events to plugins from the managed code
would be useful for any kind of procedural audio that we want to trigger from
within a game.
This one is a little bit complicated! If we made it possible to pass data
to a 'UnityAudioEffect_CreateCallback', e.g. via the 'UnityAudioEffectState'
struct, we can most likely merge the 'UnityAudioPluginInterface-Raw.hpp' and
'UnityAudioPluginInterface-Raw.inl' headers into the
'UnityAudioPluginInterface.cpp' file. As of now, we need to include both of the
'Raw'-files in the 'EntryPoints.cpp'-file, which is annoying. Getting rid of
those two files, 'UnityAudioPluginInterface.hpp' would be the only file that
users ever need to think about.
A more complex demo effect, such as an equalizer or something similar, with
multiple parameters and a GUI.