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

Use generic qhull include location #2319

Merged
merged 1 commit into from May 22, 2019
Merged

Conversation

jasontibbitts
Copy link
Contributor

Currently the qhull includes are referenced absolutely, but the compiler is always called with the src directory in the include path and so it should be safe to specify a more generic path.

Fedora doesn't have the same issues with the qhull package that Debian has, and so we would like to use the system version of the library. Changing the includes in this manner should still work fine when qhull is not installed (as I've tested). I believe it should also still work with the Debian package, though I cannot test that as I do not use Debian.

Currently the qhull includes are referenced absolutely, but the compiler
is always called with the src directory in the include path and so it
should be safe to specify a more generic path.
@bubnikv
Copy link
Collaborator

bubnikv commented May 22, 2019

this will certainly break our static build.

@bubnikv bubnikv merged commit ef028cd into prusa3d:master May 22, 2019
@bubnikv
Copy link
Collaborator

bubnikv commented May 22, 2019

I think I was wrong, it will compile. @lukasmatena will in addition modify the cmake scripts, so that we will handle the qhull library the same way we handle expat.

By the way, there is a note by @lukasmatena in the README of the bundled qhull, where he documents that he changed the REALfloat define, so that our qhull compiles with floats, not doubles. I suppose the qhull that uses doubles will link just fine with PrusaSlicer, only the compiler will emit some additional warnings.

This distribution of qhull library is only meant for interfacing qhull with Slic3rPE
(https://github.com/prusa3d/PrusaSlicer).

The qhull source file was acquired from https://github.com/qhull/qhull at revision
f0bd8ceeb84b554d7cdde9bbfae7d3351270478c.

No changes to the qhull library were made, except for

  • setting REALfloat=1 in user_r.h to enforce calculations in floats
  • modifying CMakeLists.txt (the original was renamed to origCMakeLists.txt)

Many thanks to C. Bradford Barber and all contributors.

Lukas Matena (lukasmatena@seznam.cz)
25.7.2018

@jasontibbitts
Copy link
Contributor Author

Yes, the REALfloat=1 thing is indeed an issue; if you don't adapt for that, you will get compilation failures because (at least with the compilation flags Fedora uses) there is no automatic conversion from doubles to floats.

The qhull library is somewhat poorly written but if you patch the system version of user_r.h with ifndef guards around the various definitions then you can work around it. Another possibility is to copy the system version and patch it. Or... just not bother with it for a relatively obscure, header-only library. But none of that that's not really your concern anyway; that's on the distribution maintainers.

@lukasmatena
Copy link
Collaborator

Maybe the best thing to do would be to switch back to REALfloat=0, let qhull use doubles internally and just do the extra conversions in our code, where the qhull output is passed to PrusaSlicer. I'm worried that otherwise it will continue to be a maintenance issue. I doubt it will hurt much as for performance, although we could do some profiling before.

@bubnikv Should I do the change?

@bubnikv
Copy link
Collaborator

bubnikv commented May 23, 2019

I don't know. We need to evaluate the effects on performance. We may make the possible double to float to double conversion compile time dependent.

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.

None yet

3 participants