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

Prepare proper software release of Software Remesher #1

Open
wants to merge 22 commits into
base: master
Choose a base branch
from

Conversation

dmikushin
Copy link

@dmikushin dmikushin commented Oct 11, 2024

Hi @orzzzjq ,

Thank you for publishing the source code of Surface Remesher.

I understand that Surface Remesher is the academic work, so its source code is not production-ready, and polishing the source code is not an academic goal.

This PR makes Surface Remesher code ready for deployment in the following ways:

  1. The bugs and compile errors in the code are fixed. There is one particularly unusual issue that was also fixed: without ordering CGAL include files in one exact way, the software won't ever compile.
  2. The software is packed up into a Docker container. Whether we like it or not, Docker is an industry standard, which allows to start using the software in seconds, without building it. I strongly suggest that it should be a part of the release.
  3. The embedded self-made mesh rendering is removed in favor of a standalone mesh viewer. It's very likely that users may wish to use their own rendering engines, so we should provide an example of that.
  4. Configurable parameters can be specified with a JSON file

I wish to extend this work with Python API and Blender plugin, but currently don't have time. I hope you'll like this PR as it is.

…CUDA compiler and resolve the following link error:

/usr/bin/ld: CMakeFiles/surfremesh.dir/src/gDel2D/gDel2D/GPU/GpuDelaunay.cu.o: in function `GpuDel::doInsertConstraints()':
tmpxft_000003c2_00000000-6_GpuDelaunay.cudafe1.cpp:(.text+0x34dc): undefined reference to `Visualizer::addFrame(DevVector<Point2> const&, DevVector<Segment> const&, DevVector<Tri> const&, thrust::THRUST_200500_800_NS::host_vector<int, std::allocator<int> > const&, int)'
/usr/bin/ld: tmpxft_000003c2_00000000-6_GpuDelaunay.cudafe1.cpp:(.text+0x363c): undefined reference to `Visualizer::addFrame(DevVector<Point2> const&, DevVector<Segment> const&, DevVector<Tri> const&, thrust::THRUST_200500_800_NS::host_vector<int, std::allocator<int> > const&, int)'
collect2: error: ld returned 1 exit status
make[2]: *** [CMakeFiles/surfremesh.dir/build.make:334: surfremesh] Error 1

This symbol is requested:

$ nm -C CMakeFiles/surfremesh.dir/src/gDel2D/gDel2D/GPU/GpuDelaunay.cu.o | grep addFrame
                 U Visualizer::addFrame(DevVector<Point2> const&, DevVector<Segment> const&, DevVector<Tri> const&, thrust::THRUST_200500_800_NS::host_vector<int, std::allocator<int> > const&, int)

However, only this symbol is present:

$ nm -C CMakeFiles/surfremesh.dir/src/gDel2D/Visualizer.cpp.o | grep addFrame
0000000000003724 T Visualizer::addFrame(DevVector<Point2> const&, DevVector<Tri> const&, int)
0000000000003a06 T Visualizer::addFrame(DevVector<Point2> const&, DevVector<Segment> const&, DevVector<Tri> const&, thrust::THRUST_200500___CUDA_ARCH_LIST___NS::host_vector<int, std::allocator<int> > const&, int)

Note there is "__CUDA_ARCH_LIST__" instead of "800" in the name, because "__CUDA_ARCH_LIST__" in CMake this macro is only substituted by the CUDA compiler. Therefore, the easiest fix is to use CUDA compiler for Visualizer.cpp
… main.cpp:

If the CGAL includes order is different sometimes GCC and Clang both fail with weird template substitution errors. This is probably a defect in CGAL. The issue was tracked down by comparing the main.cpp to a similar seam_Polyhedron_3.cpp CGAL example.
…es using pymeshlab and polyscope. Save the camera view, so that all subsequent screenshots use the same camera view
…ze these days; removing allows to drop many OpenGL-related dependencies
… parameters available as the function arguments
…sion is .json, load surfmesh arguments from .json; if .json also provides values for any optional arguments, use them instead of the defaults as well
* Adding command line description
* Adding --choose-scene command line option, which will enforce choosing the scene and overwrite existing JSON with cached scene
@orzzzjq
Copy link
Owner

orzzzjq commented Oct 18, 2024

Hi @dmikushin,

Thank you so much for your contribution and suggestions to the project. I was traveling in the past few days therefore didn't reply at once.

Your suggestion of using Docker is great. However, personally I am not quite familiar with Docker. I will check it recently and considering merging your commits into the repo once I figure out how it works.

@dmikushin
Copy link
Author

Hi @orzzzjq , thank you for the reply, and do not hesitate to ask me about anything. Have a good travel!

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