feat!: add `Sync` bound on `Modulator` and `Sound` traits.

[zicklag]

Resolves #60.

Not sure if this is the desired solution, just pushing the code so I can use my fork temporarily in my project.

[zicklag]

Oh, unfortunately this doesn't work on WASM. The cpal backend isn't sync on WASM due to what looks like a similar dyn FnMut() without an extra Sync bound on it.

[tesselode]

Is there any reason why you'd want to not have the Sync bounds? If so, the Sync bound could be gated behind a feature flag.

[zicklag]

Is there any reason why you'd want to not have the Sync bounds? If so, the Sync bound could be gated behind a feature flag.

The reason for not having the Sync bound would be to allow people to use the AudioManger with a non-Sync audio backend, if they only need to access it from a single thread. That sounds like a potential use-case so a feature flag is probably good here.

It'd be great if we could make it generic somehow without needing a feature flag, but I think I heard that that's not possible without extra features in the Rust compiler. I might open a forum question about that, just to be sure.

[tesselode]

@zicklag did you find a way to make it generic?

[zicklag]

Honestly I forgot about this and haven't spent any more time on it yet. :/

[zicklag]

I'll open a forum topics for both of the questions now, actually.

[zicklag]

Regarding the optional Send bound I just found an existing forum topic covering this:

https://users.rust-lang.org/t/optional-send-sync-implementations-through-a-feature-flag/96007/6?u=zicklag

Unfortunately it seems that the only way to really do it is to have two traits. I think there's a chance we might be able make work nicely enough, and I have an idea for it, but it might not work. I'll try to see if I can figure something out real quick.

[zicklag]

OK, nevermind, after trying it out, I don't think there's a good way to make it generic.

I think a good solution, though, would be to require that it be Sync + Send, and then any audio backends that can't be Sync or Send can just use send_wrapper, which, now that I think of it, is the perfect solution for the WASM backend right now!

So SendWrapper allows you to wrap a non-Sync or non-Send type and makes it Sync + Send, but it will panic if you make any attempt to use it anywhere but the original thread. Using this on the WASM audio backend will give us complete confidence in the soundness of the implementation, without having to worry about what happens if WASM gets threads implemented.

I'll update this PR to reflect what I'm thinking, and try it out real quick with my game.

[zicklag]

There you go! Updated the PR, tested and working with my game, and no unsafe to be seen!

[tesselode]

To clarify, does this mean that if WASM does get thread support, and you try to use an AudioManager from multiple threads in WASM, you'll get a panic? (That's not a dealbreaker since that's a hypothetical future, and in that hypothetical future the CpalBackend could become properly Sync.)

[zicklag]

Yes, it would panic in that scenario, but I think you're right that the CpalBackend would probably end up sync in that scenario, and we could remove the SendWrapper.

Even if it didn't end up Sync, I still think the SendWrapper is probably the most flexible solution, because it doesn't make the API more limited, and it allow for Sync backends where supported.

[zicklag]

Just a friendly bump here. I'm wanting to do a crates.io release of my crate, but I'm still depending on this git branch so I can't do that yet.

Were there any remaining concerns to work out?

[tesselode]

@zicklag My main concern is that I'm not sure if there's any important use cases for Sounds, Effects, and Modulators to not be Sync. We discussed the use case of having a non-Sync backend, but I think that's been resolved by the use of send-wrapper for the wasm cpal backend, but I'm wondering if there's any reason why someone would want their Sounds to be non-Sync, even if they're ok with the backend being Sync.

[zicklag]

If you had a !Sync + !Send sound type, you could wrap it in a SendWrapper similar to the backend, and that would let you use the sound like normal as long as it's all on the same thread.

If you have a sound that is Send + !Sync, and you need to be able to send that sound across threads, it's a little trickier. It's possible to make a SyncWrapper, that is like SendWrapper, but allows you to transfer ownership of the type between threads, but I haven't found one on crates.io. Somebody would have to implement it, if they really needed that.

I think that scenario is probably the less common case, compared to needing a Sync + Send audio manager, but I'm not sure.

[tesselode]

@zicklag what's the crate you're wanting to publish?

[zicklag]

The Bones crate, used in the Jumpy game. Getting it on crates.io should also help us get a flatpak build working for Jumpy, since flatpak doesn't seem to like working with our git dependencies as it stands currently.

[erlend-sh]

Would greatly appreciate a yay or nay on this for the purpose making a release based on Kira proper or a workaround 🙏

Have you had a look at Bones? Now that it supports Lua scripting it might be the closest thing to a rusty equivalent of LÖVE, which you seem to have a lot of experience with!

[tesselode]

Sorry for the delay, I'm taking a look now.

[tesselode]

@zicklag @erlend-sh I found a way to make AudioManager Sync without requiring Sound or Modulator to be Sync. The only thing that was preventing the AudioManager from being Sync (assuming the backend is Sync) was the unused resource consumers, so I wrapped those in mutexes. I also included @zicklag's method to make CpalBackend Sync on wasm.

You can test this on the manager-sync branch.

Can you let me know if this works for you?

[zicklag]

Much appreciated! I've tried the branch and the only issue I have is that MockBackend is not Sync.

[tesselode]

Updated! Any other changes you can think of I should make before merging this in?

[zicklag]

That worked! That should be all I need, thank you! I'll close this PR, then.