-
-
Notifications
You must be signed in to change notification settings - Fork 998
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
WINTERMUTE: Correctly find .ogg version of .wav files #728
Conversation
So the changes here are (a) adding the dot before the extension, and then (b) removing the call to combine? I'm curious about (b) since your commit message says "probably don't want".. why probably? Also, you say "should fix #7088" - you don't have the game to test it? |
I tested the fix with the game (which is freely available online), and it works. It's correct that the simplest fix is adding the "." to the path. However, I'd rather keep the current path separator instead of reconstructing the entire path. Maybe the code could be simplified to look at the last 3 chars and replace them (in a new copy of the string) without reconstructing the path + name parts. Specifically: lower-case (BTW, the game crashes during the intro while playing |
True
True
I have it and it works for me. I have rephrased the commit, thanks.
Because I wouldn't want you to take my word for it until either @somaen (who originally wrote this bit) or @wjp have seen it.
Yes, it's #7089 |
I like this better as well. See updated commit. |
Common::String oggFilename = useFilename; | ||
oggFilename.erase(oggFilename.size() - 3); | ||
assert(oggFilename.lastChar() == '.'); | ||
oggFilename = oggFilename + "ogg"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't there an append function? I think s = s + suffix
creates a new string.
Instead of erasing, maybe just set the last 3 chars manually:
size = s.size()
s[size - 3] = 'o';
s[size - 2] = 'g';
s[size - 1] = 'g';
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really wouldn't worry about that compared to the cost of opening a file. But see also #688.
As it was, it didn't reliably work across platforms because it turned some\\windows\\path.wav into some/system/pathogg Note no "." before "ogg"; also since we use the new filename to search for the file inside DCPs, which use Windows naming, we don't want system-specific path format. Fixes #7088
AnsiString newFile = PathUtil::combine(path, name + "ogg"); | ||
if (BaseFileManager::getEngineInstance()->hasFile(newFile)) { | ||
useFilename = newFile; | ||
if (useFilename.hasSuffix(".wav")) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this case sensitive?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but it does do a toLowercase
on useFilename
above, so it shouldn't be an issue (for this check).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you prefer getExtension
? I'm rather agnostic on the issue (however you polish I think... well, it's still a hack) I was mostly taking @salty-horse's advice.
As it was, it didn't work because it turned
"some\windows\path.wav" into "some/system/pathogg" (not ".ogg"), so any file called by the scripts with its ".wav" extension but actually present in compressed .ogg form on the .dcp archive could not be found.
base_disk_file.cpp (apparently) takes care of preprocessing the path to make it system-agnostic already for those files which reside on the disk.
This one should fix https://sourceforge.net/p/scummvm/bugs/7088/
The corresponding bit in the original engine is:
This way it should work reliably on all platforms like this, but just to be sure I would love it if somebody who runs Windows would check it before merging (I won't probably be able to do it before tomorrow).