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 workaround for QuantumRange not defined error when hdri is disabled #68

Merged
merged 1 commit into from
Jun 11, 2020

Conversation

captainbland
Copy link
Contributor

Relates to: #40

Not sure if this is the optimal solution (excluding the fix in bindgen of course which, of course, is) or whether you think adding this kind of workaround is a good idea but I've done it for my own benefit so maybe others would find it useful.

This defines a feature which the user of the library can enable in their project in the case that they're using a build of ImageMagick with --disable-hdri compiled in.

It just uses the same values defined in magick-type.h for each QuantumDepth to the best of my understanding although this may be worth double checking as part of the review. Tests pass with such an ImageMagick build if you run the tests as cargo test --features=disable-hdri.

Things to note are: Introduces a Result object when accessing the QuantumRange and I've not attempted to do anything clever with the types as implied in the magick-type.h file casting it to a Quantum which itself is another type and so on... I've just gone with f64. I've only gone as far as validating that you can still produce a convincingly sepia toned image (with a value of 0.65) with hdri disabled at Q8 with this change.

@captainbland captainbland marked this pull request as draft June 9, 2020 18:58
@captainbland
Copy link
Contributor Author

Converting to draft because I think there's actually a better solution

@nlfiedler
Copy link
Owner

I don't have any objection to the initial change, but I'm happy to learn from whatever you suggest, as this is an area of Rust I have not explored before. Thanks.

@captainbland
Copy link
Contributor Author

captainbland commented Jun 10, 2020

I've not really looked at it that much before either to be honest with you which is why I've kind of weighed up our options with some deliberation

But basically I was considering manipulating bindgen into generating the constants conditionally, rather than wrapping them with methods with conditional compilation.

One way to do this is to put the C preprocessor definitions in the header file that bindgen loads, which has the nice property of not requiring people to specify a feature when they include the library. It works but seeing it in action I think it's a bit of a hack since that file is intended for just adding includes whereas the version in this PR is a bit more idiomatically rust-y. But it would be more convenient for the library user.

I looked for other mechanisms to do it with bindgen but didn't find anything suitable.

So I guess there's a bit of a trade-off there, but if you're happy with this version then I am

@captainbland captainbland marked this pull request as ready for review June 10, 2020 17:35
@nlfiedler nlfiedler merged commit ce36ba8 into nlfiedler:master Jun 11, 2020
@nlfiedler
Copy link
Owner

Terrific, thanks!

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