JuliaGeometry / TetGen.jl

Julia's TetGen wrapper
MIT License
42 stars 9 forks source link

TetGen 1.6, artifacts etc. #21

Closed j-fu closed 3 years ago

j-fu commented 4 years ago

Hi, there is a new TetGen version 1.6 available. Also, I would like to enhance the API in order to get more information out (e.g. cell, facet region numbers). My ultimate plan is to use TetGen in addition to Triangle in SimplexGridFactory.jl . Also, transition to artifacts would be cool. I would volunteer to work on this in the next couple of months if nobody else plans to do so. How do you see this ?

SimonDanisch commented 4 years ago

That'd be great :) Let me know if you have any questions!

j-fu commented 4 years ago

Whats the best place to discuss them, in case ? I assume here ?

SimonDanisch commented 4 years ago

Yeah why not ;)

j-fu commented 4 years ago

Hi, got the first test running with artifacts, for linux so far, see https://github.com/j-fu/YggBonsai/blob/master/T/TetGen/build_tarballs.jl

I don't expect problems with the other architectures.

The jll is temporarily registered in https://github.com/j-fu/PackageNursery. Once we agree, I'll build for all architectures and make the PR to Yggdrasil, once it's there, I can make the PR to JuliaGeometry.

Some questions:

SimonDanisch commented 4 years ago

As for the c wrapper, I made a here script to create the file directly in the build script.

I think you can just have a file in the subfolder in yggdrasil via a DirectorySource, see: https://github.com/JuliaPackaging/Yggdrasil/tree/master/K/KaHyPar. This should be the way to go!

So far I build the 64bit version. Should I build 32bit as well ?

I'd just go for supported_platforms(), I don't think there was any issue with it.

Any memory management improvements would be great :)

j-fu commented 4 years ago

Ok thanks. as for 32/64 ... it was already late when I wrote that... I meant Float64 and Float32. You have this kind of artificial templating in your folder and the option to call TetGen with Float32 or Float64.

SimonDanisch commented 4 years ago

Should we both variants be supported ?

I think that would be valuable, although not highest priority. Would be nice, to at least convert in the julia wrapper to whatever is supported.

For double/single, I'd kind of hope we can rely on:

For x86 and x86_64 (amd64), you can rely on the types float and double being IEC-60559-conformant, though functions using them and operations on them might not be.

And using Cdouble doesn't seem to help with anything: https://github.com/JuliaLang/julia/blob/c44644442949238da70670b547312ca1ae9a9c7d/base/ctypes.jl#L115

j-fu commented 4 years ago

Ok, moved the C wrapper to DirectorySource. Compiled now for all supported platforms and was able to test on linux / windows. I also patched TetGen to use malloc/free, so this can be tried For the float version, tetgen1.5.1. doesn't compile with -DREAL=float, I would postpone this for 1.6.

So the plan is now:

At this stage, TetGenBuilder could be archived

j-fu commented 4 years ago

Hi, got the first steps with the "Triangulate-like" interface running; https://github.com/j-fu/TetGen.jl/commit/2448f3768d4f391695f2ad47dbcc04e4c1c4d4e8

We would have to agree about the naming of the different TetGenIO variants:

Now there is:

I very much would like to rename RawTetgenIO to TetGenIO. In any case I see this as an additional part oft the API. Any higher level API on top of this should go into a different package.

j-fu commented 4 years ago

Hi may be I was too brief and a bit robust in my communication, so let me explain:

What I meant by the additions to the API is now implemented in RawTetGenIO (besides of the local refinement stuff). RawTetGenIO essentially wraps the arrays returned by TetGen into Julia arrays. The API In Triangulate.jl is similar and works with a struct called TriangulateIO, without copying of data. Furthermore, in TetGen_jll.jl, TetGen has been patched so that new() and delete() are mapped on malloc() and free(), thus the Julia arrays now can get the ownership of the memory.

I would not touch the existing API which as far as I understand converts the data from CPPTetGenIO to different variants of Meshes, and I assume it will evolve along with GeometryBasics. Do you see the existing TetgenIO as part of the API ? If so I of course would revert the renaming.

I see various reasons for having an array based API along with the API based on the mesh structure:

What I would like to avoid is a situation like in the case of Triangulate/Triangle where there are two packages.

SimonDanisch commented 4 years ago

Why don't you add this as PR to discuss things there?

j-fu commented 3 years ago

All of this is done, only waiting for TetGen 1.6.1 finalization by upstream. Closing this.

SimonDanisch commented 3 years ago

Great work, thank you :)