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

Feature addition and bugfix to SCI resource compression tool #2

Merged
merged 3 commits into from Aug 24, 2011

Conversation

Projects
None yet
3 participants
@agfor
Contributor

agfor commented Aug 7, 2011

The first commit fixes incorrect offset adjustment for Freddy Pharkas resources with incorrect sizes.

The second commit adds support for compressing KQ5 and Jones in the Fast Lane audio resources.

agfor added some commits Aug 7, 2011

TOOLS: Fixed compress_sci Freddy Pharkas WAV size fix
Freddy Pharkas has some incorrect WAV sizes in resource.sfx.
The fix for this did not correctly adjust the file position for
resources with SOL headers.
TOOLS: Add raw audio resource support to compress_sci
KQ5 and Jones in the Fast Lane contain AUDIOO1.002 files which are raw PCM.
They also contain AUDIO001.MAP files that list the offsets and sizes of
each sample. This patch allows the SCI resource compression tool to
recognize the RAW files, parse the maps, and compress the resources.
@lordhoto

This comment has been minimized.

lordhoto commented on engines/sci/compress_sci.cpp in 12846bc Aug 8, 2011

New should throw an exception in case allocation fails, so this is probably not needed here.

The old code didn't seem to think about that, but I guess it's not a reason to add it again ;-). Same goes for lines 307+308 and 328+329.

This comment has been minimized.

Owner

agfor replied Aug 8, 2011

I made it consistent, the old code checked some allocations and not others. I'll remove them.

This comment has been minimized.

lordhoto replied Aug 8, 2011

That's a good thing :-).. But in this case we just do not need it, thus we can drop it at this occasion too ;-).

@lordhoto

This comment has been minimized.

lordhoto commented on engines/sci/compress_sci.cpp in 12846bc Aug 8, 2011

Wouldn't it be simpler to use the "fake" entry as proper end of file indicator?

This comment has been minimized.

Owner

agfor replied Aug 8, 2011

The intention was to make sure not to reach the end of the file. I'll change it.

This comment has been minimized.

lordhoto replied Aug 8, 2011

Any reason why you do not want to reach the end of the file? It seems simpler to take advantage of the marker in the resource map. Since that would be pretty much just a simple while loop.

This comment has been minimized.

Owner

agfor replied Aug 8, 2011

There seem to be a lot of malformed files that the tool compensates for in various ways. I was being cautious in case there was no fake entry at the end in some file. It's easy enough to put it back if that turns out to be the case; for now I've switched it as you suggested.

@lordhoto

This comment has been minimized.

lordhoto commented on engines/sci/compress_sci.cpp in 12846bc Aug 8, 2011

Please use && instead of and.

This comment has been minimized.

Owner

agfor replied Aug 8, 2011

Too much Python. I'll fix this and the "or".

@lordhoto

This comment has been minimized.

lordhoto commented on engines/sci/compress_sci.cpp in 12846bc Aug 8, 2011

Please use || instead of or.

Also please use decimal numbers for file sizes instead of hexadecimal ones.

This comment has been minimized.

Owner

agfor replied Aug 8, 2011

This was again to be consistent with the rest of the file, specifically lines 113 & 115. Would you like me to change all of them to decimal?

This comment has been minimized.

lordhoto replied Aug 8, 2011

I am afraid I am not really familiar with this tool. I just wanted to give some feedback, since it seems no one else bothered to do so, so I didn't know it's using this in other occasions too. Personally I think decimal numbers are better for file sizes, since it's easier to compare it to your files. So it might be a good idea to change these at the lines you mentioned too.

@lordhoto

This comment has been minimized.

lordhoto commented on engines/sci/compress_sci.cpp in 12846bc Aug 8, 2011

Why is this a warning? Shouldn't it rather be a debug output?

This comment has been minimized.

Owner

agfor replied Aug 8, 2011

There were other "warning()"s in the file, and no debugs, so I just went with consistency, although the others are actually places where the original files are malformed. I'll change it.

This comment has been minimized.

Owner

agfor replied Aug 8, 2011

I forgot (it's been a while since I wrote this) -- the is no "debug()", only "warning()" and "print()". "warning" just prepends "WARNING" to the string. I'll change it to "print()".

This comment has been minimized.

lordhoto replied Aug 8, 2011

Yes, using print is probably what you want.

@lordhoto

This comment has been minimized.

lordhoto commented on engines/sci/compress_sci.cpp in 12846bc Aug 8, 2011

Warning seems odd here too, it should probably use something just for status output like print. Also I think we automatically add new lines.

This comment has been minimized.

Owner

agfor replied Aug 8, 2011

Consistency again - the other "warning()"s and "print()"s have newlines. I'll change it to "print()". Do you want me to remove all the newlines or leave them?

This comment has been minimized.

lordhoto replied Aug 8, 2011

I checked up again, it seems our tools code doesn't add newlines automatically. I remember it differently. So sorry about that, just keep the new lines.

TOOLS: Cleanup compress_sci.cpp
This commit removes unneeded memory allocation checks,
It replaces hexadecimal file sizes with decimal file sizes.
It cleans up the previous patch which added raw audio support
@bluegr

This comment has been minimized.

Member

bluegr commented Aug 13, 2011

Thanks for updating the tool :) The changes you added should help compressing those huge raw audio files in KQ5 and Jones.

After lordhoto's comments and suggestions (thanks!), this looks to be in a good shape to be merged. Unfortunately, I can't test the changes, as I'm on vacations right now :( So, unless someone else can test this, I can test and merge it in about 10 days, once I get back.

@agfor

This comment has been minimized.

Contributor

agfor commented Aug 13, 2011

Ten days sounds great to me; the patch sat on the tracker for 3 months so far :). If you're trying to make the ultimate ScummVM USB stick, the 50mb saved on KQ5 counts! Jones? Well, it wasn't any extra work, anyway.

@agfor agfor closed this Aug 13, 2011

@agfor agfor reopened this Aug 13, 2011

@agfor

This comment has been minimized.

Contributor

agfor commented Aug 13, 2011

Sorry, misclick.

bluegr added a commit that referenced this pull request Aug 24, 2011

Merge pull request #2 from agfor/master
Feature addition and bugfix to SCI resource compression tool. The audio resources of KQ5 CD and Jones CD can now be compressed.

@bluegr bluegr merged commit e4650fa into scummvm:master Aug 24, 2011

@bluegr

This comment has been minimized.

Member

bluegr commented Aug 24, 2011

Great work :) Works beautifully, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment