search

nlfiedler / magick-rust

Rust bindings for ImageMagick
https://crates.io/crates/magick_rust
Apache License 2.0
254 stars 68 forks source link

Add workaround for QuantumRange not defined error when hdri is disabled #68

Closed captainbland closed 4 years ago

captainbland commented 4 years ago

Relates to: https://github.com/nlfiedler/magick-rust/issues/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 commented 4 years ago

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

nlfiedler commented 4 years ago

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 commented 4 years ago

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

nlfiedler commented 4 years ago

Terrific, thanks!