GLM and API surface area

[wtholliday]

I've put in a few hours of work trying to replace GLM. It's not exactly a couple hundred lines as @pca006132 suggested earlier. And manifold doesn't use GLM in totally trivial ways. There is more numerical code in Manifold that I expected (svd.h, smoothing.cpp). There's a bit of stuff for me to get wrong. To be clear, it's totally something I can do and have done this sort of thing before (though I haven't had to write my own vector/matrix classes since grad school).

So I'm wondering, should GLM just be out of the public API but still used for internal calculations? Since it's header-only what's the harm in just checking it in to the repo so it doesn't need to be an external dep? That seems preferable to some little short-on-features set of structs I would write. Why reinvent the wheel?

[elalish]

Great question. I really don't know - I originally used GLM as a submodule for pretty much exactly this reason. I'm still trying to understand how dependency management in C++ can be so bad that I can't even use a header math library! @fire, @kintel, @starseeker, @pca006132 let's see if we can put our heads together and figure out a decent solution here.

Things we have tried:

1. GLM as a submodule (apparently everyone hates submodules)
2. link in libglm or else auto-download it (current state)
3. allow auto-download to be disabled in CMake (Attempt at simplifying the file structure and CMake build #961)

Possible futures:

3. Dump giant pile of GLM headers into our own repo, effectively forking it?
4. Trim down giant header pile into what we actually use - fork it even more.
5. Make our own math library (maybe based on GLM) - completely fork it.
6. Leave GLM as is but remove it from our public API - a bit like our C bindings.
7. ... any other ideas?

Please chime in and help us come to consensus here.

[pca006132]

Ignoring APIs that are provided by std, some constants as well as matrix/vector multiplication and quaternion operations, here is a list of APIs that we use:

glm::all
glm::any
glm::clamp
glm::conjugate
glm::cross
glm::dot
glm::greaterThan
glm::greaterThanEqual
glm::length
glm::lessThan
glm::max
glm::min
glm::mix
glm::mod
glm::normalize
glm::outerProduct
glm::quat_cast
glm::rotate
glm::rotateX
glm::rotateY
glm::rotateZ
glm::rotation
glm::saturate
glm::scale
glm::translate
glm::transpose

glm::determinant
glm::inverse
glm::smoothstep
glm::lerp

I would expect the first part to be easy to implement and the remaining 5 requiring more code. But yeah, I was probably underestimating the code needed...

Regarding should we keep glm: I don't think removing it from the public API will buy us anything for godot, they care about compile-time dependency as well. I think it is fine to keep this as is, if someone wants to get rid of this they are free to open a PR and implement everything...

[starseeker]

My take would be to avoid re-inventing the glm wheel and stick with solution 2/3. That said, I would definitely advise getting glm out of the public manifold API (option 6), unless there are serious negative implications in performance or code complexity in doing so.

I'll own up to a selfish reason for wanting option 6 - BRL-CAD bundles manifold, and its exposure of glm in the public headers means we also need to bundle glm because of how we manage and build against dependencies. It would be better if glm were just a compile time dependency - we do distinguish between them.

More importantly, if someday down the road glm becomes problematic for whatever reason and Manifold wants or needs to switch to some other spiffy math library, having glm types in the public headers means that'll be a breaking change for client codes on a much larger scale.

[stephomi]

Yes it’s mostly about getting rid of the glm include in the headers exposed to the user (at least the mandatory ones, manifold.h).
Internal use is fine.

It did bite me a while ago, back then I was using the “master” version of glm but manifold was using the “release” glm version, which was much more older.

Box is only used once in manifold.h, so you can simply return an array and keep the glm box in internal headers.

[starseeker]

@pca006132 I like the idea of cleaning up common.h (and the other public headers) so they only expose what we want to expose as the public API - that'll help a lot.

That's probably the first step then, rather than me trying to redefine vec2 and friends straight away - no point in recoding things to use different vec/mat types if they wind up being internal. How should that work?

[pca006132]

Sounds good, I can open a PR for some simple cleanup first. I think changing vec/mat to array types will be a rather large change, because we probably need to change some of our tests and bindings as well, so that should go into another PR.

[starseeker]

This might be a crazy question, but does the Manifold class need to be public? If MeshGL is how data goes into and out of Manifold, could that be the only public class? The comment in manifold.h says Manifold is the "library's internal representation of an oriented 2-manifold, triangle mesh"...

[pca006132]

The actual internal representation is a lot more complicated than that, I think the documentation was outdated for 2 years or something.

It should be public because something the user can operate on, and easily copy without any cost (it is just a shared ptr). And users may want to query it for something without actually exporting the mesh, e.g. if they want some approximation of the bounding box. (not implemented yet)

[kintel]

..or go straight for Eigen, which may be a bit more ubiquitous :)

[elalish]

Eigen feels like massive overkill for what we need - it's got to be 100x the size of GLM, right?

[kintel]

Yeah, probably, but mostly (only?) header only if that helps.

[elalish]

Well, GLM is already header-only. If I start needing higher dimensions than 4 I'll grab Eigen, but I don't think that'll happen in this library.

[wtholliday]

In the interest of incremental progress, I'd say doing option 6 is a good one. That reduces the API surface area. The decision of whether to (effectively) fork GLM is a separate issue.

As an alternative to GLM, maybe this one? https://github.com/sgorsten/linalg. Just one header so you can drop it into the repo without much guilt :)

[elalish]

linalg looks pretty slick, actually! If you'd like to take a look at the feasibility of switching to that from GLM, I'd be quite interested.

As you say, I suppose option 6 is still a good idea regardless.

[starseeker]

Maybe I'm unnecessarily paranoid, but when we talk about switching out glm I can't help thinking back to #610 and how that one line change made such a difference in the reliability of the booleans (and I think there have been a few more like that for zebra et. al.) I guess the test suite will have our backs if we did make such a change...

[elalish]

Appreciated, but that was my own code deep in our triangulator, which is always our problem child. These vector ops are much more settled territory, and every one of those math functions gets called probably no less than millions of times in our test suite. I think this should be fairly safe - more a matter of how many functions aren't included in the other library. And GLM has an open enough license we can grab a few lines of code if we need them.

[fire]

https://github.com/sgorsten/linalg looks slick, bit worried about the amount of work we have to do but Make our own math library (maybe based on GLM) - completely fork it. and using linalg seem to be in the same area.

[elalish]

Starting a new thread based on:

One other naive C++ question: is there a vec3 struct that we could safely cast a glm::vec3 struct back and forth to? So, don't actually touch the memory, but just cast away the GLM-ness of it? Then I could cast them back to GLM in our tests and it would all be pretty transparent. Not sure if that's a "there be dragons" scenario though.

According to this, GLM is actually designed for this kind of casting. Would this be a simple solution to 6)? Just cast any GLM structs currently exposed in our public API to generic arrays? What do you think @pca006132?

[elalish]

That sounds perfect!

[starseeker]

Being selfish again, my input data to Manifold is a C array of doubles holding vertex coordinates and a C array of triangle vertex indices. At the moment I'm using the older Mesh container and creating glm::vec3 and glm::ivec3 copies of the data to populate it, so I know I'm going to need to update things in any case. Since I'll be updating that code, I can't help wondering... In an ideal world it would be awesome to create C++ containers (std::array?) from those C arrays and pass them off directly to Manifold with no further processing - would that be possible?

[pca006132]

MeshGL64 already accepts flattened array of doubles and triangle vertex indices. It may be possible to make our API accept a VecView and avoid the copy to a C++ vector, but this may require duplicating some code on our side. Can have a look...

[wtholliday]

@starseeker what's your concern there? extra memory usage? eliminating the conversion code? (isn't it just a few lines?)

[starseeker]

@wtholliday Primarily extra memory usage.

[pca006132]

Opening a new thread regarding avoiding copy for MeshGL/MeshGL64. I think this is a fair request because we usually don't need to use the arrays inside as vectors, it is just some temporary data structure, so allowing users to provide pointers to some C array can make sense.

My proposal for this would be to make MeshGLP self-referential, i.e. things like vertProperties can point to a vector stored in dataSource, or just a pointer to some user provided buffer.:

struct MeshGLP {
  VecView<Precision> vertProperties;
  // ...
  std::unique_ptr<DataSource<Precision, I>> dataSource = nullptr;
};

The data source pointer handles ownership. When MeshGLP is destroyed, we will free the data source if it is not null, and it will free the buffers inside if any. Typically, for MeshGLP exported from manifold, users don't have to worry about lifetime because it is the same as the existing API, users can deallocate the Manifold object without worrying about dangling pointers in this MeshGLP object. However, if users set the fields of MeshGLP for import into manifold and don't explicitly copy things into the data source, they have to take care of the lifetime themselves, and not delete the referred vector/array before constructing a manifold. After a manifold is being constructed, they don't have to worry about the lifetime of data inside the used MeshGLP as well.

[elalish]

Pinging @ncthbrt @NickUfer since they are interested in Rust bindings and this potential API change might make a difference there. I'm still a little hazy on best practices here for passing large arrays in and out of a library. What do other popular libraries do? In general I feel like a functional approach is nice where ownership isn't ever being passed around.

[hrgdavor]

I was guilty many times looking into optimisation too soon. So at first this sounded like nice to have.

Thinking about it more I feel I would want to have safer version too if optimised version is present. This then pushes me into direction of preferring this optimised zero copy version as extension to API. Then for now, I would explore if something needs to be done now, to allow easy extension of API (zero copy) down the line as non breaking change.

[NickUfer]

Pinging @ncthbrt @NickUfer since they are interested in Rust bindings and this potential API change might make a difference there. I'm still a little hazy on best practices here for passing large arrays in and out of a library. What do other popular libraries do? In general I feel like a functional approach is nice where ownership isn't ever being passed around.

I think that whatever kind of zero copy optimizations you guys want to add should be fine with rust. The bindings are not done yet, but I already tested passing the vertProperties to rust and there was no problem. There is also no conversion going on, we can just use the data as is and construct a Vec out of it. Right now manifold_meshgl_vert_properties copies the data into a user provided buffer, but even if we just pass a pointer + length back to rust to achieve zero copy we can use the data as a Vec just fine without modification. This should hold true not only for vectors of primitive data types but also for structs. We simply mark them as repr(C) in rust, then they have the same order, size and alignment as in C/C++.

[starseeker]

Per #969 (comment) sounds like we'll shelve the zero-copy question for now, and revisit (maybe with a parallel API) later if needed.

@elalish Should we set up an issue for the glm hiding so it can be added to the 3.0 milestone?

[elalish]

Sure, would be good to summarize the upshot of these long threads anyway.

[pca006132]

FYI: #976 (comment)