-
Notifications
You must be signed in to change notification settings - Fork 5
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
Make Velocity optional #53
Conversation
This might cause more confusing than good. We do need this component for spatial audio. If a developer forgets to add this component to an entity, the game will not app, but spatial audio will be broken. For global music, this might make sense, but I believe we should find a better solution. Maybe we can create two bundles:
These bundles only make sense if an event has or has not a spatializer plugin. Is there a way to check this with libfmod? Overall, I think the correct composition of Entities with all necessary components is an underlying issue of how bevy works. I don't know if there is a better way that enforces this behavior at build time. I think crashing the app on startup is the best feedback we can offer to the developer for now. |
I don't really see it as spatial audio would be broken without the Velocity component, instead I see it as: The Doppler effect would be disabled. Spatial audio would still work with this PR. Anyways, I don't mind going the bundle route but a few things:
Under Core API channels are some functions that could help here, but it looks like you'd have to go through each DSP of an event and check if there is an FMOD spatializer. And this way you cannot account for 3rd party spatializers I think... |
I had a wrong assumption that I included the logic for sending position info to FMOD in the Velocity component. As this component really is only used for providing the velocity for the Doppler effect, I think it can be optional. But..., I'd love this crate to be working 100% out of the box. We could investigate if ChannelControl::get3DDopplerLevel is sufficient to check on startup if any sources require some sort of Velocity provider. If an event does require it, and no velocity provider is given, we return a warning / an error (#46). |
Studio::EventDescription::isDopplerEnabled this would be the way if we want to add a warning I think. Already added #56 for inspiration on the bundle route, hopefully I will have time this weekend to look into #54 |
Let's revisit this issue as I'm a bit confused right now.
How does this currently break spatial audio? |
Currently on main the |
In other words: With this PR leaving out the Velocity component on an entity would disable the Doppler effect. Currently on main leaving out the Velocity component removes all spatialization, which is weird behavior IMO. |
Can you add a doc comment to AudioSource? Explaining that in order for spatial audio to work, the user has to add the Velocity component. Other than that, LGTM. I don't see a better solution right now. |
Added some comments, the bundles PR should improve things a bit as well |
Ready for review |
Just found out there is a bug where if you don't have the Velocity component the spatial audio doesn't work at all. This will fix that.