-
Notifications
You must be signed in to change notification settings - Fork 96
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
Refactor public API to hide glm as an implementation detail #976
Comments
I suppose the first thing for this is to decide what types to use in the public API? Once we know that we can look at where and how casting happens... |
|
@pca006132 Hmm. Maybe the question is whether there would be any downside to using std::array instead of the double array? |
I don't think there is any downside, I guess I just have to see if they can be automatically cast from one to another (array <-> glm vector). |
We cannot return fixed-size C array from a function, so std array is probably the only choice here. And I think we can't directly cast glm vector into std array, we need copy, but it should be fine. |
Starting looking at this a little bit, trying to get a sense of what's needed. One particular aspect that jumps out so far is the std::bind usage in src/face_op.cpp - that's an aspect of C++ I'm not familiar with, so I've got some docs reading to do... If someone more knowledgeable than me is planning to tackle this I should probably wait, since I don't know that anything I come with will really be suitable, but if nobody else is engaged I'll see if I can push it forward and at least lay some ground work that might be useful - AFAIK this is the major piece that I'll need to have a "3.0-like" API I can make adjustments to on the BRL-CAD end of things... |
@starseeker I think src/face_op.cpp is internal only and should not require changing? Anyway, std::bind is just setting some parameters for functions, e.g. turning a function that takes two parameters into taking one parameter. |
I'm going to have to shift gears away from this for a while - if someone else wants to tackle it feel free. |
Was talking to @elalish about this: About the various API choices:
The main issue for this change is probably not in our public API, but we need to change quite a bit of our test code. Users will not gain any compatibility from this change, we will just make users using glm harder to use our library. And this doesn't really eliminate the large build dependency, so users such as godot will still need to vendor in a rather large dependency. Alternative: replace glm with linalg. Probably more work than just changing our public API and updating our tests, but this should make people happier with a smaller dependency. We will just expose it in the public API, we will probably just copy the header into our library considering it is just a single small header file. |
How difficult is for Manifold to replace glm with linalg? Any estimates? |
I've been looking and I think linalg is the way to go. Honestly I like the functions they built better than GLM anyway. On the one hand, it'll be a huge PR - on the other hand, I think it'll be mostly find/replace. I should probably just do it myself - I'll put it on my plate for v3.0. Estimate will be heavily dependent on when I finally kick this stupid sinus infection... |
OK, thanks for the heads up. I'll shelve my GLM hiding exploration (honestly was looking rather invasive) and keep an eye out for the linalg changes. |
Per discussions in #962 we want to look into only exposing standard data types in the public manifold headers, translating them (preferably using casting) to glm types under the hood.
Scope is deliberately limited in this issue to the changes needed to allow applications to compile against an installed Manifold instance without also requiring glm. (Other discussed topics such as replacement of glm as the internal math library, "zero-copy" inputs, etc. are considered separate topics from this issue.)
The text was updated successfully, but these errors were encountered: