8000 stop building assimp by elalish · Pull Request #123 · elalish/manifold · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

stop building assimp #123

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 4 commits into from
Closed

stop building assimp #123

wants to merge 4 commits into from

Conversation

elalish
Copy link
Owner
@elalish elalish commented May 16, 2022 •
edited
Loading

@pca006132 I see why you went this route, but I'm impatient when it comes to building, plus the patch makes building a bit complicated. I don't love having to wait to rebuild assimp every time I make a random change to CMakeLists. The only trick is I'm not sure the best way to install it for Windows. I'm wondering if we should switch to vcpkg for our dependencies so we have a consistent system between Linux and Windows. Assimp recommends it. What do you think?

@elalish elalish self-assigned this May 16, 2022
@pca006132
Copy link
Collaborator

Never tried vspkg before, but sounds good to me if its version is more up to date, comparing to the one installed with the OS. I think we still need to build assimp for emscripten to run our tests though.

If what we need is faster build time, I think there are several things that we can do to speed up compilation without removing assimp entirely:

  1. Reduce the number of import and export formats. Currently we build every format supported by assimp, but we don't need most of them just for our tests.
  2. Try ccache? Haven't tried that before but I've heard that it can speed up compilation a lot.

@elalish
Copy link
Owner Author
elalish commented May 16, 2022

Hmm, in the same way we've been talking about separating meshIO out of Python/WASM, maybe we should separate it from our tests as well. It's not really used by very many. I'd be pretty comfortable not even bothering with meshIO/assimp at all for the WASM build. What do you think?

If someone does want meshIO, it's nice to have it automatically working for all the formats, but we could fall back to reducing that if we really need to keep building it ourselves.

@pca006132
Copy link
Collaborator

I'm fine with separating it from our tests.

@elalish
Copy link
Owner Author
elalish commented May 16, 2022

Okay @pca006132, I'm going to leave this for now and focus on fixing the bugs you've reported. If you happen to get interested in this PR, you're welcome to take it over. Not a huge priority, but I think separating meshIO as much as we can and using pre-built assimp everywhere else will simplify things.

@pca006132
Copy link
Collaborator

Maybe you can try this patch and see if it improves your build time?

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 8f32750..70f2138 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -33,6 +33,17 @@ if(NOT THRUST_BACKEND STREQUAL "CUDA" AND NOT THRUST_BACKEND STREQUAL "OMP" AND
     message(FATAL_ERROR "Invalid value for THRUST_BACKEND: ${THRUST_BACKEND}. Should be one of \"CUDA\", \"OMP\" or \"CPP\"")
 endif()
 
+option(ASSIMP_FAST_BUILD "build ASSIMP just for tests" ON)
+if(ASSIMP_FAST_BUILD)
+    option(ASSIMP_INSTALL FALSE)
+    option(ASSIMP_BUILD_ALL_IMPORTERS_BY_DEFAULT FALSE)
+    option(ASSIMP_BUILD_ALL_EXPORTERS_BY_DEFAULT FALSE)
+    foreach(FMT OBJ;PLY;STL;GLTF)
+        set(ASSIMP_BUILD_${FMT}_IMPORTER TRUE)
+        set(ASSIMP_BUILD_${FMT}_EXPORTER TRUE)
+    endforeach()
+endif()

Just disabled most of the importers and exporters. The build is significantly faster on my computer.

@fire
Copy link
Contributor
fire commented May 16, 2022 •
edited
Loading

I am not that hot on vcpkg :/

I think disabling is fine as long the unit tests run.

@elalish
Copy link
Owner Author
elalish commented Jun 3, 2022

This has been superseded by trimming down the number of Assimp importer/exporters to build. That's much simpler for now.

@elalish elalish closed this Jun 3, 2022
This was referenced Sep 1, 2022
Merged
Merged
@elalish elalish deleted the assimp branch September 6, 2022 18:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@elalish elalish

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@elalish @pca006132 @fire
0