Adding built-in support and rules for games#134
Adding built-in support and rules for games#134thomaslaurenson merged 2 commits intoTheGrayDot:mainfrom
Conversation
| {"generic", GameProfile::GENERIC}, | ||
| {"diablo1", GameProfile::DIABLO1}, | ||
| {"diablo", GameProfile::DIABLO1}, | ||
| {"lordsofmagic", GameProfile::LORDSOFMAGIC}, | ||
| {"lomse", GameProfile::LORDSOFMAGIC}, | ||
| {"starcraft", GameProfile::STARCRAFT1}, | ||
| {"starcraft1", GameProfile::STARCRAFT1}, | ||
| {"sc", GameProfile::STARCRAFT1}, | ||
| {"sc1", GameProfile::STARCRAFT1}, | ||
| {"warcraft2", GameProfile::WARCRAFT2}, | ||
| {"wc2", GameProfile::WARCRAFT2}, | ||
| {"war2", GameProfile::WARCRAFT2}, | ||
| {"diablo2", GameProfile::DIABLO2}, | ||
| {"d2", GameProfile::DIABLO2}, | ||
| {"warcraft3", GameProfile::WARCRAFT3}, | ||
| {"wc3", GameProfile::WARCRAFT3}, | ||
| {"war3", GameProfile::WARCRAFT3}, | ||
| {"warcraft3-map", GameProfile::WARCRAFT3_MAP}, | ||
| {"wc3-map", GameProfile::WARCRAFT3_MAP}, | ||
| {"war3-map", GameProfile::WARCRAFT3_MAP}, | ||
| {"wow1", GameProfile::WOW_1X}, | ||
| {"wow-vanilla", GameProfile::WOW_1X}, | ||
| {"wow2", GameProfile::WOW_2X}, | ||
| {"wow-tbc", GameProfile::WOW_2X}, | ||
| {"wow3", GameProfile::WOW_3X}, | ||
| {"wow-wotlk", GameProfile::WOW_3X}, | ||
| {"wow4", GameProfile::WOW_4X}, | ||
| {"wow-cataclysm", GameProfile::WOW_4X}, | ||
| {"wow5", GameProfile::WOW_5X}, | ||
| {"wow-mop", GameProfile::WOW_5X}, | ||
| {"starcraft2", GameProfile::STARCRAFT2}, | ||
| {"sc2", GameProfile::STARCRAFT2}, | ||
| {"diablo3", GameProfile::DIABLO3}, | ||
| {"d3", GameProfile::DIABLO3} |
There was a problem hiding this comment.
These are all valid strings to use for the --game parameter
There was a problem hiding this comment.
These look good. Tested from the CLI help menu and it is very clear.
| case GameProfile::GENERIC: return "generic"; | ||
| case GameProfile::DIABLO1: return "diablo1"; | ||
| case GameProfile::LORDSOFMAGIC: return "lordsofmagic"; | ||
| case GameProfile::WARCRAFT2: return "warcraft2"; | ||
| case GameProfile::STARCRAFT1: return "starcraft1"; | ||
| case GameProfile::DIABLO2: return "diablo2"; | ||
| case GameProfile::WARCRAFT3: return "warcraft3"; | ||
| case GameProfile::WARCRAFT3_MAP: return "warcraft3-map"; | ||
| case GameProfile::WOW_1X: return "wow-vanilla"; | ||
| case GameProfile::WOW_2X: return "wow-tbc"; | ||
| case GameProfile::WOW_3X: return "wow-wotlk"; | ||
| case GameProfile::WOW_4X: return "wow-cataclysm"; | ||
| case GameProfile::WOW_5X: return "wow-mop"; | ||
| case GameProfile::STARCRAFT2: return "starcraft2"; | ||
| case GameProfile::DIABLO3: return "diablo3"; | ||
| default: return "generic"; |
There was a problem hiding this comment.
These values will be displayed by CLI11. They are fewer than what the --game parameter accepts. I figured it would be a bit repetitive for the user to have to wade through too many aliases, e.g. "warcraft3, wc3, war3, warcraft3-map, wc3-map, war3-map". The reason I put these aliases there are, of course, to make it convenient for the user
There was a problem hiding this comment.
Agreed - this is a good solution and easy to read/use in the help menu
| str(binary_path), "create", | ||
| "--version", str(version), | ||
| "--file-flags1", "4294967295", | ||
| "--file-flags2", "4294967295", | ||
| "--file-flags3", "0", | ||
| "--attr-flags", "15", | ||
| "--flags", "66048", | ||
| "--compression", "2", | ||
| "--compression-next", "4294967295", | ||
| "-o", str(output_file), | ||
| str(target_dir), |
There was a problem hiding this comment.
I tried to have these MPQs be as similar as possible to how they were before, otherwise many old tests would need to be updated. But this test becomes a bit "messy" because of it.
There was a problem hiding this comment.
No problem - maybe I could clean them up later, but they are fine
| str(binary_path), "create", "-s", | ||
| "--file-flags1", "4294967295", | ||
| "--file-flags2", "4294967295", | ||
| "--file-flags3", "0", | ||
| "--attr-flags", "15", | ||
| "--flags", "512", | ||
| "--compression", "2", | ||
| "--compression-next", "4294967295", | ||
| "-o", str(output_file), | ||
| str(target_dir), |
There was a problem hiding this comment.
I tried to have these MPQs be as similar as possible to how they were before, otherwise many old tests would need to be updated. But this test becomes a bit "messy" because of it.
| # Create output_file path without suffix (default extract behavior is MPQ without extension) | ||
| output_file = output_dir.with_suffix("") | ||
|
|
||
| # Create output_files set based on directory contents (not full path) | ||
| output_files = set(fi.name for fi in output_file.glob("*")) | ||
| output_files = set(fi.name for fi in output_dir.glob("*")) | ||
|
|
||
| assert result.returncode == 0, f"mpqcli failed with error: {result.stderr}" | ||
| assert output_lines == expected_lines, f"Unexpected output: {output_lines}" | ||
| assert output_file.exists(), "Output directory was not created" | ||
| assert output_dir.exists(), "Output directory was not created" |
There was a problem hiding this comment.
I see now that this actually isn't related to the rest of the PR (sorry!). This change makes the test nicer and more consistent with other test cases though.
There was a problem hiding this comment.
Haha - agreed it is a nice change, happy that it got included
| "Header offset: 0", | ||
| "Header size: 32", | ||
| "Archive size: 1380", | ||
| "Archive size: 1381", |
There was a problem hiding this comment.
Since we're now creating archives with SFileCreateArchive2 instead of SFileCreateArchive, there are some minor size differences.
There was a problem hiding this comment.
That makes sense - thanks for clarifying
| expected_output = { | ||
| " 27 enUS (listfile)", | ||
| " 148 enUS (attributes)", | ||
| " 149 enUS (attributes)", |
There was a problem hiding this comment.
Since we're now creating archives with SFileCreateArchive2 instead of SFileCreateArchive, there are some minor size differences.
thomaslaurenson
left a comment
There was a problem hiding this comment.
This is a great PR - thanks so much for the contribution. Sorry it took so long for a review, it had lots of changes and I wanted to do as much testing as I could. But from my testing of your branch, it works nicely - and is a welcome addition.
Happy to merge!
| {"generic", GameProfile::GENERIC}, | ||
| {"diablo1", GameProfile::DIABLO1}, | ||
| {"diablo", GameProfile::DIABLO1}, | ||
| {"lordsofmagic", GameProfile::LORDSOFMAGIC}, | ||
| {"lomse", GameProfile::LORDSOFMAGIC}, | ||
| {"starcraft", GameProfile::STARCRAFT1}, | ||
| {"starcraft1", GameProfile::STARCRAFT1}, | ||
| {"sc", GameProfile::STARCRAFT1}, | ||
| {"sc1", GameProfile::STARCRAFT1}, | ||
| {"warcraft2", GameProfile::WARCRAFT2}, | ||
| {"wc2", GameProfile::WARCRAFT2}, | ||
| {"war2", GameProfile::WARCRAFT2}, | ||
| {"diablo2", GameProfile::DIABLO2}, | ||
| {"d2", GameProfile::DIABLO2}, | ||
| {"warcraft3", GameProfile::WARCRAFT3}, | ||
| {"wc3", GameProfile::WARCRAFT3}, | ||
| {"war3", GameProfile::WARCRAFT3}, | ||
| {"warcraft3-map", GameProfile::WARCRAFT3_MAP}, | ||
| {"wc3-map", GameProfile::WARCRAFT3_MAP}, | ||
| {"war3-map", GameProfile::WARCRAFT3_MAP}, | ||
| {"wow1", GameProfile::WOW_1X}, | ||
| {"wow-vanilla", GameProfile::WOW_1X}, | ||
| {"wow2", GameProfile::WOW_2X}, | ||
| {"wow-tbc", GameProfile::WOW_2X}, | ||
| {"wow3", GameProfile::WOW_3X}, | ||
| {"wow-wotlk", GameProfile::WOW_3X}, | ||
| {"wow4", GameProfile::WOW_4X}, | ||
| {"wow-cataclysm", GameProfile::WOW_4X}, | ||
| {"wow5", GameProfile::WOW_5X}, | ||
| {"wow-mop", GameProfile::WOW_5X}, | ||
| {"starcraft2", GameProfile::STARCRAFT2}, | ||
| {"sc2", GameProfile::STARCRAFT2}, | ||
| {"diablo3", GameProfile::DIABLO3}, | ||
| {"d3", GameProfile::DIABLO3} |
There was a problem hiding this comment.
These look good. Tested from the CLI help menu and it is very clear.
| case GameProfile::GENERIC: return "generic"; | ||
| case GameProfile::DIABLO1: return "diablo1"; | ||
| case GameProfile::LORDSOFMAGIC: return "lordsofmagic"; | ||
| case GameProfile::WARCRAFT2: return "warcraft2"; | ||
| case GameProfile::STARCRAFT1: return "starcraft1"; | ||
| case GameProfile::DIABLO2: return "diablo2"; | ||
| case GameProfile::WARCRAFT3: return "warcraft3"; | ||
| case GameProfile::WARCRAFT3_MAP: return "warcraft3-map"; | ||
| case GameProfile::WOW_1X: return "wow-vanilla"; | ||
| case GameProfile::WOW_2X: return "wow-tbc"; | ||
| case GameProfile::WOW_3X: return "wow-wotlk"; | ||
| case GameProfile::WOW_4X: return "wow-cataclysm"; | ||
| case GameProfile::WOW_5X: return "wow-mop"; | ||
| case GameProfile::STARCRAFT2: return "starcraft2"; | ||
| case GameProfile::DIABLO3: return "diablo3"; | ||
| default: return "generic"; |
There was a problem hiding this comment.
Agreed - this is a good solution and easy to read/use in the help menu
| createInfo.dwRawChunkSize = settings.rawChunkSize; | ||
| createInfo.dwMaxFileCount = fileCount; | ||
|
|
||
| const bool result = SFileCreateArchive2( |
There was a problem hiding this comment.
Thanks for updating to the newer/better Stormlib function. Had been meaning to do this - so a very welcome addition
| if locale == "": # Default locale - create a new MPQ file | ||
| result = subprocess.run( | ||
| [str(binary_path), "create", "-v", "1", "-o", str(mpq_many_locales_file_name), str(locales_files_dir)], | ||
| [str(binary_path), "create", "--version", "1", "-o", str(mpq_many_locales_file_name), str(locales_files_dir)], |
There was a problem hiding this comment.
This might be a breaking change (removing the -v option) - but I don't think it is a big issue. Also, I like that the single char argument is not provided - because version is (will be) a less used option and MPQ creation override
| str(binary_path), "create", | ||
| "--version", str(version), | ||
| "--file-flags1", "4294967295", | ||
| "--file-flags2", "4294967295", | ||
| "--file-flags3", "0", | ||
| "--attr-flags", "15", | ||
| "--flags", "66048", | ||
| "--compression", "2", | ||
| "--compression-next", "4294967295", | ||
| "-o", str(output_file), | ||
| str(target_dir), |
There was a problem hiding this comment.
No problem - maybe I could clean them up later, but they are fine
| # Create output_file path without suffix (default extract behavior is MPQ without extension) | ||
| output_file = output_dir.with_suffix("") | ||
|
|
||
| # Create output_files set based on directory contents (not full path) | ||
| output_files = set(fi.name for fi in output_file.glob("*")) | ||
| output_files = set(fi.name for fi in output_dir.glob("*")) | ||
|
|
||
| assert result.returncode == 0, f"mpqcli failed with error: {result.stderr}" | ||
| assert output_lines == expected_lines, f"Unexpected output: {output_lines}" | ||
| assert output_file.exists(), "Output directory was not created" | ||
| assert output_dir.exists(), "Output directory was not created" |
There was a problem hiding this comment.
Haha - agreed it is a nice change, happy that it got included
| "Header offset: 0", | ||
| "Header size: 32", | ||
| "Archive size: 1380", | ||
| "Archive size: 1381", |
There was a problem hiding this comment.
That makes sense - thanks for clarifying
| - Pipe the output to `grep` or other tools to search, filter, or process files | ||
| - Redirect output to files or other commands for further automation | ||
|
|
||
| **This project is primarily for older World of Warcraft MPQ archives**. This means it has been authored for MPQ archives versions 1 and 2, which include the following World of Warcraft (WoW) versions: Vanilla (1.12.1), TBC (2.4.3), and WoTLK (3.3.5). It has only been tested on WoW MPQ archives/patches that use MPQ versions 1 or 2. No testing has been performed on other MPQ versions or archives from other games. However, the tool will most likely work on other MPQ archive versions, as the underlying Stormlib library supports all MPQ archive versions. |
There was a problem hiding this comment.
A welcome addition to remove that statement 👍
Listing and extracting works in older games, but not MPQ creation or adding files to archives.
The Readme says (emphasis mine):
... however, this is incorrect; mpqcli currently compresses using Zlib, which is a compression type that older games don't handle. Different games use different flags and compression methods, and they may differ on file types.
This PR implements support for all relevant Blizzard/Sierra games starting with Diablo I (1997) up until Diablo III (2012).
The CLI now exposes a
--gameflag forcreateandaddsubcommands, and by providing it, mpqcli automatically selects the correct settings based on the game and what sort of file the user is adding. The CLI also exposes ways to override all of these settings for advanced users, but just giving e.g.--game warcraft3should be enough for all normal usage.Three rules exist to determine what settings to chose: based on file size, based on a file name mask (e.g. *.wav), or default rules if nothing else applies.
This "compatibility matrix" of settings and games comes directly from Ladislav Zezula: ladislav-zezula/StormLib#406 (comment)
I realize it is quite a big PR, but it should hopefully be straight-forward enough. Please let me know if you have any thoughts or issues.