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

Add Zstandard + compression level #1888

Merged
merged 6 commits into from Jul 10, 2019

Conversation

@alphaonex86
Copy link
Contributor

alphaonex86 commented Feb 20, 2018

No description provided.

@bjorn

This comment has been minimized.

Copy link
Owner

bjorn commented Mar 7, 2018

Hmm, I can't accept a change that "presumes zstd library is installed". Either the zstd source code needs to be shipped or building with zstd support should be optional.

src/libtiled/compression.cpp Outdated Show resolved Hide resolved
src/libtiled/compression.cpp Outdated Show resolved Hide resolved
@alphaonex86

This comment has been minimized.

Copy link
Contributor Author

alphaonex86 commented Mar 7, 2018

Hi, fixed

@Oipo

This comment has been minimized.

Copy link
Contributor

Oipo commented Jul 5, 2019

Appreciate the work done here by both of you, but I would really like this functionality. Is this something that will ever get merged into tiled? Otherwise I'll just fork tiled and keep it up to date myself.

@bjorn

This comment has been minimized.

Copy link
Owner

bjorn commented Jul 5, 2019

@Oipo So far I was rather reluctant to add this since I've heard nobody but @alphaonex86 about this and additional options do come at a maintenance cost. But you're the second person asking for this compression format, so maybe it's worth supporting it, especially since apparently the difference between zlib and zstd is significant enough that you'd maintain your own fork for it.

That said, this patch makes it an optional build option and zstd will need to be available on the system when compiling Tiled, and probably will also require shipping additional files (unless it's statically linked). Any help getting zstd supported in the macOS, Windows and Linux builds would be very appreciated!

@alphaonex86

This comment has been minimized.

Copy link
Contributor Author

alphaonex86 commented Jul 5, 2019

Hi, I can help you to build with zstd on all OS, it's what's I do (include web assembly).
On Mac: brew install zstd
On linux debian: libzstd-dev
On Windows: make;make install

Forward compression level to top method

Last fix

Fix compression method, now the zstd load and save correctly

Try fix compression level but failed

Try more

Fix for gzip

Do zstd optional, some improvements
@Oipo

This comment has been minimized.

Copy link
Contributor

Oipo commented Jul 5, 2019

@alphaonex86 I fixed the merge conflicts and pushed some fixes to my fork: https://github.com/Oipo/tiled/commits/master

Perhaps you want to include those as well?

@bjorn bjorn force-pushed the alphaonex86:master branch from cb76649 to 61016c4 Jul 5, 2019
@bjorn

This comment has been minimized.

Copy link
Owner

bjorn commented Jul 5, 2019

@Oipo I've fixed up the merge conflicts as well and pushed it into @alphaonex86's master branch. I shall cherry-pick your fixes on top.

@Oipo

This comment has been minimized.

Copy link
Contributor

Oipo commented Jul 5, 2019

@bjorn Adding zstd to the linux build/travis file is quite easy, but I don't know how to do it for the appveyor file. I assume that's the automated build for windows? Also, where is the build file for mac builds?

* Preprocessor checks start on first column
* Space between "if(" and after comma, "{" not on next line after "if"
* Compression level "int" instead of "unsigned int"
* Compression level -1 used to mean "default for compression method"
* Default compression method back to Zlib
* Don't arbitrarily refuse to set compression in Map::setCompressionLevel
* "compressionlevel" -> "compressionLevel"
* Don't pass member variable as argument to member function call
  in MapWriterPrivate
* Fixed applying of compression level to child layers in Lua export
@bjorn

This comment has been minimized.

Copy link
Owner

bjorn commented Jul 5, 2019

@Oipo Right, the appveyor.yml makes the Windows builds (32-bit, 64-bit and WinXP as per the "environment.matrix" entries). The macOS builds happen on Travis CI (the os: osx section).

I've just pushed a bunch of small cleanups. Note that also, the "optional" nature of this patch currently requires commenting out lines in the libtiled.pro file. That should of course be changed to an actual build option. Also, actually I don't use qmake anywhere except for the snap builds (which are another thing to update when we want zstd supported there, see snap/snapcraft.yaml). The Qbs project files should be updated as well to add this option.

@bjorn

This comment has been minimized.

Copy link
Owner

bjorn commented Jul 5, 2019

@alphaonex86 Why is a separate buffer allocated here which is then copied from, rather than compressing into a QByteArray directly as is done for the gzip/zlib code path?

        size_t const cBuffSize = ZSTD_compressBound(data.size());

        void* const cBuff = malloc(cBuffSize);
        if (!cBuff) {
            qDebug() << "error to alloc" << cBuffSize;
            return QByteArray();
        }

        size_t const cSize = ZSTD_compress(cBuff, cBuffSize, data.constData(), data.size(), compressionLevel);
        if (ZSTD_isError(cSize)) {
            qDebug() << "error compressing:" << ZSTD_getErrorName(cSize);
            return QByteArray();
        }

        QByteArray data(static_cast<char *>(cBuff), cSize);
        free(cBuff);
@bjorn

This comment has been minimized.

Copy link
Owner

bjorn commented Jul 5, 2019

As one can see this PR required still a bit of work, and we're not done yet since it's not trivial to adjust the build scripts on each platform as well as the Windows installer. I have to pause my efforts here, possibly until Tuesday or Thursday next week depending on whether there is time available in the evenings.

@bjorn bjorn added this to Tiled 1.3 (next feature release) in Roadmap Jul 5, 2019
@alphaonex86

This comment has been minimized.

Copy link
Contributor Author

alphaonex86 commented Jul 5, 2019

@alphaonex86 Why is a separate buffer allocated here which is then copied from, rather than compressing into a QByteArray directly as is done for the gzip/zlib code path?

I don't remember why now. I think it should be changed to QByteArray

@Oipo

This comment has been minimized.

Copy link
Contributor

Oipo commented Jul 5, 2019

@bjorn Alright.

In the meantime, I've managed to make ZSTD configurable in qmake as well as qbs and integrate it into the linux builds. See the PR I opened. I think that for OSX, and possibly Windows as well, we'll have to download and compile the zstd source ourselves.

@bjorn bjorn force-pushed the alphaonex86:master branch from c947be1 to 7d24c3c Jul 10, 2019
bjorn and others added 2 commits Jul 5, 2019
* Introduced Tiled::compressionToString to share creation of compression
  method string based on layer data format.

* Hide "Base64 (Zstandard compressed)" in Properties window and in the
  New Map dialog when this compression method is not supported.
* Make zstd configurable in qbs/qmake
* Have linux builds compile with Zstd
* Directly use QByteArray for Zstd compression
* Linux automated builds
* OSX automated build
* Windows automated build
@bjorn bjorn force-pushed the alphaonex86:master branch from 7d24c3c to 540559c Jul 10, 2019
@bjorn bjorn merged commit 540559c into bjorn:master Jul 10, 2019
1 of 2 checks passed
1 of 2 checks passed
continuous-integration/appveyor/pr AppVeyor was unable to build non-mergeable pull request
Details
WIP Ready for review
Details
Roadmap automation moved this from Tiled 1.3 (next feature release) to Recently Completed Jul 10, 2019
bjorn added a commit that referenced this pull request Jul 10, 2019
Add Zstandard + compression level

Closes #1886

Conflicts:
	src/libtiled/maptovariantconverter.cpp
	src/libtiled/maptovariantconverter.h
	src/libtiled/mapwriter.cpp
	src/plugins/lua/luaplugin.cpp
	src/tiled/changemapproperty.cpp
	src/tiled/changemapproperty.h
	src/tiled/propertybrowser.cpp
	src/tiled/propertybrowser.h
@bjorn

This comment has been minimized.

Copy link
Owner

bjorn commented Jul 24, 2019

@Oipo Hmm, something regarding the static linking appears to not work properly. All Windows builds now require a zstd.dll file, which is not included in the distribution. So at the moment the latest development snapshot fails to launch. I see you're doing cpp.staticLibraries: ["libzstd"], so I'm not sure why it anyway ended up needing the dynamic library. Any idea?

@Oipo

This comment has been minimized.

Copy link
Contributor

Oipo commented Jul 24, 2019

That's weird. I'll check around on appveyor.

@bjorn

This comment has been minimized.

Copy link
Owner

bjorn commented Jul 24, 2019

I noticed that the output after "compiling static library" is empty in the AppVeyor build output.

@Oipo

This comment has been minimized.

Copy link
Contributor

Oipo commented Jul 24, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Roadmap
  
Recently Completed
3 participants
You can’t perform that action at this time.