Salzian / bevy_fmod

Idiomatic integration of the FMOD audio engine into Bevy projects. Enables spatial audio, real-time parameter adjustments, live updates, and Doppler effects.
Apache License 2.0
56 stars 5 forks source link

Consider disambiguating names with Bevy's prelude #85

Closed msvbg closed 3 months ago

msvbg commented 3 months ago

It seems that AudioSource conflicts with a definition in the bevy prelude. Even if I disable bevy_audio, rust analyzer seems to get tripped up by this. The examples import both * and AudioSource from the bevy_fmod prelude, which looks a little bit weird to my eyes. I think naming it something like FmodAudioSource would be a little bit cleaner and avoid these problems.

Salzian commented 3 months ago

I see preludes in the rust ecosystem as a convenience for getting started quick with using a crate. That said, naming collisions are a problem happening in almost every programming language.

The examples import both * and AudioSource from the bevy_fmod prelude, which looks a little bit weird to my eyes

Well… that's exactly how you solve naming collisions. We only use the prelude in the examples for showing off the prelude itself. I personally recommend stopping using preludes once you go beyond the first steps and just import directly. Actually, given that our prelude approach now found its first victim, I might want to mark it somehow as not recommended. That way, people don't run into this problem anymore.

At work, we completely disallow wildcard imports for this reason. I personally adopted this approach for all my private projects and therefore never have this issue. I think the benefit of wildcard imports is too small compared to the potential headaches.

If you still want to use bevy::prelude::*, I would like to recommend this approach to you:

Define in your main.rs or a custom module an import alias like this:

pub(crate) use bevy_fmod::components::audio_source::AudioSource as FmodAudioSource;

This fulfills your original request. However, given my own workflow, I don't want to start prefixing each and every struct in this crate with Fmod as a hack to avoid naming collisions. I don't think it reads nicely, and it creates more questions down the line:

Further reading: https://www.lurklurk.org/effective-rust/wildcard.html

msvbg commented 3 months ago

I don't see preludes that way. My game is now ~12k SLOC and most files start with use bevy::prelude::*, and I think it is common practice to use the bevy prelude in the ecosystem. It might be better for the library to not have a prelude at all if it is not intended to be used.

Peepo-Juice commented 3 months ago

I am with @Salzian on this one. That's not to say that this complaint is invalid, but the best solution is to use aliases, and not to use preludes and wildcards.

Even if we did use the FmodXxx naming scheme, whats there to say it wont conflict with bevy or some other crate in the future? I'm not saying its likely they will implement their own fmod integration but its still technically possible. My point is that this solution soils the names for our crate and everyone else who chooses to take this path without actually eliminating the issue, whereas the solutions outlined by @Salzian allows for each crate to name their structs however they want while actually solving it once and for all. Its probably why Rust added aliasing in the first place.

GitGhillie commented 3 months ago

Even if I disable bevy_audio, rust analyzer seems to get tripped up by this. 

IMO this is an issue with Bevy/RA. If the bevy_audio feature is disabled then the Bevy audio types shouldn't be compiled

GitGhillie commented 3 months ago

IMO this is an issue with Bevy/RA. If the bevy_audio feature is disabled then the Bevy audio types shouldn't be compiled

Nvm this actually. Because we implement AudioSinkPlayback we depend on bevy_audio. Though it wouldn't be too difficult to get rid of that dependency if we want to go that route.

Salzian commented 3 months ago

@GitGhillie Didn't see your comment and just got to the same conclusion. I opened a PR. That will not solve all questions asked in this PR, but will resolve the root problem of the name clash.

msvbg commented 3 months ago

Nice. This is satisfactory to me.