Details

Reviewers

Group Reviewers

Restricted Owners Package

(Owns No Changed Paths)

Summary

Currently we bundle xml files with the mods, while only XMB should be used. (If they don't work that's another issue) For the main game packaging or not makes a difference of about 40MB when archive is compressed, but for some mods it can be up to 50% of the mod size, which is non negligible. This patch adds a flag to the game, to allow users not to bundle them.

This is also have the advantage of making mods a bit more hack proof, as it's harder to get the XML code. This would be useful for proprietary mods, were the case the arrive

Test Plan

Test that XML files are not bundled anymore, and that the rest still works fine.

pyrogenesis.exe -mod=no-blood-and-gore-mod -archivebuild=..\data\mods\no-blood-and-gore-mod -archivebuild-compress -archivebuild-output="no-blood-and-gore-mod.2.pyromod" -archivebuild-ignorexml

Diff Detail

Repository

rP 0 A.D. Public Repository

Lint

Lint Skipped

Unit

Unit Tests Skipped

Build Status

Buildable 7274
Build 11851: Vulcan Build	Jenkins

Event Timeline

Stan created this revision.Jun 22 2018, 11:10 AM

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/differential/665/display/redirect

Harbormaster completed remote builds in B6267: Diff 6783.Jun 22 2018, 11:14 AM

Stan added a parent revision: D1585: Show an end of packaging confirmation message..Jun 22 2018, 11:53 AM

Stan removed a parent revision: D1585: Show an end of packaging confirmation message..

Stan added a child revision: D1585: Show an end of packaging confirmation message..

I wonder about consistency: We don't offer a way to package png files but only the dds files are zipped, same with the other converted files (DAE). So I wonder why this one is optional when the other ones force the cached contents.

source/ps/ArchiveBuilder.cpp
146	Perhaps we need this comment only once at the top, not twice as it is now or adding a third copy

In D1586#63680, @elexis wrote:

I wonder about consistency: We don't offer a way to package png files but only the dds files are zipped, same with the other converted files (DAE). So I wonder why this one is optional when the other ones force the cached contents.

I guess the reason is given by the comment. one can also notice that DDS files are not copied, only their cached.dds variants which is nice. Bundled versions both shouldn't need any recaching anyway them so there shouldn't be another loading.

I think XMB have some limitations when it comes to XML but it's up to us to make sure it never comes to those limitations.

source/ps/ArchiveBuilder.cpp
146	Maybe though, it's more relevant for DAE's and PNG than it is for XML as those are usually way bigger than XML.

Should be enabled by default if it's truly not broken.
Don't see why this one type of cached file should be treated differently. Could be one option per cached file that is enabled by default, or no options at all for excluding convertable source files (but it could also be useful to distribute source files (One may also think about options to cache at all then.)).

source/ps/ArchiveBuilder.cpp
146	Well you gave the argument that its reelvant for XML files
source/ps/ArchiveBuilder.h
1	8

In D1586#63685, @elexis wrote:

Should be enabled by default if it's truly not broken.

It seems to be broken for templates, but I need to check the packaged version, maybe it's only broken for SVN. Else I suspect the js code doesn't load templates the way the c++ code does.

In D1586#63685, @elexis wrote:

Don't see why this one type of cached file should be treated differently. Could be one option per cached file that is enabled by default, or no options at all for excluding convertable source files (but it could also be useful to distribute source files (One may also think about options to cache at all then.)).

So a cache/ no cache switch instead ?

source/ps/ArchiveBuilder.cpp
146	True but mostly for mods :)
source/ps/ArchiveBuilder.h
1	Ah right

I agree with @elexis, we don't need to prefer the one type of files. The compressed version of a mod is less portable. The security reason is nothing for the open-source project, because you can just modify the code a little bit to easily extract things.

source/ps/ArchiveBuilder.cpp
1	`2018`.
62	`ignoreXML`.
173	For `debug_printf` above the file name is used without quotes. It should have the one style here. Probably `''` quotes.

It also has the nice effect to make people download the last version of the mod instead of hacking the zip file and complaining.

@elexis @vladislavbelov so do you agree it should be a flag like -archivebuild-include-non-cached-files that is all or nothing ?

In D1586#63688, @Stan wrote:

It seems to be broken for templates, but I need to check the packaged version, maybe it's only broken for SVN.

Else I suspect the js code doesn't load templates the way the c++ code does.

JS has no file loading functions that don't go through C++ and C++ only has Xeromyces as far as I know.

In D1586#63695, @Stan wrote:

It also has the nice effect to make people download the last version of the mod instead of hacking the zip file and complaining.

If people are doing that, agree, they shouldn't.

So a cache/ no cache switch instead ?

At least exclude XML by default where it is unneeded.

a flag like -archivebuild-include-non-cached-files that is all or nothing ?

I don't know how many users there are using this or more finegrained flags.
No flags, one flag, multiple flags are something that can be added whenever someone sees motivation to add it.

So the underlying problem why this doesn't work in some places is that some places iterate over all XML files of a given directory. We can identify the places by searching for "\*.xml" in the code.
These are CGUIManager::LoadPage, CTerrainTextureManager::LoadTerrainTextures, CColladaManager LoadSkeletonDefinitions, PrepareCacheKey, CGUI::Xeromyces_ReadObject, possibly more.

While this can be changed to look for both XML and XMB, there can also be unintended side effects from doing so.

Consider someone installs a mod with file1.xml and file2.xml. They will be converted to file1.xmb and file2.xmb.
Then consider this someone installs a new version of the mod where file2.xml was deleted. The cached file still exists.
If now something loads all *.xml and *.xmb files, it will load both files, but the mod only contains the first XML file.

I suspect that there is no code iterating over texture or collada files, and hence it was possible to leave out the source files for these types without this "outdated cache" problem.

I'm not sure what can be done to alleviate this problem other than deleting cache files if there is no equal XMB or corresponding XML file in the zip archive or source directory.

As we discussed XML is a pretty specific case and it may have some problems. But I can suggest to add a more common parameter, like an ignore mask. Example: