Skip to content
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

load mp3/wav file with its own extension #320

Closed

Conversation

chaosprint
Copy link
Contributor

@chaosprint chaosprint commented Apr 17, 2023

Even if in the future we switch ffmpeg to pure rust solutions, we may still need to avoid inexplicit extension change from wav or mp3 to ogg. The logic is that if we do the format conversion secretly, we should do the URL conversion secretly as well to avoid extra cognitive load for users/developers.

@philpax
Copy link
Contributor

philpax commented Apr 17, 2023

I agree with the change (it is confusing to change the extension), but I'm not sure if replacing the extension in asset::load is the best place to do it.

Maybe we should have the build system create a folder and keep the file in there, so that you can pass in assets/shoot.wav to asset::url, which will look up ambient-asset:/assets/shoot.wav/out.ogg?

That would also allow us to have different-quality sounds if need be.

@chaosprint
Copy link
Contributor Author

I agree with the change (it is confusing to change the extension), but I'm not sure if replacing the extension in asset::load is the best place to do it.

Maybe we should have the build system create a folder and keep the file in there, so that you can pass in assets/shoot.wav to asset::url, which will look up ambient-asset:/assets/shoot.wav/out.ogg?

That would also allow us to have different-quality sounds if need be.

Good idea. I will work on the audio loading part to support different formats.

@chaosprint
Copy link
Contributor Author

replaced by #344

@chaosprint chaosprint closed this Apr 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants