Make tests optional + fix option declarations #366
Conversation
Codecov ReportPatch coverage has no change and project coverage change:
Additional details and impacted files@@ Coverage Diff @@
## master #366 +/- ##
==========================================
- Coverage 89.39% 85.37% -4.03%
==========================================
Files 33 36 +3
Lines 3979 4375 +396
==========================================
+ Hits 3557 3735 +178
- Misses 422 640 +218
... and 14 files with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
|
I suspect these windows and mac failures are due to |
|
Interesting, I did not pay much attention to the Mac and Windows CI. I think we did not install python headers for those jobs? It would be nice if we can install them and do a test for python binding. |
|
Setting |
|
Investigating the error being hit in the mac build, I found an issue that makes it look like it is due to newer python version deprecations in I wonder if this is something that has already been remedied in newer pybind11 commits (eventually the default python will increment in the other systems). |
|
The mac CI is succeeding with |
Break out the MeshIO from the tests, and make building the tests optional. In cases where building/re-building the tests is not wanted/needed, this can shorten build times by over 25% on my machine. A major motivator for this is building 3rd party bindings. Also included here are descriptors for the option declarations, which I noticed do not actually set the default value as expected if the description argument is omitted (defaults to OFF, rather than ON for _TEST and _PYBIND for example -- on my machine anyway).
Testing that a deprecation in 3.11 is indeed the source of the breakage (other CI systems are not on such a recent python). If this works, we should probably see about updating pybind if they have corrected for this already.
|
🎉 the mac is building |
| add_subdirectory(bindings) | ||
|
|
||
| if(MANIFOLD_EXPORT) | ||
| add_subdirectory(meshIO) |
There was a problem hiding this comment.
I'm still a bit concerned about promoting meshIO out of the test directory. I really can't stand behind it - export is decent, but import is very shaky. Formats are a lot of work - we need to careful about the promises we make. What do you think our goal should be here?
There was a problem hiding this comment.
I haven't personally used it (or assimp) for import, so I have not experienced that shakiness, but I still feel having something ready to go to get designs exported is valuable (even if it is an off by default library like meshIO). If people want to try out manifold for making models to 3d print etc, it's a bit of a hurdle to then have to put together their own export solution. Requiring learning more about the library, and springing a leak in the otherwise pretty friendly abstraction that let them build their thing.
I know that you fear the presence of meshIO creating a slipperly slope into being asked to support more and more export features, I'm still of the mind that not having something available and discoverable makes Manifold less approachable. Not having the assimp wrapper of meshIO might even drive more people into the arms of stl (which I know you have a great distaste for 😉) if they decide not to depend on another library, and just write the darn mesh to file in the simplest way possible (the only export I've added to OCADml so far is ASCII and binary STL -- I know, I know).
There was a problem hiding this comment.
Yeah, that's a good point, particularly about driving users into the arms of STL. @hrgdavor I'd say 3MF is probably the best, though I'm considering an extension for glTF that would make it equally good. Maybe what this really means is we should put enough effort into meshIO that it does a decent job.
Let's go ahead with this change and take it as an action to improve the I/O.
@elalish I am only watching from the side-lines, so I do not know many details. What is the recommendation for most efficient import of meshes (closest to internals for fast, and also reliable import that does not need fixing like STL)? just a thought as an outsider, .. if meshIO is useful, and used for tests, but is not up-to snuff to be part of manifold, maybe separate it in a lib that has more limited support for issues than manifold. This would make it available to those eager to use it regardless. You would avoid the burden of supporting it too much. |
Break out the MeshIO library from the tests subdirectory, and make building the tests optional. In cases where building/re-building the tests is not wanted/needed, this can shorten build times by over 25% on my machine. A major motivator for this is building 3rd party bindings. Also included here are descriptors for the option declarations, which I noticed do not actually set the default value as expected if the description argument is omitted (defaults to OFF, rather than ON for `_TEST` and `_PYBIND` for example).
This breaks out the MeshIO from the tests, and makes building the tests optional. In cases where building/re-building the tests is not wanted/needed, this can shorten build times by over 25% on my machine. A major motivator for this is building 3rd party bindings using the C FFI bindings (that include an export function for convenience).
Also included here are descriptors for the option declarations, which I noticed do not actually set the default value as expected if the description argument is omitted (defaults to OFF, rather than ON for
_TESTand_PYBINDfor example -- on my machine anyway).