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

Make boolean engine selection more flexible #2309

Merged
merged 4 commits into from
Oct 31, 2024

Conversation

onitake
Copy link
Contributor

@onitake onitake commented Oct 26, 2024

Closes: #2306

As announced, here's a patch that refactors the boolean engine autodetection.

I had to refactor a few things to make the two engines behave the same way and avoid circular imports.

reduce_cascade is now in util.py. I assumed that this is needed in other places, so I didn't move it into the new interfaces.manifold module.

interfaces.blender and interfaces.manifold are now dynamic imports. This avoids a circular import loop and allows autodetection at load time.

@mikedh mikedh mentioned this pull request Oct 29, 2024
@mikedh
Copy link
Owner

mikedh commented Oct 29, 2024

Awesome thanks for the PR!! Few quick thoughts:

  • is there any reason we couldn't use trimesh.util.has_module for the autodetection? It seems pretty much the same idea as here.
  • I've been meaning to move trimesh.boolean.reduce_cascade and trimesh.util.chain into their own trimesh.iteration submodule for the "materially similar to functions in itertools" stuff. I'll do that on merge which should reduce circular import problems.

Is there a side effect of try: import manifold3d? Because we could also pick the default by using the first one that isn't an ExceptionWrapper:

# which backend boolean engines                                                            
_engines = {
    "manifold": boolean_manifold,
    "blender": interfaces.blender.boolean,
    "junk": ExceptionWrapper(ValueError("nah")),
}
_engines[None] = next(
    (v for v in _engines.values() if not isinstance(v, ExceptionWrapper)), None
)

@onitake
Copy link
Contributor Author

onitake commented Oct 30, 2024

Looks like it didn't quite work as well as I had hoped.

Thanks for the pointers, I'll take a look at has_module.

@mikedh mikedh changed the base branch from main to units/xaml October 31, 2024 19:14
@mikedh
Copy link
Owner

mikedh commented Oct 31, 2024

No worries! I'll play around with it in my branch a bit. Thanks for the PR!!

@mikedh mikedh merged commit 13d743c into mikedh:units/xaml Oct 31, 2024
1 of 9 checks passed
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.

Boolean unit tests fail if manifold3d is not installed
2 participants