Skip to content

Add Python bindings for meshoptimizer #850

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

Draft
wants to merge 30 commits into
base: master
Choose a base branch
from

Conversation

afshawnlotfi
Copy link

@afshawnlotfi afshawnlotfi commented Mar 5, 2025

This PR introduces complete Python bindings for the meshoptimizer library, allowing Python developers to take advantage of the mesh optimization algorithms directly through a convenient API.

Features

  • Complete coverage of the meshoptimizer API
  • NumPy integration for efficient data transfer
  • Automatic parameter inference (vertex count, stride, etc.)
  • Comprehensive documentation and examples
  • Full test coverage of all binding functionality

Implementation Details

The bindings are implemented using C types to create a Python module that exposes all the functionality of the C++ library. NumPy arrays are used for data exchange, making it seamless to integrate with existing Python workflows.

All functions follow the same naming convention as the C++ API, with some Python-specific conveniences:

  • Optional parameters are inferred when possible (e.g., vertex_count from array length)
  • NumPy array allocation is handled automatically
  • Error checking is more robust with clear Python exceptions

Package Distribution

I've already published this package on PyPI under the name meshoptimizer to make it easily accessible to Python users. I'm more than happy to transfer the PyPI ownership to you if you'd like to maintain it going forward.

Documentation

Documentation is included in the README.md file, with examples showing how to use all the major features of the library.

Testing

The bindings include comprehensive tests to ensure compatibility with the core C++ library.


Please let me know if you have any questions or if you'd like me to make any changes to the implementation. I'm happy to help with the integration process.

  • Afshawn

…y checks and streamline include directory handling
…e source file handling; update .gitignore and MANIFEST.in for new structure
…y to generate module file from CMakeLists.txt, and improve memory allocation handling
@zeux
Copy link
Owner

zeux commented Mar 6, 2025

Thanks for the PR! Setting expectations: this is a large PR, and I probably won't have enough time to look at this in detail in the next couple weeks as I'm trying to wrap up a few remaining items for a pre-GDC release. It makes sense for Python bindings to exist; meshoptimizer JS bindings are maintained as part of this repository, whereas Rust and .NET bindings are separately developed by other people, but this is more of a historical artifact, so both might be reasonable. For me to maintain these as part of this repository it would help to understand if these are useful for other people; I'm assuming they are useful for you :) but want to confirm this, as while these are fairly lightweight, it's still a lot of extra code to read, understand, and think about for future changes.

I only briefly skimmed the patch so a couple high level comments on that front:

  • It would be nice to minimize the changes here that are not strictly necessary. Specifically, devcontainers & pypi.md from top level should go; VSCode config should go; .gitignore should only have the minimum amount of extra changes that would ignore build files (e.g. python/src python/dist folders).
  • It would help to understand the build process a little better here. The C++ source files are bundled with the Python files, and that's uploaded to PyPI. When Python is used in practice, it may have different versions (which presumably requires a recompile due to different ABI), and may be used on different systems (32/64-bit, Linux/Windows, Arm/X86). Where is the native module compiled in that case - does PyPI do that during package upload?
  • The .so library that's built on Linux is fairly large, I think it's because it has the debug symbols. Not sure if that's the preferred approach in PyPI ecosystem, but it might make sense to pass extra link arguments to strip debug info.
  • The actual Python interface can be simplified by removing the extraneous "count" arguments when they are redundant; for example, simplify in JS interface takes a Uint32Array with indices (no need to pass the length here - it can be inferred), and target_index_count because that can't be inferred. This does result in a different interface vs C++, but it's more idiomatic and more in line with Rust/JS bindings.
  • For encoder/decoder, these should ideally work with other types than float32. While compressing float32 values definitely works, compressing integer values works even better so the binding should have flexibility there. Maybe during decode this means we need to specify the ndtype to decode to. Because the compression is "strided", and the decoding needs a count, this is the one case where count might be useful to specify separately
  • It would be good to have Python bindings tested in CI as part of build.yml; release.yml is not ran on pull requests to keep PR checks fast
  • I'm not sure what these are; meshoptimizer doesn't rely on these defines, and _IMPLEMENTATION define is redundantly defined in command line options, producing warnings
// Define implementation before including the header
#define MESHOPTIMIZER_NO_RESET_OVERRIDE 
#define MESHOPTIMIZER_IMPLEMENTATION
  • I'm also getting build warnings from ndarray headers, suggesting deprecated functionality might be used, which is probably worth fixing:
In file included from /tmp/build-env-vbho6p2e/lib/python3.12/site-packages/numpy/_core/include/numpy/ndarraytypes.h:1913,
                 from /tmp/build-env-vbho6p2e/lib/python3.12/site-packages/numpy/_core/include/numpy/ndarrayobject.h:12,
                 from /tmp/build-env-vbho6p2e/lib/python3.12/site-packages/numpy/_core/include/numpy/arrayobject.h:5,
                 from src/module.cpp:3:
/tmp/build-env-vbho6p2e/lib/python3.12/site-packages/numpy/_core/include/numpy/npy_1_7_deprecated_api.h:17:2: warning: #warning "Using deprecated NumPy API, disable it with " "#define NPY_NO_DEPRECATED_API NPY_1_7_API_VERSION" [-Wcpp]
   17 | #warning "Using deprecated NumPy API, disable it with " \
      |  ^~~~~~~

@afshawnlotfi
Copy link
Author

Gotcha, I actually have another repo called pymeshoptimizer that adds extra functionality. Do you think it would just be a better idea to have meshoptimizer as a Git submodule instead of merging into this repo?

@zeux
Copy link
Owner

zeux commented Mar 6, 2025

Sure, I'd be fine with that as well; this makes implementation details highlighted above a little less relevant, and can always be revisited in the future if coordination issues arise or there's more interest in the lower-level access. The submodule approach in a separate repository is the one the Rust bindings take atm.

@zeux zeux marked this pull request as draft March 13, 2025 23:15
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.

2 participants