Feat/mesh geometry by HenningScheufler · Pull Request #466 · exasim-project/NeoN

HenningScheufler · 2026-03-01T16:02:32Z

Geometry computation utilities

Added headers and implementations for computing geometric quantities: face centres, face areas, cell centres, cell volumes, and cell-to-face mappings (faceGeometry.hpp/.cpp, cellGeometry.hpp/.cpp, computeGeometry.hpp/.cpp). These are exposed via a new MeshGeometry struct and related functions. [1] [2] [3] [4]

Adds face topology, cell reconstruction, node reordering, and VTK cell extraction algorithms. Hash-map dedup runs on host and results are copied to the executor. No IO format dependency — pure algorithmic layer. Guarded by NeoN_WITH_VTK; links VTK modules required for type definitions.

Face area/normal, cell centroid/volume kernels. Depends on connectivity types but has no IO format dependency.

github-actions · 2026-03-01T16:05:19Z

Thank you for your PR, here are some useful tips:

If this is your first PR, please add yourself to the AUTHORS.md file
Use labels to change the workflow behavior. Set auto-fix to automatically apply fixes, full-ci for a comprehensive set of tests and sanitizer runs, ready-for-review to show that you're done and need a review.
I deployed a test documentation to https://exasim-project.com/NeoN/Build_PR_466
For full-ci builds benchmark results are available at https://github.com/exasim-project/NeoFOAM-BenchmarkData/466
Also check if the GPU pipelines on https://gitlab-ce.lrz.de/greole/neon/-/jobs are successful.

Copilot

Pull request overview

Adds unstructured-mesh IO utilities for deriving connectivity and geometric quantities (faces/cells) and introduces corresponding Catch2 tests, integrating the new sources into the build when VTK support is enabled.

Changes:

Added face/cell geometry computation utilities and a MeshGeometry aggregate API.
Added connectivity utilities for extracting/rebuilding cell/face topology and node ordering.
Added unit tests for the new connectivity/geometry utilities and wired them into the test build.

Reviewed changes

Copilot reviewed 25 out of 25 changed files in this pull request and generated 8 comments.

Show a summary per file

File	Description
`src/mesh/unstructured/io/geometry/{faceGeometry,cellGeometry,computeGeometry}.cpp`	Implements face centres/areas, cell centres/volumes, and full geometry pipeline.
`src/mesh/unstructured/io/connectivity/{faceTopology,cellReconstruction,nodeOrdering,vtkExtraction}.cpp`	Implements face dedup/topology, rebuilding cell info/connectivity, node ordering, and VTK extraction.
`include/NeoN/mesh/unstructured/io/{meshGeometry.hpp,meshConnectivity.hpp}`	Adds umbrella headers exposing new IO/geometry/connectivity APIs.
`include/NeoN/mesh/unstructured/io/{geometry,connectivity}/*.hpp`	Adds public headers for new APIs and their data types.
`src/CMakeLists.txt`	Adds the new IO sources and VTK link dependencies under `NeoN_WITH_VTK`.
`test/mesh/unstructured/io/*` + `test/mesh/unstructured/CMakeLists.txt`	Adds unit tests and hooks them into the test build.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-29T14:49:18Z

+{
+    // Take first face as base (n0, n1, n2), remaining node is apex
+    auto& baseFace = cell.cellFaceNodes[0];
+    std::set<localIdx> baseNodes(baseFace.begin(), baseFace.end());
+    localIdx apex = -1;
+    for (localIdx n : cell.nodeIds)
+    {
+        if (baseNodes.find(n) == baseNodes.end())
+        {
+            apex = n;
+            break;
+        }
+    }
+    return {baseFace[0], baseFace[1], baseFace[2], apex};
+}


localIdx apex = -1; will underflow when localIdx is unsigned, and if no apex is found this returns an invalid node id. Prefer tracking a bool found (or std::optional<localIdx>) and asserting the apex exists before returning, without relying on negative sentinels.

Copilot · 2026-04-29T14:49:19Z

+    auto& base = faces[baseIdx];
+    std::set<localIdx> baseSet(base.begin(), base.end());
+
+    localIdx apex = -1;
+    for (localIdx n : cell.nodeIds)
+    {
+        if (baseSet.find(n) == baseSet.end())
+        {
+            apex = n;
+            break;
+        }
+    }
+
+    return {base[0], base[1], base[2], base[3], apex};
+}


Same issue as above: localIdx apex = -1; can underflow for unsigned localIdx, and returning it silently can propagate invalid node ids. Avoid negative sentinels; assert that an apex was found (or return an error/optional).

Copilot · 2026-04-29T14:49:19Z

+CellConnectivity extractCellConnectivity(vtkUnstructuredGrid* grid, const Executor& exec)
+{
+    std::vector<std::vector<localIdx>> hostCellToNodes;
+    std::vector<int32_t> hostCellTypes;
+


extractCellConnectivity dereferences grid without validating it. If callers pass a null pointer this will crash; add an explicit check/assert/exception at function entry to fail fast with a clear error.

Copilot · 2026-04-29T14:49:19Z

+SegmentedVector<localIdx, localIdx> buildCellToFaceMapping(
+    const Executor& exec,
+    const Vector<localIdx>& faceOwner,
+    const Vector<localIdx>& faceNeighbour,
+    localIdx nInternalFaces,
+    localIdx nCells
+)
+{
+    localIdx nFaces = faceOwner.size();
+
+    // Step A: Count faces per cell
+    Vector<localIdx> facesPerCell(exec, nCells, localIdx(0));
+    auto fpcView = facesPerCell.view();
+    auto ownerView = faceOwner.view();
+    auto neiView = faceNeighbour.view();
+
+    // Count: internal faces contribute to both owner and neighbour,
+    // boundary faces contribute to owner only.
+    parallelFor(
+        exec,
+        {0, nFaces},
+        NEON_LAMBDA(const localIdx f) {
+            Kokkos::atomic_increment(&fpcView[ownerView[f]]);
+            if (f < nInternalFaces)
+            {
+                Kokkos::atomic_increment(&fpcView[neiView[f]]);
+            }
+        },


buildCellToFaceMapping assumes internal faces are stored first and that faceNeighbour.size() == nInternalFaces; otherwise neiView[f] can read out-of-bounds when f < nInternalFaces. Add input validation (e.g., nInternalFaces <= faceOwner.size(), faceNeighbour.size() == nInternalFaces, and nCells > max(faceOwner/faceNeighbour)) to prevent undefined behavior.

Copilot · 2026-04-29T14:49:19Z

+if(NeoN_WITH_VTK)
+  target_sources(
+    NeoN
+    PRIVATE "mesh/unstructured/io/connectivity/faceTopology.cpp"
+            "mesh/unstructured/io/connectivity/cellReconstruction.cpp"
+            "mesh/unstructured/io/connectivity/nodeOrdering.cpp"
+            "mesh/unstructured/io/connectivity/vtkExtraction.cpp"
+            "mesh/unstructured/io/geometry/faceGeometry.cpp"
+            "mesh/unstructured/io/geometry/cellGeometry.cpp"
+            "mesh/unstructured/io/geometry/computeGeometry.cpp")


These sources (face topology, reconstruction, node ordering, and all geometry) are only compiled/linked when NeoN_WITH_VTK is enabled, but their public headers are always installed and most of the code does not depend on VTK. This makes non-VTK builds expose declarations that either won’t link (undefined symbols) or unnecessarily require enabling VTK. Consider always compiling the non-VTK sources, and gating only vtkExtraction.cpp + VTK link libraries behind NeoN_WITH_VTK (or alternatively add a NF_WITH_VTK compile definition and wrap the public headers/APIs accordingly).

Suggested change

if(NeoN_WITH_VTK)

target_sources(

NeoN

PRIVATE "mesh/unstructured/io/connectivity/faceTopology.cpp"

"mesh/unstructured/io/connectivity/cellReconstruction.cpp"

"mesh/unstructured/io/connectivity/nodeOrdering.cpp"

"mesh/unstructured/io/connectivity/vtkExtraction.cpp"

"mesh/unstructured/io/geometry/faceGeometry.cpp"

"mesh/unstructured/io/geometry/cellGeometry.cpp"

"mesh/unstructured/io/geometry/computeGeometry.cpp")

target_sources(

NeoN

PRIVATE "mesh/unstructured/io/connectivity/faceTopology.cpp"

"mesh/unstructured/io/connectivity/cellReconstruction.cpp"

"mesh/unstructured/io/connectivity/nodeOrdering.cpp"

"mesh/unstructured/io/geometry/faceGeometry.cpp"

"mesh/unstructured/io/geometry/cellGeometry.cpp"

"mesh/unstructured/io/geometry/computeGeometry.cpp")

if(NeoN_WITH_VTK)

target_sources(NeoN PRIVATE "mesh/unstructured/io/connectivity/vtkExtraction.cpp")

Copilot · 2026-04-29T14:49:20Z

+CellConnectivity rebuildCellConnectivity(
+    const Executor& exec,
+    const Vector<localIdx>& faceOwner,
+    const Vector<localIdx>& faceNeighbour,
+    const SegmentedVector<localIdx, localIdx>& faceNodes,
+    localIdx nCells,
+    localIdx nInternalFaces,
+    localIdx nFaces
+)
+{
+    // VTK cell type constants
+    constexpr int VTK_QUAD_TYPE = 9;
+    constexpr int VTK_TETRA_TYPE = 10;
+    constexpr int VTK_HEXAHEDRON_TYPE = 12;
+    constexpr int VTK_WEDGE_TYPE = 13;
+    constexpr int VTK_PYRAMID_TYPE = 14;
+
+    // Copy to host for sequential algorithm
+    auto hostOwner = faceOwner.copyToHost();
+    auto hostNeighbour = faceNeighbour.copyToHost();
+    auto hostFaceNodes = faceNodes.copyToHost();
+
+    auto owView = hostOwner.view();
+    auto neiView = hostNeighbour.view();
+    auto fnView = hostFaceNodes.view();
+
+    // Collect faces per cell
+    std::vector<std::vector<localIdx>> cellFaces(static_cast<std::size_t>(nCells));
+    for (localIdx f = 0; f < nFaces; ++f)
+    {
+        auto oi = static_cast<std::size_t>(owView[f]);
+        cellFaces[oi].push_back(f);
+        if (f < nInternalFaces)
+        {
+            auto ni = static_cast<std::size_t>(neiView[f]);
+            cellFaces[ni].push_back(f);
+        }
+    }


rebuildCellConnectivity/rebuildCellInfo take nFaces as a separate argument but also index faceOwner, faceNeighbour, and faceNodes based on it. If nFaces doesn’t match faceOwner.size()/faceNodes.numSegments() (or if nInternalFaces doesn’t match faceNeighbour.size()), this will go out-of-bounds. Add validation (or derive nFaces from the container sizes to avoid mismatches).

Copilot · 2026-04-29T14:49:20Z

+        const auto& faces = faceTemplateForType(cellType);
+        if (faces.empty()) continue;
+


When faceTemplateForType(cellType) returns empty (unsupported VTK cell type), the code silently continues, producing a FaceTopology that is missing all faces for that cell. This likely leads to incorrect downstream geometry/topology without an obvious failure. Consider failing fast with an assert/exception (or returning an error) when an unsupported cellType is encountered, and document the supported set explicitly.

Copilot · 2026-04-29T14:49:20Z

+struct FaceData
+{
+    localIdx owner {-1};
+    localIdx neighbour {-1};
+    std::vector<localIdx> nodes;
+};


FaceData uses localIdx owner{-1} / neighbour{-1} and later checks like fd.neighbour >= 0. Since localIdx can be unsigned (see NeoN_US_IDX), -1 underflows and neighbour >= 0 will always be true, breaking the internal/boundary partitioning and potentially causing out-of-bounds logic. Use a separate boolean/std::optional to represent “has neighbour”, or use a sentinel like std::numeric_limits<localIdx>::max() and compare against that (and avoid >= 0 tests).

HenningScheufler added 2 commits March 1, 2026 16:19

feat: mesh geometry kernels

32ab1c6

Face area/normal, cell centroid/volume kernels. Depends on connectivity types but has no IO format dependency.

HenningScheufler added the ready-for-review Set this label to indicate that the PR is ready for review label Mar 1, 2026

greole requested a review from Copilot April 29, 2026 14:39

Copilot started reviewing on behalf of greole April 29, 2026 14:40 View session

Copilot AI reviewed Apr 29, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat/mesh geometry#466

Feat/mesh geometry#466
HenningScheufler wants to merge 2 commits into
stack/mesh_iofrom
feat/mesh-geometry

HenningScheufler commented Mar 1, 2026

Uh oh!

github-actions Bot commented Mar 1, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Apr 29, 2026

Uh oh!

Copilot AI Apr 29, 2026

Uh oh!

Copilot AI Apr 29, 2026

Uh oh!

Copilot AI Apr 29, 2026

Uh oh!

Copilot AI Apr 29, 2026

Uh oh!

Copilot AI Apr 29, 2026

Uh oh!

Copilot AI Apr 29, 2026

Uh oh!

Copilot AI Apr 29, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		const auto& faces = faceTemplateForType(cellType);
		if (faces.empty()) continue;

Conversation

HenningScheufler commented Mar 1, 2026

Geometry computation utilities

Uh oh!

github-actions Bot commented Mar 1, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 29, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 29, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 29, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 29, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 29, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 29, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 29, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 29, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants