Use struct instead of Delaunator class

[delfrrr]

• remove properties that will not be needed after triangulation
• convert private methods to static functions
• do not perform triangulation in a constructor (struct method or static function?)

@flippmoke anything else?

[mourner]

I think this makes sense. I didn't yet get rid of class-based API because it's harder to make it as fast in JS (passing around lots of arguments has an overhead), but I might switch in future.

I also got rid of some properties in recent commits — now the result of the triangulation is three arrays, triangles, halfedges and hull (and also coords array), and everything else is garbage-collected at the end of the constructor.

[delfrrr]

I wonder which interface will be better?

Option 1: very simple and difficult to make a mistake but not very modern

vector<double> coords = {...};
vector<double> triangles;
vector<double> halfedges;
if (delaunator::trianglate(coords, triangles, halfedges) ==  delaunator::SUCCESS) {
 //use result from triangles, halfedges
}

Option 2: bit more modern (maybe too modern?)

vector<double> coords = {...};
auto [ triangles, halfedges ] = delaunator::trianglate(coords); // returns tuple

Option 3? / cc @flippmoke

[flippmoke]

I might suggest the following:

// structure internal to library
// used for storing all our data that is relative to each index
struct TriangulatePoint {
    std::size_t triangle;
    std::size_t halfedge;
    std::size_t hull;
}

// Point structure defined by user but templated out here
struct MyPoint {
    double x;
    double y;
    // other data could be here
}

// Our vector of point data
std::vector<MyPoint> coords = {....};

// Users will have to define a custom get_x(), get_y() function here
// as some users might have slightly different format for point data
//  this is a template that is defined within our library and we provide a 
// predefined delaunator::Point if the user wants to use it, but otherwise
// can define their own.
double delaunator::get_x(MyPoint const& pt) {
    return pt.x
}

double delaunator::get_y(MyPoint const& pt) {
    return pt.y
}

// User can now call triangulate
std::vector<TriangulatePoint> out = delaunator::triangulate(coords);

[mourner]

TriangulatePoint looks confusing to me. I'd rather suggest:

struct DelaunatorResult {
  std::size_t triangles;
  std::size_t halfedges;
}

[delfrrr]

@flippmoke do I need to do something special to avoid copying of vectors in this case? like implementing move constructor in DelaunatorResult? in case if return value optimisation is not possible for some reason?

std::vector<DelaunatorResult> out;
out = delaunator::triangulate(coords);//vectors will be copied 😱?

[mourner]

@delfrrr FYI, I ported Delaunator to Rust as a learning project. Made some refactoring there to make the code simpler and clearer, and exposed a struct-based API. The C++ version will probably benefit from the same structure. Check it out: https://github.com/mourner/delaunator-rs

[flippmoke]

Sorry, I have been on vacation.

do I need to do something special to avoid copying of vectors in this case? like implementing move constructor in DelaunatorResult? in case if return value optimisation is not possible for some reason?

std::vector<DelaunatorResult> out;
out = delaunator::triangulate(coords);//vectors will be copied 😱?

This is not a style I would ever suggest that a user of said library ever use, but if you did here it would result in the use of the std vector operator=. This has a move operator included and assuming that delaunator::triangulate had RVO, there would be no copies of the std vector ever created and it would simply overwrite the previously allocated std vector with a move operation.

[delfrrr]

@flippmoke np; I did a mistake which led to confusion; my question will be relevant for this code

//as @mourner suggested (I guess)
struct DelaunatorResult {
  std::vector<std::size_t> triangles;
   std::vector<std::size_t> halfedges;
}
...
delaunator::DelaunatorResult out;
out = delaunator::triangulate(coords);//vectors will be copied 😱?

how to avoid copy in this scenario, given that RVO is not available (or RVO will be always available given the other constrains as CXX_STANDARD 14)

[flippmoke]

@delfrrr I am not sure why RVO would not be available with your example assuming that delaunator::triangulate() was designed to support it.