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

Pk3 support + improved filesystem abstraction #337

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

skalleAnka
Copy link

@skalleAnka skalleAnka commented Sep 7, 2024

This implements a new file system abstraction for game data that should be easier to understand and follow than the workarounds that have accumulated over the years. The main reason for this refactoring is to enable easy implementation of other pack formats than PAK. More specifically, this adds pk3 support.

Access to game data in packs and on disk is now done through the QFS_* functions in filesys.h/filesys.c. The old file system functions dealing with this have been removed. Some functions from common.c were moved to filesys.c and renamed to begin with QFS_ rather than COM_ for consistency. The game search path handling remains in common.c.

PK3 Support

Should resolve issue #4. Support for PK3 files is implemented, and this is done using the existing miniz.c that was already included in the source tree, so no new dependencies are introduced.

Decompression is done "on the fly", meaning no temporary files with extracted data are created. It doesn't just slurp the entire file into memory from the pk3 file either, so memory usage should not become an issue even for large files.

Good to know

If a game folder has both pakN.pak and pakN.pk3 with the same number, the pak file will be used and the pk3 file ignored. This one I wasn't quite sure of - it seems the most backward compatible way, but if you want it to work some other way just let me know and I'll change it.

The function Sys_FileOpenRead which opened .PAK files and sometimes other files had a limitation of at most 32 simultaneously opened handles (10 before FitzQuake). Not sure what this was all about, but my guess it that this limit was originally supposed to be for the number of open packs. There is now just a limit of 32 open packs.

Music files requires more seeking back and forth inside the file than other file usage. In compressed pk3 packs, this operation is a bit more expensive CPU wise. For optimal results, already compressed music files could be stored as uncompressed data inside the pk3 file - but it's probably not a big deal either way, I didn't experience any noticeable ill effects while playing music from compressed data inside pk3 files.

Minimum required version of XMP (when used) is bumped to 4.5 (was 4.2). There were two different implementations of the file handling here, depending on version. I only rewrote the newer one and removed the old altogether.

Structures in the PAK handling and parts of the removed file handling used Z_Malloc. I didn't see a reason for any of this to waste space in the zone memory so now it all just uses malloc.

I also made some other small adjustments to adjacent code that didn't seem quite right, such as the id3 tag code reading values from buffers without checking the results from file reads now getting a memset to 0 of the buffer between reads.

@andrei-drexler This is your project, so if you want me to change anything about this, big or small, just let me know and I'll take care of it.

@NightFright2k19 You posted the original feature request, feel free to try it out and post feedback.

@skalleAnka skalleAnka force-pushed the pk3_support branch 2 times, most recently from a70715e to 19b3522 Compare September 8, 2024 14:39
@NightFright2k19
Copy link

NightFright2k19 commented Sep 9, 2024

I gave this a quick look today with the "Dimension of the Machine" addon, mostly since you gain a lot there with the PK3 format as the whole thing shrinks from almost 500 MB to merely 150 with maximum compression. Behavior seems identical and loading times are just as fast, which is what one would expect.

Sadly, one still cannot load addons from subdirectories such as "C:\Quake\addons\mg1", everything must reside inside a folder within the Quake root dir. There are many ports out there which can handle this. That however is probably a different issue and must be dealt with in another way.

A valuable note is the one about compressing music along with the rest of the files. Personally I always have music tracks external and therefore uncompressed, so I wouldn't do anything wrong. Some projects however release their tracks embedded, however in PAK format, so anyone who repacks those needs to keep that in mind.

@skalleAnka skalleAnka force-pushed the pk3_support branch 4 times, most recently from 203e8c2 to 8ce4a23 Compare September 11, 2024 20:48
@NightFright2k19
Copy link

Just gave this a quick shot with Mjolnir, which is probably the most massive mod I could find. Compressed with PK3, the whole thing goes from over 3 GB to less than half. I'm not sure whether it's OK to compress everything into one file, but I have spread it across eight parts or so to make sure Ironwail doesn't choke on it. Doing so seems to work fine and I cannot notice any impact on performance or loading times.

@skalleAnka
Copy link
Author

Thank you for trying it out @NightFright2k19

It should work fine to merge everything into one pk3 as there is no limit of 2048 files in the pk3 per pack as there is with.pak files. I have tried to do this with AD and experienced no problems.

@mhQuake
Copy link

mhQuake commented Sep 12, 2024

The 2048 files limit is artificial, though. There's nothing in the PAK format itself that imposes this limit. The PAK loader does put an array this size on the stack, but that's trivially coded around.

@skalleAnka
Copy link
Author

Yes, 2048 file size limit in a PAK could easily be removed if there is desire to do so. My opinion is that PAK files should stay PAK files with the same limitation they always had, as I belive that would make things less confusing for modders. That is, however, not for me to decide and also outside the scope of this PR.

At least with PK3 modders can knowingly chose to use a pack format that supports more files, and probably make a more informed decision about which engines would support it.

@NightFright2k19
Copy link

NightFright2k19 commented Sep 12, 2024

Alright, I will merge those Mjolnir pk3s then. After all, I only did the splitting in the first place since the pak format wouldn't allow me to. The original mod comes with a ton of loose files and I hate that. I guess it will also rather be beneficial to the loading process if it's just one file instead of several.

It's remarkable how much disk space can be saved with this. My Quake folder was over 7 GB with lots of addons before and now went down to 3. In average, paks can be shrunk by 50-60%. That's really a compelling argument for the format.

Question: Compressing sounds isn't an issue, right? Asking since it apparently is for music.

@skalleAnka
Copy link
Author

Sounds are just loaded as they are into memory if I remember correctly so no seeking around there.

Compressing the music probably won't cause any problems in practice either as far as I have noticed, if it did I would expect it to manifest as breaks in the playback.

@NightFright2k19
Copy link

So, Mjolnir works just fine with a single pk3 as well, as expected. Other large addons I've tested: Alkaline, Arcane Dimensions, Brutalist Jam and Remobilize. No problems with any of them, as far as I can see.

Normally I'm only using builds from the master branch, but this one's just too good to ignore. Please keep pushing this branch until it gets merged, which is hopefully soon.

@skalleAnka skalleAnka force-pushed the pk3_support branch 2 times, most recently from 5d95780 to 20aebfa Compare September 16, 2024 12:07
@andrei-drexler
Copy link
Owner

Hey, @skalleAnka, thanks for doing this work! It will take me a while to go through all the changes, and since they affect a core aspect of the codebase, it seems safer to do this after the 0.8 release. I'm hoping to get that out soon enough, though, and maybe .pk3 support could then come in a 0.8.1 update afterwards.

@skalleAnka
Copy link
Author

Hey @andrei-drexler , It was my pleasure. Take the time you need - in the mean time I will keep this branch updated with changes from master. Let me know if there is anything else I can do to make it easier.

@NightFright2k19
Copy link

NightFright2k19 commented Sep 17, 2024

What could still be considered:

Right now it only works if filenames stick to the pakXX.pk3 naming convention. For better content identification, it'd be great if any name were possible. Loading would then be done in alphabetical order.

Example:
mappack.pk3
newmusic.pk3
progs_test.pk3
...

@skalleAnka
Copy link
Author

If Andrei wants to, I could add that to this PR. Otherwise maybe a new one after this is merged?

@NightFright2k19
Copy link

Well, if it's still going to take a while before it's merged, feel free to add it here so I can test it before.

@skalleAnka
Copy link
Author

Would this be the correct behavior:
First load all that start with pakN.pk3, then deal with the rest alphabetically?

@NightFright2k19
Copy link

I would say so. Normally the pakN names would be expected as PAKs anyway, so these should always have priority. So if there are pakN.pak or pakN.pk3, these come first, then anything else.

Right now if there is a pak0.pak and pak1.pak for example, the first pakN.pk3 that's loaded would have to be named pak2.pk3. I guess this behavior can be kept since it's counter-intuitive to have identical filenames for pak and pk3 (e.g. pak0.pak and pak0.pk3 in the same directory).

@NightFright2k19
Copy link

NightFright2k19 commented Sep 18, 2024

I found a case which might be worth investigating.
I've converted "Travail" from PAK to PK3 and noticed that if you also zip the music, the game will directly crash when trying to start a new game - at least under certain conditions.

How to reproduce:

  1. Download my prepared "Travail" folder (148 MB)
  2. Place it in an "addons" subdir within the Quake folder (e.g. C:\Quake\addons)
  3. Launch the game like this: ironwail -basedir . -basedir addons
  4. Select the addon ingame via Options > Mods > Travail
  5. Crash will occur

Explanation about what the folder contains:

  • pak0.pk3: All original game files
  • pak1.pk3: Small patch made by Seven to add some extras like footsteps sounds and correction for nailgun projectiles
  • pak2.pk3: lit/vis files for colored lighting and transparent water
  • pak3.pk3: Travail soundtrack (uncompressed zipfile)

Important: The crash will NOT occur if...

  • manually copying a pre-existing ironwail.cfg into the Travail directory
  • unzipping the music and deleting pak3.pk3
  • placing the game in the root directory instead (C:\Quake\travail) and launching the mod via ironwail -game travail

@skalleAnka
Copy link
Author

Nice find @NightFright2k19! I have reproduced the problem and am looking into it.

@skalleAnka skalleAnka force-pushed the pk3_support branch 2 times, most recently from bc5758a to f856b48 Compare September 18, 2024 21:09
@skalleAnka
Copy link
Author

It should be fixed now. Thanks again for testing and the detailed bug report @NightFright2k19!

@skalleAnka skalleAnka force-pushed the pk3_support branch 7 times, most recently from dd8c73d to 9bd6d53 Compare October 16, 2024 16:05
@skalleAnka skalleAnka force-pushed the pk3_support branch 3 times, most recently from 01bdb44 to 10e97c9 Compare October 19, 2024 17:10
@skalleAnka
Copy link
Author

Regarding file names outside the pakN sheme, I had a peek at the QSS source code, and it looked like they first load the pakN.pak/pk3 and afterwards all the other ones in whatever order the filesystem API will list them. This would be pretty much alphabetically.

This method seems to work fine for at least QSS and maybe some other Quake ports, Quake 3 has also worked like this from the beginning without any major headaches as far as I know.

There is some value in doing it the same way as other Quake ports (less confusing for the users), so I think it should be done in a similar way.

As j4reporting pointed out, differet platforms and localization settings may cause file names to sort differently, so I think few adjustments to the basic scheme would be appropriate.

I propose that pk3 files should be loaded the same way as QSS, except that file names which contain non ASCII-characters would not even be considered for loading, and that the filenames that are ASCII only would always be sorted as if they were lowercase.

This would avoid most theoretical problems, while also being compatible with QSS for almost all practical purposes.

I wouldn't want to add anything that causes too much disagreement, though - so unless there seems to be some disagreements brewing I could start adding this in a few days.

@skalleAnka skalleAnka force-pushed the pk3_support branch 3 times, most recently from aa6da00 to 2fae8ca Compare October 26, 2024 23:44
@skalleAnka
Copy link
Author

I have now (as suggested by @NightFright2k19) added support for other filenames than pakN. This only applies if the file is a .pk3 file. It is also required that the file name is ASCII characters only for it to be considered. The files are loaded after the regular pak files and always sorted as if they were lowercase. This should ensure consistent load order across platforms.

I also threw in support for legacy encoded file names inside the pk3 file. Not sure if really necessary or just bloat as filenames in packs tend to be ASCII only (and those that aren't should be UTF-8 if modern tools are used), but QSS seems to be doing this also.

@Moonkeytest
Copy link

We'll finally be getting PK3 support in Ironwail soon-ish? o_0

@skalleAnka skalleAnka force-pushed the pk3_support branch 2 times, most recently from 785e1c9 to 1938133 Compare November 20, 2024 16:50
@skalleAnka skalleAnka force-pushed the pk3_support branch 2 times, most recently from dea3a59 to 48d734d Compare November 27, 2024 19:31
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.

6 participants