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

Header list is missing SavitarExport.h #2

Closed
onitake opened this issue Apr 27, 2017 · 7 comments
Closed

Header list is missing SavitarExport.h #2

onitake opened this issue Apr 27, 2017 · 7 comments

Comments

@onitake
Copy link
Contributor

onitake commented Apr 27, 2017

ThreeMFParser.h includes SavitarExport.h, but this file is not installed by default.

As a result, compiling anything against libSavitar outside the tree will fail.

Please add the include file to savitar_HDRS in CMakeLists.txt .

@onitake
Copy link
Contributor Author

onitake commented Apr 27, 2017

Looks like I spoke too soon, this causes a dependency problem with the Python.h include.

Maybe this should be moved to the .cpp sources instead?

@onitake
Copy link
Contributor Author

onitake commented Apr 27, 2017

I think this issue could be solved by changing the types of the getVerticesAsBytes, getFacesAsBytes and getFlatVerticesAsBytes methods in class MeshData, thus removing any leftover Python dependencies from the C++ library.

Are you using these anywhere outside the Python bindings?
If not, I'd like to propose a patch that changes PyObject* to std::vector and adds a suitable MappedType in Types.sip.

@nallath
Copy link
Member

nallath commented May 1, 2017

Cura is the only one that uses it right now.

@onitake
Copy link
Contributor Author

onitake commented May 1, 2017

The reason why I brought this up is because I ran into some errors when I making Debian packages of libSavitar. The Python symbols would make the resulting -dev package depend on libpython3.5-dev and require additional compilation arguments. This is not appropriate for a C++ library, I think.

So I moved all Python stuff into the SIP library, and this does in fact work fine. I tested it against Cura 2.5.0.

@LipuFei
Copy link
Contributor

LipuFei commented May 1, 2017

@nallath @onitake In this case, perhaps separating this into C++ library code and python bindings code would be a better solution (of course, more work is required). The current changes in #3 will break Cura unfortunately...

@onitake
Copy link
Contributor Author

onitake commented May 1, 2017

@LipuFei They actually won't, since only the C++ parts are changed. The Python3 API is still the same.

I verified this, Cura 2.5 works fine with the changes.

@onitake
Copy link
Contributor Author

onitake commented May 4, 2017

My PR has been merged and I ran some tests.

Compiling out-of-tree C++ against libSavitar works fine now and the Python bindings are still ABI compatible.

Closing this issue.

@onitake onitake closed this as completed May 4, 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