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

allow extension independent soundfile path, fix #642 #773

Merged
merged 1 commit into from Jul 26, 2015

Conversation

illwieckz
Copy link
Member

Hi, this PR allows the engine to use extension independent sound path, so mappers can just write elemusic to load elemusic.wav or elemusic.opus, if one of them is available.

Also, the engine can load the elemusic.opus path if the bsp was compiled with elemusic.wav path. It allows asset managers to convert sound file without having to recompile the bsp.

It's the first time I'm writing some c++ lines in my life, so your comments are welcome, perhaps there is a better way to do it! 😉

This will also help out-of-tree map packaging, allowing mappers to compile their map against lossless assets and distribute them with lossy assets.

This PR fixes issue #642.

@Amanieu
Copy link
Contributor

Amanieu commented Jul 19, 2015

Use FS::Path::Extension and FS::Path::BaseName instead of doing it manually. Looks good otherwise.

@Kangz
Copy link
Member

Kangz commented Jul 19, 2015

Please consider using the same approach as in tr_image: https://github.com/Unvanquished/Unvanquished/blob/master/daemon/src/engine/renderer/tr_image.cpp#L2030 as it handles paks dependencies too.
I don't like that we silently replace the extension as it is a bit confusing though. Can we only add an extension of none is present?

@DolceTriade
Copy link
Member

Hm, I didn't read the code close enough. I agree with kangz that we should prefer extension if given, otherwise, if none is provided or the extension given is not found, then we try all the available extensions and see what sticks.

@illwieckz
Copy link
Member Author

Use FS::Path::Extension and FS::Path::BaseName instead of doing it manually. Looks good otherwise.

Hum, it's ok for FS::Path::Extension, it returns .ogg if file is sound/ui/heartbeat2.ogg.
But FS::Path::BaseName is not the good one to use: it returns heartbeat2.ogg but I need sound/ui/heartbeat2.

Sorry if I fooled you with the “basename” variable, I need the full path without extension. Is there a FS::Path function to achieve that?

@Amanieu
Copy link
Contributor

Amanieu commented Jul 20, 2015

Oh sorry, the correct function should have been FS::Path::StripExtension

@illwieckz
Copy link
Member Author

Thanks, and I will use another variable name to be more explicit. :-)

@illwieckz
Copy link
Member Author

So, I modified my patch using FS::Path::Extension and FS::Path::StripExtension and with a package prioritization when looking for an alternative name if file is missing or use an unknown file format.

The travis-ci's build errored because of The command "sudo apt-key adv --keyserver x-hkp://pool.sks-keyservers.net --recv-key 06AF4C14" failed and exited with 2 during ., it's not related to this patch.

[Edit: after a force-push with the exact same code, travis-ci is fine]

@Kangz
Copy link
Member

Kangz commented Jul 21, 2015

That's much better, thanks for the iteration! I'll merge it soon unless @DolceTriade or @Amanieu have other concerns.

@illwieckz
Copy link
Member Author

@Amanieu, @Kangz, @DolceTriade, do you need something more (perhaps spacing?) to be able to merge this PR in unv 0.42.0 ?

@DolceTriade
Copy link
Member

LGTM

@illwieckz illwieckz changed the title use extension independent soundfile path, fix #642 allow extension independent soundfile path, fix #642 Jul 26, 2015
DolceTriade added a commit that referenced this pull request Jul 26, 2015
allow extension independent soundfile path, fix #642
@DolceTriade DolceTriade merged commit 4d59f1b into Unvanquished:master Jul 26, 2015
@illwieckz
Copy link
Member Author

Thanks for the merge.

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.

None yet

4 participants