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

Refactor public API to hide glm as an implementation detail #976

Closed
starseeker opened this issue Oct 3, 2024 · 12 comments · Fixed by #984
Closed

Refactor public API to hide glm as an implementation detail #976

starseeker opened this issue Oct 3, 2024 · 12 comments · Fixed by #984
Milestone

Comments

@starseeker
Copy link
Contributor

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.)

@elalish elalish added this to the v3.0 milestone Oct 3, 2024
@starseeker
Copy link
Contributor Author

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
Copy link
Collaborator

std::array<double, n>? or double[n]?

@starseeker
Copy link
Contributor Author

@pca006132 Hmm. Maybe the question is whether there would be any downside to using std::array instead of the double array?

@pca006132
Copy link
Collaborator

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).

@pca006132
Copy link
Collaborator

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.

@starseeker
Copy link
Contributor Author

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...

@pca006132
Copy link
Collaborator

@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.

@starseeker
Copy link
Contributor Author

I'm going to have to shift gears away from this for a while - if someone else wants to tackle it feel free.

@pca006132
Copy link
Collaborator

Was talking to @elalish about this:

About the various API choices:

  • std::array: We have to add conversion functions here and there because glm doesn't cast to/from std::array.
  • C array is bad as well because functions cannot return C arrays. And passing a pointer back is bad because we have to store the data somewhere, and the stack is not a good place to store that (easily become dangling pointers).
  • Custom structs: We may be able to use custom this and define implicit conversion from glm vector/matrix to our struct, but conversion back to glm vector/matrix will require conversion functions.

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.

@fire
Copy link
Contributor

fire commented Oct 10, 2024

How difficult is for Manifold to replace glm with linalg? Any estimates?

@elalish
Copy link
Owner

elalish commented Oct 10, 2024

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...

@starseeker
Copy link
Contributor Author

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.

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

Successfully merging a pull request may close this issue.

4 participants