simplify directory structure for compatibility with Swift Package Manager

[wtholliday]

This is a WIP and I'm not good with CMake so it won't yet build, but it's probably not that far off.

This PR moves cpp files into a single directory (src) and headers into another (src/include/manifold).

The directory structure had multiple include/manifold directories which confused Swift Package Manager when locating headers. Its pretty weird (to say the least) to see in code: #include "manifold/manifold.h" and #include "manifold/polygon.h" and actually have those files in different directories!

There were also three CMakeLists files which were mostly cut & paste it seems.

One of the modules (collider) was even just a single header.

There's not actually that much code here (11k loc) so why split things up into multiple targets and complicate things? KISS rule applies. But if you're really attached to the multiple targets, I could probably add a few source files (SwiftPM needs at least one .cpp file) and make it work, or at least try.

I should mention that one problem with the SwiftPM stuff is that the dependencies also have to be in SwiftPM as well, and I did a quick job of that, linking to my own stuff for glm and clipper2.

Anyway I'm sorry if I come off as negative... this is really an excellent project and I'm ULTRA IMPRESSED with what you've accomplished :)

[elalish]

Since there are conflicts, I'm guessing you didn't pull our master first? Also, since Github's diff view is not great for reorgs, would you mind writing here a before and after directory structure so this is easier to talk about? Thanks!

[wtholliday]

dir structure before:

after:

[pca006132]

I'm not sure if we should merge cross_section here, as we spent some effort to make it optional previously. And we probably want to put the source files into src folder?

We should ask downstream users about this though.

[pca006132]

Pinging some of our C++ users...

@kintel @howardt @starseeker

[starseeker]

If it were me, I would do something like the following:

toplevel |
                - bindings
                - docs
                - include
                - samples
                - src
                - test

I'd have all the headers in include (probably include/manifold) and the src files in src. If multiple libraries are to be built, that can be done in the one CMakeLists.txt file - IMHO manifold doesn't have enough source files to warrant breaking them out into subdirectories.

I do have a question about the separation of cross_section, polygon, etc. - I understand making them optional, but is it necessary for them to be optional at run time, or could they be enable/disable configure options when building manifold proper? I would imagine most Linux distros etc. would just enable them by default, and a minimalist configuration doing its own build could just set the configure options to produce a libmanifold.so without the unneeded features, but it's possible I'm not properly understanding the use case.

I could try my own hand at reorganization of the files and build logic if that's of interest - I have a fair bit of experience with CMake at this point, although I'm still working on understanding some newer things like the exported targets.

[starseeker]

I'll take a quick stab at what I'm thinking about in my own fork for comment - it'll probably take me a day or so, I'll post a link here once I've got something that compiles.

[elalish]

@starseeker Yes, please make your own PR so we can compare and contrast. I believe you have the right idea, but I'd like to hear confirmation from our other users. I'm much more an algorithm guy than a packaging guy.

[wtholliday]

Seems like what I did is a subset of what @starseeker proposed above BTW

[starseeker]

Just to confirm - is there explicit user interest in keeping the C bindings separate - i.e. something to be turned off? I'm asking because unlike the other languages, in almost all cases anything able to compile Manifold's main C++ code should also be able to compile the C binding code. I would expect it would be a slight simplification overall to just build the C bindings by default with the rest of Manifold.

[pca006132]

• cross_section: We made it optional so users don't have to install clipper2. I think it is fine to make clipper2 required dependency? Clipper2 is not a huge library anyway.
• C binding: Yes we can just always build it, probably need to change

  manifold/bindings/c/CMakeLists.txt

  Line 4 in 359274d

  add_library(${PROJECT_NAME} SHARED

  and remove the SHARED attribute.

[elalish]

Some of the structure we had was to ensure a very minimal version could be built, which was driven to some extent by @fire for Godot integration. I think keeping Clipper2 optional is a good idea, as it's less about the size of the dependencies as the number of dependencies. @fire, you should probably look over this thread - I'd bet you'll have opinions. Hopefully this is moving in a direction that's also helpful to you.

[fire]

At this point the largest manifold dependency is glm and I'm not sure forcing the code to have pluggable math libraries are worth it.

[pca006132]

We are not using that many features of glm, so replacing it with some custom implementation will probably take around 3 hundred lines or something? The problem is that I don't see anyone is interested in doing this...

[starseeker]

Am I interpreting correctly that cross_section must be enabled for the bindings to be enabled?

[pca006132]

They should be independent, perhaps we forgot to write the logic in cmake.

[starseeker]

Also, are there parts of the code that are supposed to not require C++17? I'm seeing a lot of specifications of the standard, which makes me wonder if there is a subset of the code that's supposed to compile with an older standard?

[pca006132]

No, I think we just duplicated that everywhere. Probably some don't require C++17, but it doesn't hurt to use C++17 (we are not using deprecated stuff).

[howardt]

• cross_section: We made it optional so users don't have to install clipper2. I think it is fine to make clipper2 required dependency? Clipper2 is not a huge library anyway.

This was at my request (for Blender). I don't need cross_section and would rather not have the Blender project take on a dependency it doesn't need. I agree Clipper2 isn't large so this isn't a huge deal from a code-size problem, it's more that every new dependency is something that has upstream activity that may break things, has to be kept up to date, etc. If Clipper2 goes back to not being optional, I might do local edits to the manifold distribution after I download it to remove the dependency (which is what I originally did). So not a deal breaker, but just expressing my desire that it be optional if possible.

[pca006132]

I see. I think making cross_section optional is independent of simplifying directory structure. We can conditionally include the file when we want to compile it.

[elalish]

Am I interpreting correctly that cross_section must be enabled for the bindings to be enabled?

Actually yes, I believe we decided that only the C++ users really need to be able to cut dependencies to the minimum. For the bindings and packaging systems, cross_section / Clipper2 are included.

[elalish]

At this point the largest manifold dependency is glm and I'm not sure forcing the code to have pluggable math libraries are worth it.

@fire I'm not sure what you mean by forcing the code to have pluggable math libraries? Forcing what code (ours, yours)? Is GLM somehow pluggable? Sorry if my questions are stupid, but I'd love to know the specific steps we can take to make this easier to use. Is this about our internal dependencies or our public API or both?

[wtholliday]

We are not using that many features of glm, so replacing it with some custom implementation will probably take around 3 hundred lines or something? The problem is that I don't see anyone is interested in doing this...

If it's really not that much code, I'll do it. Having glm part of Manifold's API makes Manifold not directly usable from Swift (I had to write a wrapper)

[pca006132]

Yeah, it is not that much code, mostly just things like dot product, cross product, vector norm, matrix multiplication, quaternion multiplication.

[wtholliday]

glm seems to wrap platform-specific SIMD APIs, so I suspect replacing it is not easy without pessimizing perf. no?

[pca006132]

I don't think we use those SIMD here, iirc default glm operations are not vectorized.

https://stackoverflow.com/questions/68254947/does-glm-use-simd-automatically-and-a-question-about-glm-performance

[fire]

We are not using that many features of glm, so replacing it with some custom implementation will probably take around 3 hundred lines or something? The problem is that I don't see anyone is interested in doing this...

If it's really not that much code, I'll do it. Having glm part of Manifold's API makes Manifold not directly usable from Swift (I had to write a wrapper)

that’ll be amazing. yes this is the one of things where in godot engine we have a primary math library and adding new libraries means like over a hundred files of duplications

[kintel]

Perhaps more interesting than reducing the # of dependencies is that using different math libraries in manifold and client code means we need to convert data between the types. Not sure there are any good ways of fixing this besides templating the hell out of all math interfaces and rely on ADL to match types. Probably not worth looking into at this point.

[starseeker]

Not done yet, but starting to get close - I can build and run manifold-test on Linux: https://github.com/starseeker/manifold/tree/reorg

[starseeker]

@howardt If Clipper2 were bundled and built as part of Manifold, rather than a dependency, would you still want to disable it?

[starseeker]

Right now I've kept cross_section as optional - the main argument for just building it all the time is to simplify everything. If we do extract and use just the bits of glm we need, then it looks like that would make the core C++ Manifold stand-alone except when using TBB for threading...

[starseeker]

I also kept polygon.cpp as an optional component - what's the motivation there? I don't think that's pulling in any dependencies?

[starseeker]

Perhaps more interesting than reducing the # of dependencies is that using different math libraries in manifold and client code means we need to convert data between the types. Not sure there are any good ways of fixing this besides templating the hell out of all math interfaces and rely on ADL to match types. Probably not worth looking into at this point.

We're having to copy data as well, but I don't know of an answer either that isn't a lot of trouble. My recommendation would be to stay the course at the moment, unless/until someone comes up with a use case and pattern that justifies the effort it would take.

[howardt]

@howardt If Clipper2 were bundled and built as part of Manifold, rather than a dependency, would you still want to disable it?

What do you mean by "bundled and built as part of Manifold"? If you mean: when I fetch the source for Manifold, the source files for Clipper2 were there too as a subdirectory, and the cmake build project automatically compiles those, then I probably wouldn't bother disabling it (though I still would disable it if I could). It's the separate fetch of Ciipper2 from somewhere else that I want to avoid.

[starseeker]

What do you mean by "bundled and built as part of Manifold"? If you mean: when I fetch the source for Manifold, the source files for Clipper2 were there too as a subdirectory, and the cmake build project automatically compiles those, then I probably wouldn't bother disabling it (though I still would disable it if I could). It's the separate fetch of Ciipper2 from somewhere else that I want to avoid.

@howardt Right. The options for using Clipper2 are "treat it as an optionally externally supplied library" like we're doing now, which is where the fetch is coming into play if an external version is not found, and just putting the needed source files in a Manifold subdir and building them directly into Manifold proper. The latter is a way simpler integration for Manifold itself and users (who no longer need to know or care about Clipper2) but has the drawback of needing to periodically updated the bundled copy (Linux distributions tend to vastly prefer not bundling.)

[starseeker]

Given this particular situation, my own take would be to maintain it as an option, but rather than the download just have a bundled copy of the requisite Clipper2 C++ files available in a subdir to build (basically replacing the current download step with an "add_subdirectory" if find_package fails and bundled dependencies aren't disabled.)

[starseeker]

It's easily revertible, but I added an example in my reorg branch above of bundling Clipper2 while still allowing it to be disabled as it currently is.

[starseeker]

Yeah, it is not that much code, mostly just things like dot product, cross product, vector norm, matrix multiplication, quaternion multiplication.

A cursory look at glm and Manifold's use of it makes me suspect that extracting the necessary subset may be involved - glm includes tend to include other includes which include other includes, so it would take some analysis to isolate just the necessary pieces. Could bundle it in lieu of downloading it...

[elalish]

@starseeker Thanks, that's definitely looking like a cleaner CMake system, which is great. I don't think there's any need to keep polygon as it's own thing - it doesn't have dependencies, so technically someone could build the triangulator without the rest of Manifold, but I haven't heard any clamoring for that. Plus it's small enough they might be better off just copying the file.

Before the fetch thing we had Clipper2 as a git submodule - the switch to fetch was motivated by @fire and co not liking the submodule approach, plus OpenSCAD didn't want two copies of Clipper. But maybe the submodule is fine again now that cross_section is optional and can also use the system Clipper? I don't want to copy Clipper's sources into our repo - I'd rather they be referenced somehow.

[starseeker]

@starseeker Thanks, that's definitely looking like a cleaner CMake system, which is great. I don't think there's any need to keep polygon as it's own thing - it doesn't have dependencies, so technically someone could build the triangulator without the rest of Manifold, but I haven't heard any clamoring for that. Plus it's small enough they might be better off just copying the file.

Got it, will simplify that.

Before the fetch thing we had Clipper2 as a git submodule - the switch to fetch was motivated by @fire and co not liking the submodule approach, plus OpenSCAD didn't want two copies of Clipper. But maybe the submodule is fine again now that cross_section is optional and can also use the system Clipper? I don't want to copy Clipper's sources into our repo - I'd rather they be referenced somehow.

OK. I reverted the commit testing clipper inclusion. Download vs. submodule is probably a toss-up - both have their pros and cons. I've added the option to block downloads, so those wanting to make sure that behavior isn't triggered can set that. My experience with submodules in BRL-CAD makes me leery of them for this simple use case - they're useful when used correctly, but the usability is... somewhat lacking.

[elalish]

I don't think we use those SIMD here, iirc default glm operations are not vectorized.

That's weird - I definitely thought SIMD was one of the reasons to use GLM. @wtholliday If you do write us a nice little math library, and if you happen to know anything about getting it to use SIMD, there's a chance we could get a significant speed-up.

[starseeker]

@elalish Should I go ahead and make a pull request? I'm not set up to test things like the python and wasm logic, so I'll need assistance there. Once things settle a bit I'll test this with my integration in BRL-CAD, but that's still on my TODO...

[elalish]

Yes, a PR would be great. Time to get some green checks! And it'll make for a better place to discuss the details anyway.

[starseeker]

OK, PR is up: #961

[wtholliday]

This has been subsumed by @starseeker 's exellent work on #961. Closing :)