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

Mesh import/export from python #981

Closed
wants to merge 1 commit into from

Conversation

occivink
Copy link

@occivink occivink commented Oct 9, 2024

hi,

I thought this was a simple change that makes it easier to try out the library locally
I named the functions import_mesh and export_mesh, since just import is restricted

Copy link

codecov bot commented Oct 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.21%. Comparing base (d437097) to head (3118a2e).
Report is 117 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #981      +/-   ##
==========================================
- Coverage   91.84%   88.21%   -3.63%     
==========================================
  Files          37       62      +25     
  Lines        4976     8670    +3694     
  Branches        0     1043    +1043     
==========================================
+ Hits         4570     7648    +3078     
- Misses        406     1022     +616     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@pca006132
Copy link
Collaborator

Thanks, but our recommendation is to use trimesh for mesh io. Did you try using that? I think the integration should not be hard. You can refer to our python colab example.

@occivink
Copy link
Author

occivink commented Oct 9, 2024

I did notice, but it feels redundant to use a separate library when the functionality is already there natively, just not exposed in the bindings.
Likewise, the C bindings also expose import/export functions so I figured it would not be out of place to add it to the python ones.

@elalish
Copy link
Owner

elalish commented Oct 9, 2024

Thanks, but 3D file I/O is worth a library unto itself. We have the absolute bare minimum optional support to enable testing, but that's it, and even our support is through a 3rd-party library (Assimp). Trimesh is much better for Python file I/O than our MeshIO is, plus including our I/O in our bindings will make our Python library much larger (Assimp dwarfs our code base).

@occivink
Copy link
Author

occivink commented Oct 9, 2024

I understand, but when compiled with MANIFOLD_EXPORT the python bindings are already pulling in assimp transitively, so exposing these two functions doesn't practically change the size/surface of the library.
I get your reasoning for wanting to defer to a distinct library, but I think requiring every user of the python bindings to copy some boilerplate is an unnecessary hurdle.
For what it's worth, as a middling code-CAD practitioner, basically any kind of mesh export is enough for any usecase I have, and I'm sure that what is provided by assimp is sufficiently good.
Anyway, feel free to close this PR if you wish.

@pca006132
Copy link
Collaborator

I think we can add trimesh as optional dependency and add support for it. Our pypi release will not be compiled with MANIFOLD_EXPORT so adding this will increase our binary size quite a bit.

@occivink
Copy link
Author

occivink commented Oct 9, 2024

The change I proposed only applies when the library is compiled with both python bindings and assimp are enabled, so if MANIFOLD_EXPORT is disabled for the pypi release, then it will not be affected.

@elalish
Copy link
Owner

elalish commented Oct 9, 2024

What do you think of @pca006132 's idea? I think that would give a better result, and probably be easier to use too. And then all our Python users would be on the same page.

@occivink
Copy link
Author

Any added convenience in import/export would be welcome, I think most users would expect being able to do this in a one-liner.

Right now the library is quite easy to get started and to create shapes just by reading the help(manifold3d) documentation, but there's a conspicuous absence of any kind of file I/O in it. The current situation where the user goes to a google-colab space to copy a 10-line function is not nice, also considering that this is only covers export.

I'm of course not attached to the assimp solution, and if you think trimesh is more suitable then I trust your judgement.

@pca006132 pca006132 closed this Oct 10, 2024
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.

3 participants