Basic behavior and signatures for memory management

[nmwsharp]

It would be nice if it's easier to write "simple" C-style code in geometry-central without thinking about e.g. std::unqiue_ptr<>. Some changes that would push us in that direction without breaking existing code:

• Add copy/move constructors to the heavy classes like SurfaceMesh and Geometry subtypes. There are some pitfalls here, but that's outweighted by the benefit of avoiding pointer contortions.
• Add versions of the factory functions (loaders / copiers) which return either raw pointers or values

[keenancrane]

I think the other reason why unique_ptrs are currently used so much is that the paradigm seems to be to provide function output as lvalues, rather than passing in a handle to the output, e.g.,

// prototype: T foo( void );
result = foo();

rather than

// prototype: void foo( T& result );
foo( result );

The former is perhaps nicer to read, but the latter does not require one to think as much about ownership, move semantics, etc. To give a concrete example, here's a routine from surface_mesh_factories.cpp that returns some mesh data as l-values:

std::tuple<std::unique_ptr<ManifoldSurfaceMesh>, std::unique_ptr<VertexPositionGeometry>,
           std::unique_ptr<CornerData<Vector2>>>
makeManifoldSurfaceMeshAndGeometry(const std::vector<std::vector<size_t>>& polygons,
                                   const std::vector<std::vector<std::tuple<size_t, size_t>>>& twins,
                                   const std::vector<Vector3> vertexPositions,
                                   const std::vector<std::vector<Vector2>>& paramCoordinates) {

  // Construct
  std::unique_ptr<ManifoldSurfaceMesh> mesh;
  if (twins.empty()) {
    mesh.reset(new ManifoldSurfaceMesh(polygons));
  } else {
    mesh.reset(new ManifoldSurfaceMesh(polygons, twins));
  }
  std::unique_ptr<VertexPositionGeometry> geometry(new VertexPositionGeometry(*mesh));
  for (Vertex v : mesh->vertices()) {
    // Use the low-level indexers here since we're constructing
    (*geometry).inputVertexPositions[v] = vertexPositions[v.getIndex()];
  }
  std::unique_ptr<CornerData<Vector2>> parameterization(new CornerData<Vector2>(*mesh));
  if (paramCoordinates.size() == mesh->nFaces()) {
    for (size_t i = 0; i < mesh->nFaces(); i++) {
      Halfedge h = mesh->face(i).halfedge();
      for (size_t j = 0; j < paramCoordinates[i].size(); j++) {
        (*parameterization)[h.corner()] = paramCoordinates[i][j];
        h = h.next();
      }
    }
  }

  return std::make_tuple(std::move(mesh), std::move(geometry), std::move(parameterization));
}

Here's a version that passes in references:

makeManifoldSurfaceMeshAndGeometry(// INPUT
                                   const std::vector<std::vector<size_t>>& polygons,
                                   const std::vector<std::vector<std::tuple<size_t, size_t>>>& twins,
                                   const std::vector<Vector3> vertexPositions,
                                   const std::vector<std::vector<Vector2>>& paramCoordinates,

                                   // OUTPUT
                                   ManifoldSurfaceMesh& mesh,
                                   VertexPositionGeometry& geometry,
                                   CornerData<Vector2>& parameterization
                                   ) {

  // Construct
  if (twins.empty()) {
    mesh = ManifoldSurfaceMesh(polygons); // invoke assignment operator
  } else {
    mesh = ManifoldSurfaceMesh(polygons, twins); // invoke assignment operator
  }

  geometry = VertexPositionGeometry(*mesh); // invoke assignment operator
  for (Vertex v : mesh->vertices()) {
    (*geometry).inputVertexPositions[v] = vertexPositions[v.getIndex()];
  }

  parameterization = CornerData<Vector2>(*mesh); // invoke assignment operator

  if (paramCoordinates.size() == mesh->nFaces()) {
    for (size_t i = 0; i < mesh->nFaces(); i++) {
      Halfedge h = mesh->face(i).halfedge();
      for (size_t j = 0; j < paramCoordinates[i].size(); j++) {
        (*parameterization)[h.corner()] = paramCoordinates[i][j];
        h = h.next();
      }
    }
  }
}

A few things improved here:

• We no longer have a unwieldy return type involving both std::tuple and std::unique_ptr
• The output parameters are now named (rather than being unnamed elements of a tuple, which can cause confusion about ordering—especially when outputs have the same type).
• We don't have to construct a tuple of correctly-ordered return values at the end. As soon as we've correctly assigned the new values, we're done.
• We don't have to explicitly move() the contents of the unique_ptrs into the tuple at the end.

The downsides are:

• It's perhaps not as legible to pass in references as it is to see which values get passed on the left-hand side (with std::tie).
• We need to have constructors for VertexPositionGeometry and CornerData that do not require a mesh to be specified at construction time. This is less safe than the current setup where (I think?) these objects always have to be tied to some valid mesh.
• We'd need to implement assignment operators / copy constructors.

I don't see any difference from a performance point of view.

I also don't see any clear difference in terms of memory management safety. If you're always passing output arguments by reference, then the ownership rules are simple: the outermost calling function is responsible for owning the memory. Nobody else is allocating things with new (only making copies of data, via assignment operators), so there's no opportunity for memory to get dropped.

TL;DR: It seems like preferring return-by-reference would provide simpler syntax with equivalent performance and memory safety. But the last point is complicated, and probably I've missed an important detail or two here.

[keenancrane]

We need to have constructors for VertexPositionGeometry and CornerData that do not require a mesh to be specified at construction time. This is less safe than the current setup where (I think?) these objects always have to be tied to some valid mesh.

Maybe this really is the whole point, and maybe for that reason it's ok to stick with the current setup internally. Still seems worth pursuing a simpler interface for external users, where they do not have to think so explicitly about ownership, move semantics, etc. (e.g., not requiring them to wrap things in std::unique_ptr nor call std::move() explicitly) if they don't want to.