Skip to content

Conversation

sjkelly
Copy link
Member

@sjkelly sjkelly commented Sep 8, 2019

Currently very WIP, but opening for comments early.

This completely removes the HomogenousMesh(sdf::SignedDistanceField...) idiom. In its place we now have the return of the isosurface function.

Type signature:

isosurface(sdf::AbstractArray{T, 3}, method::MarchingCubes, 
                 ::Type{VertType}=SVector{3,Float64}, ::Type{FaceType}=SVector{3, Int};
                 origin=SVector{3, Float64}(-1,-1,-1), widths=SVector{3, Float64}(2,2,2)) where {T, VertType, FaceType}

There is also a corresponding isosurface that takes a Function as the first argument and adds keyword samples of NTuple.

returns:

VertType[], FaceType[]

With this form we can support a wide range of data types, such as AxisArrays. It does however make the use of SignedDistanceField more clunky. We may be able to have SDF<:AbstractArray and make this better. However, by setting defaults a small one-liner liner like:
vts, fcs = isosurface(load("brain.nrrd")) should just work.

And for varied precision the call is far simpler:
vts, fcs = isosurface(load("brain.nrrd"), MarchingCubes(), Point{3,Float16}, Face{3,Int})

Since we don't rely on GeometryTypes for a mesh type and simply return a Vector of the specified VertType and FaceType, Meshing should now work with either GeometryTypes of GeometryBasics for primitives. It also removes the magical _determine_types which wasn't really easy to use. This mechanism is far more generic and explicit.

cc @rdeits @SimonDanisch

- Remove GeometryTypes
- Remove _determine_types logic
- unify algorithm function signature internals
- return tuple of (vertex, face) rather than mesh
Copy link
Contributor

@rdeits rdeits left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems reasonable--it makes integration with GeometryTypes a bit harder but should make the transition to GeometryBasics a bit easier.

Just one note--there actually wasn't any type piracy happening here before this change. We were adding methods to HomogenousMesh, but those methods all required an AbstractMeshingAlgorithm argument, which is a type owned by this package and thus not type piracy. There was actual type piracy back in the day, before #14.

@sjkelly
Copy link
Member Author

sjkelly commented Sep 10, 2019

Thanks for clarifying the point on type piracy.

I think we can retain the old API and just add the generic isosurface method as the core function for each algorithm. Since GeometryBasics is tagged, I am now considering adding it as it has a different abstract type. Then we get a good first run experience for people looking to generate/visualize/save a mesh, a little bit of future proofing, and also make it easy for people to use isosurface with whatever VertType or FaceType they want.

This will also make this PR much smaller which is good for getting a new release out soon.

@sjkelly
Copy link
Member Author

sjkelly commented Sep 10, 2019

(had benchmarks here showing improvements. Upon some fixes, performance is identical to master)

@sjkelly sjkelly changed the title RFC: Remove GeometryTypes and Type Piracy RFC: Unify internal representation with isosurface method Sep 10, 2019
@sjkelly sjkelly requested a review from rdeits September 11, 2019 04:07
@sjkelly
Copy link
Member Author

sjkelly commented Sep 11, 2019

This is mostly good to go. Pending some additional docs, a few extra tests, decision on VertType restriction, and rebase.

I am much happier with the result of this versus the prior proposal. Thanks for the feedback!

@sjkelly sjkelly mentioned this pull request Sep 19, 2019
@sjkelly sjkelly closed this Sep 19, 2019
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