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

Multi-material example #309

Merged
merged 17 commits into from
Jan 28, 2023
Merged

Multi-material example #309

merged 17 commits into from
Jan 28, 2023

Conversation

elalish
Copy link
Owner

@elalish elalish commented Jan 25, 2023

Fixes #196
Fixes #286

This is a follow-on to #290 to demonstrate that the new property APIs work end-to-end by connecting them to glTF output. I've expanded MeshGL with a different style of MeshRelation data. I'm thinking I'll remove the old MeshRelation and triProperties input entirely now, basically saying Mesh is only for pure, simple geometry, while MeshGL is for anything requiring mesh properties and relationships.

@elalish elalish self-assigned this Jan 25, 2023
@elalish
Copy link
Owner Author

elalish commented Jan 25, 2023

Oops, I jumped the gun a bit; I still need to add the triangle reference into MeshGL so the tests can pass. Shouldn't be hard, hopefully I'll get to that tomorrow.

The next PR will be addressing #282 by adding a transforms vector that corresponds to the meshID vector. I think I'll even add some optional parameters to GetMeshGL() to specify which properties are normals so I can update them automatically.

@codecov-commenter
Copy link

codecov-commenter commented Jan 26, 2023

Codecov Report

Base: 91.54% // Head: 91.36% // Decreases project coverage by -0.18% ⚠️

Coverage data is based on head (276f29f) compared to base (422c7f2).
Patch coverage: 87.75% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #309      +/-   ##
==========================================
- Coverage   91.54%   91.36%   -0.18%     
==========================================
  Files          33       33              
  Lines        3608     3626      +18     
==========================================
+ Hits         3303     3313      +10     
- Misses        305      313       +8     
Impacted Files Coverage Δ
src/manifold/include/manifold.h 100.00% <ø> (ø)
src/manifold/src/impl.h 70.00% <ø> (ø)
src/manifold/src/shared.h 58.90% <ø> (ø)
src/utilities/include/public.h 73.49% <ø> (-0.32%) ⬇️
src/manifold/src/impl.cpp 94.42% <83.33%> (-1.26%) ⬇️
src/manifold/src/manifold.cpp 86.57% <90.32%> (-0.62%) ⬇️

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 at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@elalish
Copy link
Owner Author

elalish commented Jan 26, 2023

@geoffder FYI, I removed the MeshRelation API and removed them from your cbindings too, but you'll probably want to add in the other API changes.

@geoffder
Copy link
Collaborator

Is Mesh on it's way to be a purely internal class? I see that LevelSet still returns one, and there remains a Mesh constructor for Manifold, yet Smooth has been switched to take MeshGL.

@elalish
Copy link
Owner Author

elalish commented Jan 26, 2023

I'm considering it. Definitely I'll update things like LevelSet to return one. The other constructor was superfluous since MeshGL already has a constructor that takes a Mesh, so really both are supported anyway.

@geoffder
Copy link
Collaborator

Huh, giving a Mesh to Smooth still compiles? Pardon my ignorance, but does C++ implicitly find the single argument constructor of MeshGL and just use that? Never would have expected that.

@elalish
Copy link
Owner Author

elalish commented Jan 26, 2023

Yup, indeed it does. I think it's basically the same system that let's you pass 2 (an int) when the function wants a float.

@geoffder
Copy link
Collaborator

Interesting that it does the same on it's own for user defined types with no "class/inheritance relationship". Maybe just surprising to me due to my minimal C++ experience.

@geoffder
Copy link
Collaborator

@elalish Opened a PR with the new meshgl accessors: #311. Let me know if I've missed something!

@pca006132
Copy link
Collaborator

I won't be able to review this soon, I am currently on vacation for 7 days in Japan without my computer.

@elalish
Copy link
Owner Author

elalish commented Jan 28, 2023

@pca006132 No problem, enjoy your vacation!

@elalish
Copy link
Owner Author

elalish commented Jan 28, 2023

Good news: meshID is no longer needed as part of the public API - it is now internal-only. Likewise, no need for triProperties and such in the public API as we can use MeshGL for this input instead. I'm keeping the explicit Mesh constructors for now simply because they avoid some computation. I'd recommend that Mesh remain a C++-only construct though. For bindings to other libraries, better to directly create a Manifold and use MeshGL as the only I/O struct.

@elalish elalish merged commit f43dded into master Jan 28, 2023
@elalish elalish deleted the colorExample branch January 28, 2023 21:02
cartesian-theatrics pushed a commit to SovereignShop/manifold that referenced this pull request Mar 11, 2024
* removed BufferGeometryUtils

* replaced three.js with gltf-transform

* textured sphere working

* added cubeUV

* added textures

* added relation and sorting to MeshGL

* working textures example!

* added faceID

* removed MeshRelation

* some more removal

* fix output for no properties

* test cleanup

* Add new MeshGL accessors (replacing MeshRelation) (elalish#311)

+ new meshgl accessors + remove mesh_relation_size

* removed meshID from public API

* added input checking for MeshGL

* remove C api

---------

Co-authored-by: Geoff deRosenroll <geoffderosenroll@gmail.com>
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 this pull request may close these issues.

Imprecise barycentric coordinates Build multi-material three.js example
4 participants