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
introduce dpk format #14
Conversation
|
The only thing I haven't tested yet is autodownloading. Here I describe my test environment, on engine path: on homepath: While having Of course loading badly named pk3/bsp does not work, it's out of scope there, and it's not my own problem. The |
|
Loading automatically legacy Works. |
|
- obsolete - |
|
This change also means a game like Xonotic on Daemon can switch to |
|
Beware, do not merge this PR too fast, we must first ensure every asset contributor is able to switch to dpk vfs, so it's better to wait for myself uploading the assets repositories I'm working on (it's now a matter of days). |
src/common/FileSystem.cpp
Outdated
| { | ||
| if ( version.compare("") != 0 ) { | ||
| // versioned dpk | ||
| return Str::Format("%s.%s", MakePakName(name, version, *checksum), "pk3"); |
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.
Should "versioned dpk" paks not have the suffix "dpk" and "legacy unversioned pk3" have the suffix "pk3"?
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.
oh yes, that's the only part I haven't fully tested yet since it's used in autodownload stuff (have to setup a server etc.), nice catch!
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.
fixed
src/common/FileSystem.cpp
Outdated
| @@ -2386,12 +2395,36 @@ bool ParsePakName(const char* begin, const char* end, std::string& name, std::st | |||
| return true; | |||
| } | |||
|
|
|||
| std::string MakePakFileName(Str::StringRef name, Str::StringRef version, Util::optional<uint32_t> checksum) | |||
| { | |||
| if ( version.compare("") != 0 ) { | |||
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.
For readability I would prefer:
if ( !version.empty() )
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 agree!
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.
fixed
src/common/FileSystem.cpp
Outdated
| if (!UseLegacyPaks()) { | ||
| suffixLen = type == pakType_t::PAK_DIR ? strlen(PAK_DIR_EXT) : strlen(PAK_ZIP_EXT); | ||
| } else { | ||
| suffixLen = type == pakType_t::PAK_DIR ? strlen(LEGACY_PAK_DIR_EXT) : strlen(LEGACY_PAK_ZIP_EXT); |
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.
that part makes no sense at all and only works because strlen(pk3) == strlen(dpk), I'm fixing this.
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.
fixed
7ce30a2
to
1909f1d
Compare
|
This new code adds a My first code was likely to loads dpk without version when This new code also forbid |
1398d29
to
6ab3c1f
Compare
|
After @Viech remarks I modified the version check to only check on dpk. I agree it's not meant to change the debian sorting, it is meant to rely on it. |
d94bf94
to
1abbe37
Compare
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.
Did a first pass of the code
src/common/FileSystem.cpp
Outdated
|
|
||
| bool UseLegacyPaks() | ||
| { | ||
| return *fs_legacypaks; | ||
| } | ||
|
|
||
| // Pak zip and directory extensions | ||
| #define PAK_ZIP_EXT ".pk3" | ||
| #define PAK_DIR_EXT ".pk3dir/" | ||
| #define LEGACY_PAK_ZIP_EXT ".pk3" |
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.
We should change these to variables. const char LEGACY_PACK_ZIP_EXT[] = ".pk3";
src/common/FileSystem.cpp
Outdated
| @@ -122,16 +122,18 @@ template<> struct SerializeTraits<FS::LoadedPakInfo> { | |||
|
|
|||
| namespace FS { | |||
|
|
|||
| static Cvar::Cvar<bool> fs_legacypaks("fs_legacypaks", "Don't use versioned pk3 files", Cvar::NONE, false); | |||
| static Cvar::Cvar<bool> fs_legacypaks("fs_legacypaks", "Loads unversioned legacy pk3 files too", Cvar::NONE, false); | |||
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.
Also load pk3s, ignoring version.
src/common/FileSystem.cpp
Outdated
| @@ -1114,7 +1129,8 @@ static void InternalLoadPak(const PakInfo& pak, Util::optional<uint32_t> expecte | |||
| return; | |||
| } | |||
| realChecksum = crc32(*realChecksum, reinterpret_cast<const Bytef*>(&crc), sizeof(crc)); | |||
| if (filename == PAK_DEPS_FILE) { | |||
| // can't reuse isLegacy in lambda | |||
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.
You can if you capture it in the capture stanza like so:
zipFile.ForEachFile([&pak, &realChecksum, &pathPrefix, &hasDeps, &depsOffset, &isLegacy](Str::StringRef filename, offset_t offset, uint32_t crc) {
src/common/FileSystem.cpp
Outdated
| } | ||
|
|
||
| if (type == pakType_t::PAK_ZIP) { | ||
| if (!isdigit(version.at(0))) { |
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.
This shouldn't be required. Our version sorting algorithm accounts for letters in the version.
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 modified that part like this:
if (type == pakType_t::PAK_ZIP) {
+ if (version.at(0) > '9') {
+ fsLogs.Warn("Pak version is very high: '%s'", fullPath);
+ }
fsLogs.Verbose("Found pak: '%s'...", fullPath);It enforces nothing, user is free to make mistakes, but he gets a warning saying a01 (for example) is a very high version number, something that he probably not expect from an alpha version number. Can be helpful.
src/common/FileSystem.cpp
Outdated
| std::string name, version; | ||
| Util::optional<uint32_t> checksum; | ||
|
|
||
| ParsePakName(filename.begin(), filename.end() - suffixLen, name, version, checksum) || (isPakDir && checksum); |
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.
It looks like you don' use this, so why not delete this?
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.
nice catch, removed
src/common/FileSystem.cpp
Outdated
| } | ||
| fsLogs.Verbose("Adding pak: '%s'...", fullPath); |
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.
This log seems redundant compared to the one in InternalLoadPak
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.
They do not have the same purpose and are not printed for the same events. This one is printed when a pak is added to the known list of paks. It will be printed on all pak successfully found. The other one ("loading pak: ") is printed only when the pak is effectively loaded. The idea of this other log line is to allow user to check if his pak is found even if it's not loaded, using +set logs.logLevel.fs verbose or debug.
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.
Ah, I see. Sounds good.
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 modified these lines like this:
- fsLogs.Verbose("Adding pak: '%s'...", fullPath);
+ fsLogs.Verbose("Found pak: '%s'...", fullPath);This way the meaning of this log line looks more obvious.
src/common/FileSystem.cpp
Outdated
| { | ||
| if ( !version.empty() ) { | ||
| // versioned dpk | ||
| return Str::Format("%s.%s", MakePakName(name, version, *checksum), "dpk"); |
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.
PAK_ZIP_EXT, not dpk
src/common/FileSystem.cpp
Outdated
| return Str::Format("%s.%s", MakePakName(name, version, *checksum), "dpk"); | ||
| } else { | ||
| // legacy unversioned pk3 | ||
| return Str::Format("%s.%s", MakePakName(name, version, *checksum), "pk3"); |
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.
LEGACY_PAK_ZIP_EXT, not pk3
|
Just a side note that comes to my mind, currently it's not possible to support some other legacy formats (like pk4) because the code does not store the extension internally. For example the http download code guesses the final path like that (pseudo-code): Making Dæmon being able to handle more than one legacy file extension would require to change many more files and it's outside the scope of this PR. |
402a968
to
2b2c7dc
Compare
|
@DolceTriade I addressed all the issues you pointed out. |
914b52c
to
85c3b0d
Compare
As I already pointed out on the Unvanquished forums:
Mappers should have the full freedom to make errors when they have no idea what a version string is, while people who do know should not have the extra complications. (On a sidenote, I feel |
|
Also, is this PR implying that you simply discard the version of pk3s? That doesn't sound like sufficiently good legacy support, then, since people may want to have multiple pk3s and load only the newest. Ideally pk3s would even use the old ioquake3 version order. |
This PR fixes version sorting and dependency loading for non-legacy paks when legacy pak support is enabled.
This PR tries to fix non-legacy paks, legacy pak support improvement is welcome but out of scope. By the way the changes introduced in this PR can be helpful for someone wanting to improve legacy pak support. |
5b03c8e
to
042fa09
Compare
|
So, last commit fixed legacy pak (pk3) download, since pk3 can't have checksum, the fix was simply to only validate checksum for dpk packages. So, current code was tested successfully with all these scenarii:
If local dpk has wrong checksum, client re-downloads it again and uses the one with right checksum. |
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.
@Kangz Can you take a look?
src/engine/qcommon/files.cpp
Outdated
| if (isDemo || allowRemotePakDir.Get()) | ||
| continue; | ||
| Com_Error(errorParm_t::ERR_DROP, "The server is configured to load game data from a directory which makes it incompatible with remote clients."); | ||
| if (Str::IsSuffix(FS::LEGACY_PAK_ZIP_EXT, x.data())) { |
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.
Move this code inside FS::ParsePakName since you use it twice.
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.
well, in fact not, but I can be used twice. I have some ideas to improve things on that part (and perhaps reduce even more code).
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.
For explanations, currently FS::ParsePakName is a function only used with non-legacy pak, that's why it does not make sense to move this legacy-pak related code inside FS::ParsePakName, but I'm looking for a way to use FS::ParsePakName for every pak.
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 refactored that part, I was also able to remove the AddLegacyPak function I added before… 😃
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.
Smaller the footprint the better :D
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.
Looks good overall, added a couple comments in addition to @DolceTriade's
src/common/FileSystem.cpp
Outdated
| @@ -122,16 +122,18 @@ template<> struct SerializeTraits<FS::LoadedPakInfo> { | |||
|
|
|||
| namespace FS { | |||
|
|
|||
| static Cvar::Cvar<bool> fs_legacypaks("fs_legacypaks", "Don't use versioned pk3 files", Cvar::NONE, false); | |||
| static Cvar::Cvar<bool> fs_legacypaks("fs_legacypaks", "Loads unversioned legacy pk3 files too", Cvar::NONE, false); | |||
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.
nit: Load
src/common/FileSystem.cpp
Outdated
| @@ -1114,7 +1131,8 @@ static void InternalLoadPak(const PakInfo& pak, Util::optional<uint32_t> expecte | |||
| return; | |||
| } | |||
| realChecksum = crc32(*realChecksum, reinterpret_cast<const Bytef*>(&crc), sizeof(crc)); | |||
| if (filename == PAK_DEPS_FILE) { | |||
| // can't reuse isLegacy in lambda | |||
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.
You can by adding isLegacy to the lambda capture list like so: [&pak, ...., &depsOffset, isLegacy]
src/common/FileSystem.cpp
Outdated
| AddLegacyPak(pakType_t::PAK_ZIP, Path::Build(subPath, filename), basePath); | ||
| } else if (useLegacyPaks && Str::IsSuffix(LEGACY_PAK_DIR_EXT, filename)) { | ||
| AddLegacyPak(pakType_t::PAK_DIR, Path::Build(subPath, filename), basePath); | ||
| } else if (Str::IsSuffix("/", filename)) { | ||
| FindPaksInPath(basePath, Path::Build(subPath, filename)); |
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.
Here: https://github.com/DaemonEngine/Daemon/blob/master/src/common/Assert.h#L156 can you add a
#define UNREACHABLE() DAEMON_ASSERT_CALLSITE(false, , false, "Unreachable code hit.")
and add an else clause containing UNREACHABLE();
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 see, I thought about something like that but I was wondering how to implement it, thanks.
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.
done
src/common/FileSystem.cpp
Outdated
| end -= strlen(PAK_ZIP_EXT); | ||
| } else if (Str::IsSuffix(PAK_DIR_EXT, begin)) { | ||
| end -= strlen(PAK_DIR_EXT); | ||
| } |
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.
Same here, add an UNREACHABLE in an else clause.
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.
done
src/common/FileSystem.cpp
Outdated
| } else { | ||
| fsLogs.Notice("Loading legacy pakdir '%s'...", pak.path.c_str()); | ||
| } | ||
| } |
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.
@Kangz do you think I have to add an else UNREACHABLE() there?
In fact everywhere else the code is just doing if PAK_DIR then else (and does not test for PAK_ZIP).
I can also modify the other occurrence with explicit test for PAK_ZIP when not PAK_DIR, plus an else UNREACHABLE. What do you think about it?
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.
There are two choices, either test all cases in else-ifs and add an else clause with UNREACHABLE, or instead of finishing with else if (some condition) {code ...} do else {ASSERT(some condition), code...}. This way if a new case arises, we'll get a warning and know that things should be fixed.
- dpk/dpkdir format for versioned packages - pk3/pk3dir are for legacy unversioned packages - legacy pk3/pk3dir now uses internal version "" so * it's always smaller than a dpk/dpkdir version since dpk/dpkdir forbid this version * code can rely on that version to know if it's a legacy pak, and it now does - the engine now loads dpk as versioned ones and pk3 as legacy unversioned one when fs_legacypaks is set to on, previously the code was loading versioned pk3 as unversioned one when fs_legacypaks was set to on - the engine now prefers the versioned pak over the unversioned one if both exist - this code fixes the autodownload mechanism for legacy pk3s for sure, it was not able to work before since the code was adding version string to legacy pk3s that do not have version string, now the code relies on that special internal "" version to compute legacy pk3 names - modify FS::MakePakeName() to include + extension * engine/qcommon/files.cpp uses it now instead of calling FS::MakePakName() plus adding extension on every FS::MakePakName() call * src/engine/server/sv_client.cpp uses it instead of duplicating Fs:MakePakName() plus adding extension - factorise pak name parsing FS:PakParseName is used to parse both legacy and non-legacy pak names - always look for pak with version, this way, if server host a legacy pak but the client owns both legacy and non-legacy paks sharing the same name, the client is ensured to load the legacy one. - do not compare pk3 checksum legacy paks can't have checksum, so do not check it - ensure dpk paks with bad version are not loaded when fs_legacypaks is enabled - do not read DEPS from legacy paks - enable pk3 downloading Previously, the server was sending pak list to client that way: [ name1_version_checksum, name2, n_a_m_e_3, name4_version_checksum, ] So, the client was not able to parse this names with dpk/pk3 specificities, and to know the file format. Now, the server sends pak list to client that way: [ name1_version_checksum.dpk, name2.pk3, n_a_m_e_3.pk3, name4_version_checksum.dpk, ] Extension is used by client to know how to parse these names (pk3 have empty version and name can't contain checksum) and reconstruct final file name in local file system. On local file system the package can be a dpkdir or pk3dir, that information is not told in that list since there is no need to tell it.
was a nice kangz's idea extra fix: there was some places where zip format was guess testing if it was not a dir, since we don't trust pak.type the code now guess if it's a zip by testing if it's a zip
|
@Kangz I also modified some existing lines that were testing if a pak was a zip one by checking if it wasn't a dir one. Since you recommended to assert when a pak is not a zip and not a dir, I assume it's not good to trust a pak to be a zip just because it's not a dir. So, these lines now check if a pak is a zip by checking if it's a zip (bonjour Lapalisse). |
|
I squashed all the commits related to dpk support (because some were adding code the later removed). I left the one with UNREACHABLE asserts unsquashed since it acts as a very good standalone implementation example. I verified this code works with both udp and http download with both dpk and pk3 archives. |
|
Thanks @illwieckz, LGTM |
|
So, since this PR is now approved, is there an agenda to merge this PR? Note: once this PR is merged, git Dæmon client will only be able to connect properly do git Dæmon server. |
|
Yes, I would strongly prefer this to be merged into another branch. As soon as we prepare our next release, we will merge this in including the new paks based on your asset repo. |
|
So I just merged it in for-0.51.0. |
See this forum post for the reasons and explanations behind this change.
See this merge request for related changes in NetRadiant/q3map2.
What this PR does:
dpk/dpkdir format for versioned packages
pk3/pk3dir are for legacy unversioned packages
legacy pk3/pk3dir now uses internal version "" so
the engine now loads dpk as versioned ones and pk3 as legacy unversioned one
when fs_legacypaks is set to on, previously the code was loading versioned pk3
as unversioned one when fs_legacypaks was set to on
the engine now prefers the versioned pak over the unversioned one if both exist
this code fixes the autodownload mechanism for legacy pk3s
for sure, it was not able to work before since the code was adding version string
to legacy pk3s that do not have version string, now the code relies on that
special internal "" version to compute legacy pk3 names
modify FS::MakePakeName() to include + extension
plus adding extension on every FS::MakePakName() call
plus adding extension
factorise pak name parsing
FS:PakParseName is used to parse both legacy and non-legacy pak names
always look for pak with version, this way, if server host a legacy pak
but the client owns both legacy and non-legacy paks sharing the same name,
the client is ensured to load the legacy one.
do not compare pk3 checksum
legacy paks can't have checksum, so do not check it
ensure dpk paks with bad version are not loaded when fs_legacypaks is enabled
do not read DEPS from legacy paks
enable pk3 downloading
Previously, the server was sending pak list to client that way:
[
name1_version_checksum,
name2,
n_a_m_e_3,
name4_version_checksum,
]
So, the client was not able to parse this names with dpk/pk3
specificities, and to know the file format.
Now, the server sends pak list to client that way:
[
name1_version_checksum.dpk,
name2.pk3,
n_a_m_e_3.pk3,
name4_version_checksum.dpk,
]
Extension is used by client to know how to parse these names (pk3 have
empty version and name can't contain checksum) and reconstruct final
file name in local file system. On local file system the package can
be a dpkdir or pk3dir, that information is not told in that list since
there is no need to tell it.
So, this PR not only introduces
dpkformat, it is also meant to fix autodownloading legacypk3that was broken before, and factorize some code.