Skip to content

Depend on numpy #2

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

Merged
merged 1 commit into from
Apr 9, 2024
Merged

Depend on numpy #2

merged 1 commit into from
Apr 9, 2024

Conversation

musicinmybrain
Copy link
Contributor

Since this package requires numpy,

import numpy as np

it should depend on it directly.

@mikedh
Copy link
Collaborator

mikedh commented Apr 9, 2024

Thanks for the PR! By the way any interest in having this binding switch the upstream library to your fork which I see you've been working on, or vendor your modifications of the library into this package?

@mikedh mikedh changed the base branch from main to release/tag April 9, 2024 19:21
@mikedh mikedh merged commit 5031b6e into trimesh:release/tag Apr 9, 2024
@mikedh mikedh mentioned this pull request Apr 9, 2024
@musicinmybrain
Copy link
Contributor Author

Thanks for the PR! By the way any interest in having this binding switch the upstream library to your fork which I see you've been working on, or vendor your modifications of the library into this package?

Hmm… I am trying to package OpenCTM for Fedora, and I’ve been fixing what I can. My goal here is mostly to do this work on a one-time basis to make the OpenCTM software available, basically in its present state, but with a reasonable standard of care taken to produce a decent-quality package for those who might still have a use for it. I’m almost at that point now, I think.

I don’t plan to take on responsibility for OpenCTM’s full ongoing maintenance. I won’t be:

  • continuing any of the unreleased post-1.0.3 development
  • updating the first-party bindings for Python, Delphi, or NodeJS or the plugins for Maya or Blender
  • renaming the project
  • tagging releases
  • porting the viewer from GTK2 to GTK4
  • adding tests
  • finishing the migration from Makefiles to CMake, or starting over in meson
  • doing anything related to non-Linux platforms
  • fuzzing or proactively looking for bugs

I’m happy to combine my patches into a fork that you can build from, if you find that useful, and to accept PR’s for any further useful changes. I do like the idea of not duplicating effort.

I will point out that so far, Danny02/OpenCTM#17 is the only patch I’ve proposed that touches lib/ rather than tools/.


I’ve been working from the 1.0.3 release rather than from the last commit to GitHub, but I think the distinction doesn’t matter much for the shared library you’re using here. My plan for a python-openctm package in Fedora is to package the shared library system-wide in an OpenCTM-libs package, then rather than vendoring the shared library as you do on PyPI, I would patch the binding code to load the system library, something like:

--- a/openctm/binding.py
+++ b/openctm/binding.py
@@ -29,20 +29,14 @@
 # ------------------------------------------------------------------------------
 
 import ctypes
+import ctypes.util
 
 import os
 import platform
 import numpy as np
 
-_cwd = os.path.abspath(os.path.dirname(__file__))
-_system = platform.system()
-if _system == "Windows":
-    _ctm_lib = ctypes.WinDLL(os.path.join(_cwd, "openctm.dll"))
-elif _system == "Linux":
-    # hardcode the support library to the location from the wheel
-    _ctm_lib = ctypes.CDLL(os.path.join(_cwd, "libopenctm.so"))
-elif _system == "Darwin":
-    _ctm_lib = ctypes.CDLL(os.path.join(_cwd, "libopenctm.dylib"))
+# Use the system copy of the support library
+_ctm_lib = ctypes.CDLL(ctypes.util.find_library("openctm"))
 
 
 def load_ctm(file_obj, file_type=None, **kwargs):

I guess I could offer a version of this patch that preserves the existing loading code, and either:

  • first tries find_library() to look for a shared library in the system linker search path, then falls back to the vendored shared library if that fails, or:
  • first tries the vendored shared library, and then falls back to using find_library() to look for a shared library in the system linker search path if that fails

Happy to send a PR either way, if you’re interested in that approach.


Given the above, please do let me know how I can help.

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