Enable 1D and 3D: mesh construction and X3D parsing. #633

RyanWollaeger · 2019-06-08T03:01:42Z

Background

Currently, the Draco_Mesh class only supports 2D mesh objects.
However, unstructured meshes can be constructed from dimension-agnostic topological data.
The Layout data structures in Draco_Mesh are already compatible with 1D and 3D cell topologies.
In 3D, node orientation about a face with respect to the interior of a cell can be constrained to be counter-clockwise, assuming the face does not have internal nodes.

Purpose of Pull Request

Add alternate mesh construction to enable 1D and 3D unstructured meshes.
Enable the X3D mesh parser to provide dimension-agnostic data to Draco_Mesh_Builder.
Support backwards-compatibility with older 2D-specific method.
Note this new algorithm preserves the layout, but changes some of the other data Draco_Mesh stores (cell_type, cell_to_node_linkage).
Fixes Redmine Issue #1503

Description of changes

Add optional input parameter to Draco_Mesh ctor, face_type, which is the number of nodes per face per cell.
When face_type is given (size > 0):
- interpret cell_type as the number of faces per cell instead of the number of nodes per cell,
- call a corresponding overload of compute_cell_to_cell_linkage that uses face_type.
Add optional ctor argument to X3D_Draco_Mesh_Reader, use_face_types, which tells the class to provide data to build face_type in Draco_Mesh_Builder.
Test to ensure Layout data is unchanged in 2D when using new compute_cell_to_cell_linkage in tstDraco_Mesh.cc.
Test X3D reader and builder when use_face_types=true in tstX3D_Draco_Mesh_Reader.cc.

Status

Reference:
- Pre-Merge Code Review
- CDash Status
Checks
- Travis CI checks pass
- Code coverage does not decrease
- Valgrind test passes
- Toss3 checks pass
- Trinitite checks pass
- Code reviewed/approved, sufficient DbC checks, testing, documentation

+ Add optional face type (nodes per face) argument to mesh ctor. + Use face type to compute dimension-agnostic cell connectivity. + Add 1D and 2D tests to tstDraco_Mesh.cc.

+ Add default boundary data generation when face types are given. + Add test of default boundary generation to new 1D unit test.

+ Instantiate reader with optional use_face_type parameter. + Set cell type and as faces per cell when use_face_type is true. + Preserve cell->face->node in linkage when use_face_type is true. + Set face_type vector in mesh builder when use_face_type is true. + Enforce use_face_type in mesh reader virtual base class. + Add corresponding function stubs to RTT (not implemented). + Update unit test to test alternate mesh reader/builder.

+ Remove DLL_PUBLIC macros from X3D and RTT functions. + Update X3D unit test with newer failure macros.

KineticTheory

This looks really nice. I've added a few comments but nothing that should prevent this from being merged. Let me know if you want to make one more set of changes or if you want me to merge this one.

KineticTheory · 2019-06-10T03:28:06Z

src/mesh/Draco_Mesh.cc


 #include "Draco_Mesh.hh"
 #include "ds++/Assert.hh"
 #include <algorithm>
-#include <iostream>
+// #include <iostream>


If this isn't needed, go ahead and delete this line.

Thank you for catching this - it has been removed.

KineticTheory · 2019-06-10T03:33:47Z

src/mesh/Draco_Mesh_Builder.t.hh

@@ -166,7 +177,8 @@ Draco_Mesh_Builder<FRT>::build_mesh(rtt_mesh_element::Geometry geometry) {

  std::shared_ptr<Draco_Mesh> mesh(new Draco_Mesh(
      dimension, geometry, cell_type, cell_to_node_linkage, side_set_flag,
-      side_node_count, side_to_node_linkage, coordinates, global_node_number));
+      side_node_count, side_to_node_linkage, coordinates, global_node_number,
+      {}, {}, {}, {}, face_type));


I'm not crazy about the empty brackets here. It's not hard to look up the argument list as defined by the constructor, but maybe we could use inline named argument declarations here. What do you think? Is it worth the hassle?

Are keyword arguments possible? Apologies for my confusion... Some variables have been initialized as empty and used here, with consistent naming.

I'm not sure about it for this UseCase. Have you tried {} --> vector<int> foobar{} or a similar signature?

It didn't appear to work... vector<int>() works, which is somewhat more descriptive. What about this?

KineticTheory · 2019-06-10T03:35:26Z

src/mesh/RTT_Draco_Mesh_Reader.hh

@@ -34,14 +34,15 @@ private:

 public:
  //! Constructor
-  DLL_PUBLIC_mesh explicit RTT_Draco_Mesh_Reader(const std::string filename_);


👍 Nice to see these disappearing slowly. Alex and have a plan to remove all the remaining entries after the release. (except for the handful that are still needed.)

No problem - good to know!

KineticTheory · 2019-06-10T03:37:46Z

src/mesh/RTT_Draco_Mesh_Reader.hh

+                                          size_t /*face*/) const {
+    Insist(false, "cell-face nodes not implemented with RTT reader.");
+    return std::vector<unsigned>();
+  }


You might want to add the function decoration [[noreturn]] (see ds++/Assert.hh for an example) to avoid any compiler confusion.

Thank you for the recommendation - this is a useful feature. A [[noreturn]] decoration has been added and the return line has been removed.

KineticTheory · 2019-06-10T03:43:37Z

Also - CDash is reporting this warning: X3D_Draco_Mesh_Reader.cc:252:16: warning: unused variable ‘num_faces’ [-Wunused-variable]

+ Put variable unused in release builds in Remember macro. + Add noreturn decorator to function that only throws assertion. + Remove commented out code.

RyanWollaeger · 2019-06-10T15:26:18Z

The unused num_faces has been placed in a Remember macro. Thank you for catching this.

KineticTheory · 2019-06-10T15:56:28Z

LGTM. merging...

RyanWollaeger added 5 commits June 7, 2019 17:12

Prepare Draco_Mesh for 1D and 3D.

4f27af5

+ Add optional face type (nodes per face) argument to mesh ctor. + Use face type to compute dimension-agnostic cell connectivity. + Add 1D and 2D tests to tstDraco_Mesh.cc.

Add 1-3D default boundary connectivity generation.

2bf0e20

+ Add default boundary data generation when face types are given. + Add test of default boundary generation to new 1D unit test.

Make layout robust against side and ghost to node linkage ordering.

a470490

Add missing file from previous commit.

0d35f0b

RyanWollaeger requested a review from KineticTheory June 8, 2019 03:01

RyanWollaeger self-assigned this Jun 8, 2019

+ Add autodocumentation for use_face_type_ arg in X3D reader.

a92fc0a

+ Remove DLL_PUBLIC macros from X3D and RTT functions. + Update X3D unit test with newer failure macros.

KineticTheory approved these changes Jun 10, 2019

View reviewed changes

Incorporate suggestions from KT.

be9dc18

+ Put variable unused in release builds in Remember macro. + Add noreturn decorator to function that only throws assertion. + Remove commented out code.

KineticTheory merged commit 4128713 into lanl:develop Jun 10, 2019

KineticTheory mentioned this pull request Jun 17, 2019

Draco-7_2_0 #637

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable 1D and 3D: mesh construction and X3D parsing. #633

Enable 1D and 3D: mesh construction and X3D parsing. #633

RyanWollaeger commented Jun 8, 2019 •

edited by KineticTheory

Loading

KineticTheory left a comment

KineticTheory Jun 10, 2019

RyanWollaeger Jun 10, 2019

KineticTheory Jun 10, 2019

RyanWollaeger Jun 10, 2019

KineticTheory Jun 10, 2019

RyanWollaeger Jun 10, 2019

KineticTheory Jun 10, 2019

RyanWollaeger Jun 10, 2019

KineticTheory Jun 10, 2019

RyanWollaeger Jun 10, 2019

KineticTheory commented Jun 10, 2019

RyanWollaeger commented Jun 10, 2019

KineticTheory commented Jun 10, 2019

Enable 1D and 3D: mesh construction and X3D parsing. #633

Enable 1D and 3D: mesh construction and X3D parsing. #633

Conversation

RyanWollaeger commented Jun 8, 2019 • edited by KineticTheory Loading

Background

Purpose of Pull Request

Description of changes

Status

KineticTheory left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KineticTheory commented Jun 10, 2019

RyanWollaeger commented Jun 10, 2019

KineticTheory commented Jun 10, 2019

RyanWollaeger commented Jun 8, 2019 •

edited by KineticTheory

Loading