Skip to content

Conversation

7H3LaughingMan
Copy link

This is a duplicated of #81 but targeting the dev branch, also includes testing for base64 and the various compression algorithms.

@7H3LaughingMan 7H3LaughingMan mentioned this pull request Apr 27, 2025
@dcronqvist
Copy link
Owner

dcronqvist commented Apr 29, 2025

Hi! Looks interesting, but I am wondering if there really is any need for zstd support in DotTiled. While I'm sure it's a widely used compression algorithm, I'm not too sure that there would be a wide adoption among users of DotTiled.

There's already gzip and zlib as supported compression algorithms, both of which come with built-in support in .NET. Supporting zstd would mean that DotTiled would need to depend on something like ZStdSharp which itself depends on prebuilt binaries (which, if I am not mistaken, only exist for windows currently?).

I would prefer that users of DotTiled would use one of the supported alternative compression algorithms instead.

With that being said, there are test cases in this PR which are very useful - all of which should be added!

  • default-map-base64
  • default-map-base64-gzip
  • default-map-base64-zlib

If you want to amend your PR to align with this comment, I'd be more than happy to merge it, but unfortunately zstd is not something I'm planning on supporting unless there is significant push from the community.

Also, have a look at the new contribution guidelines! Rebase the branch on dev rather than merging dev into it. PRs aren't squashed when merged to master, so to maintain a linear history, I prefer to rebase and fix merge conflicts rather than merging the target branch.

@7H3LaughingMan
Copy link
Author

While I agree zstd support is probably not going to have wide adoption, I still feel it's great to have to cover all the bases. ZStdSharp does not rely on any prebuilt binaries which is the reason for picking it. The author pretty much rewrote the library in C# and it's only dependencies are Microsoft libraries if you using it in .NET Framework, .NET Standard, or .NET Core projects. Everything after .NET 5.0 does not require any dependencies.

If you take a look at the project GitHub the only time it uses prebuilt binaries is for benchmarking and testing purposes.

@7H3LaughingMan
Copy link
Author

7H3LaughingMan commented Apr 29, 2025

Also, went and rebased it for now. If you still aren't interested in ZStd support let me know, I can close out this PR and put in a new one for a different branch that just has the testing for everything else.

@dcronqvist
Copy link
Owner

I hear you, let's leave this PR up for now as I make sure to get myself more acquainted with zstd and ZStdSharp. I'll continue the discussion in the issue you opened #95. Create a different PR that contains the mentioned test cases, and remove them from this one, so we can keep things isolated.

Much appreciated :)

@7H3LaughingMan
Copy link
Author

7H3LaughingMan commented Apr 29, 2025

Created #96 for Base64, GZip, and ZLib testing then removed them from this PR. Once that gets pulled in I will probably have to rebase this

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.

2 participants