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

Miniz/zlib #46

Closed
john-chapman opened this issue Oct 4, 2017 · 10 comments
Closed

Miniz/zlib #46

john-chapman opened this issue Oct 4, 2017 · 10 comments

Comments

@john-chapman
Copy link

I am already using Miniz in my project; I therefore want to do this to disable the embedded version of Miniz:

#define TINYEXR_IMPLEMENTATION
#define TINYEXR_USE_MINIZ 0
#include "miniz.h"
#include "tinyexr.h"

However, tinyexr.h wants to include zlib.h:

#if TINYEXR_USE_MINIZ
#else
#include "zlib.h"
#endif

My opinion is that it would be preferable to remove this dependency and leave it up to the user to include their zlib-compatible header (either zlib.h or miniz.h, as in my case) before including tinyexr.h.

@syoyo syoyo closed this as completed in 620bf09 Oct 4, 2017
@syoyo
Copy link
Owner

syoyo commented Oct 4, 2017

My opinion is that it would be preferable to remove this dependency and leave it up to the user to include their zlib-compatible header (either zlib.h or miniz.h, as in my case) before including tinyexr.h.

This looks good so I made a change in the recent commit.

Thank you for reporting an issue!

@zigguratvertigo
Copy link
Contributor

I've also made similar changes on my side, where we have other modules in our code relying on miniz already.

We've basically removed lines 475 to 6909, and now only rely on:

#if TINYEXR_USE_MINIZ
#include <miniz/miniz.h>
#else
#include "zlib.h"
#endif

@syoyo
Copy link
Owner

syoyo commented Oct 4, 2017

we have other modules in our code relying on miniz already.

Okay, I'm considering to introduce TINYEXR_USE_EXTERNAL_MINIZ define, which assumes user code already includes miniz header.

@john-chapman
Copy link
Author

I don't see that it's necessary to add a new define, it's simpler just to disable the embedded version and require that the user included the header (or a compatible one) prior to including tinyexr.h. This relies on Miniz keeping a zlib-compatible interface but that's probably a safe assumption.

A key point for me is that I'd prefer not to have to modify tinyexr.h at all so that I can just drop in any updates.

@syoyo
Copy link
Owner

syoyo commented Oct 4, 2017

I don't see that it's necessary to add a new define, it's simpler just to disable the embedded version and require that the user included the header (or a compatible one) prior to including tinyexr.h

I see. Let me give some time to figure out what is the best way to solve the issue with minimal modification.

@syoyo syoyo reopened this Oct 5, 2017
@syoyo
Copy link
Owner

syoyo commented Oct 5, 2017

Do you need miniz specific API for your project, @zigguratvertigo?

If you are ok to use zlib-compatible API, as @john-chapman suggests, there is no modification required anymore and you can include external miniz.h as follows.

#include <miniz/miniz.h> // Use zlib compatible API

#define TINYDNG_USE_MINIZ (0) // Disable embedded miniz code in tinyexr.h
#include "tinyexr.h"

@john-chapman
Copy link
Author

@syoyo One additional problem I noticed: there's a block of #pragma warning starting line 4377 which is disabled with TINYEXR_USE_MINIZ, but probably shouldn't be.

@syoyo
Copy link
Owner

syoyo commented Oct 6, 2017

I see > there's a block of #pragma warning starting line 4377 which is disabled with TINYEXR_USE_MINIZ, but probably shouldn't be.

@syoyo
Copy link
Owner

syoyo commented Oct 6, 2017

Fixed wrong pragma warning push/pop pair in this commit: 13a6b15

@syoyo
Copy link
Owner

syoyo commented Oct 20, 2017

Plese reopen the issue if you still get a problem.

@syoyo syoyo closed this as completed Oct 20, 2017
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

No branches or pull requests

3 participants