Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 39 additions & 0 deletions src/EditableMesh.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,45 @@ THE SOFTWARE.
#include <cmath>
#include <cstring>

void triangulateFaces(EditableSubMesh& sub)
{
sub.triangles.clear();
if (sub.faces.empty()) return;

// Best-effort capacity hint: every face emits N - 2 triangles, so
// for a quad-dominant mesh the average is ~2× the face count. The
// caller may have any mix of polygon sizes, so we don't try to be
// exact here.
sub.triangles.reserve(sub.faces.size() * 2);

for (const auto& face : sub.faces) {
if (!face.isValid()) continue;
const auto& idx = face.indices;
for (size_t i = 1; i + 1 < idx.size(); ++i) {
EditableTriangle t;
t.indices[0] = idx[0];
t.indices[1] = idx[i];
t.indices[2] = idx[i + 1];
Comment on lines +52 to +56

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Reject out-of-range face indices during triangulation

triangulateFaces emits triangles directly from face.indices without checking that each index is < sub.vertices.size(). If a caller passes a face with an out-of-range index, this function mirrors invalid indices into sub.triangles (the legacy path used by buffer upload), which can produce invalid index buffers and undefined rendering behavior. buildFromEditableMesh already treats these faces as invalid, so this helper should enforce the same bound check before emitting triangles.

Useful? React with 👍 / 👎.

sub.triangles.push_back(t);
}
}
}

void promoteTrianglesToFaces(EditableSubMesh& sub)
{
sub.faces.clear();
sub.faces.reserve(sub.triangles.size());
for (const auto& tri : sub.triangles) {
EditableFace f;
f.indices = {tri.indices[0], tri.indices[1], tri.indices[2]};
sub.faces.push_back(std::move(f));
}
// Note: we deliberately do NOT call triangulateFaces() here. The
// existing `triangles` already mirrors what `faces` would generate
// (each face is a single triangle), so the canonical-faces invariant
// already holds.
}

bool EditableMesh::loadFromEntity(Ogre::Entity* entity)
{
if (!entity)
Expand Down
84 changes: 82 additions & 2 deletions src/EditableMesh.h
Original file line number Diff line number Diff line change
Expand Up @@ -68,19 +68,99 @@ struct EditableTriangle {
unsigned int indices[3] = {0, 0, 0};
};

/**
* @brief A polygonal face referencing N >= 3 vertices by index.
*
* Used for the n-gon (quad-aware) representation that complements the
* legacy triangle-only path. While the editor is being migrated to
* quads, both `EditableSubMesh::faces` and `EditableSubMesh::triangles`
* coexist:
*
* - When `faces` is empty, the submesh is in legacy triangle-only mode
* and `triangles` is the canonical storage.
* - When `faces` is non-empty, `faces` is canonical and `triangles` is
* a fan-triangulated mirror that downstream code (GPU upload, render,
* legacy topology ops) consumes. Helper utilities keep the two
* representations in sync.
*
* The fan-triangulation rule is `[v0, vi, vi+1]` for `i in [1, N-1)`,
* matching what `HalfEdgeMesh::appendFace` and `fillSelection` already
* produce. Convex polygons (the only case the importer is expected to
* produce) survive that rule cleanly; concave n-gons need a future
* ear-clip pass.
*/
struct EditableFace {
std::vector<unsigned int> indices;

/// @return true if this face has at least 3 vertices and no
/// consecutive duplicate index pair (naive sanity check).
bool isValid() const
{
if (indices.size() < 3) return false;
for (size_t i = 0; i < indices.size(); ++i) {
if (indices[i] == indices[(i + 1) % indices.size()]) return false;
}
return true;
}

/// @return Number of vertices on this face (>= 3 when valid).
size_t vertexCount() const { return indices.size(); }
};

/**
* @brief An editable copy of a single Ogre SubMesh.
*
* Contains all vertices, triangles, and the material name. Tracks whether
* the original SubMesh used shared vertex data.
* Contains all vertices, triangles, faces, and the material name. Tracks
* whether the original SubMesh used shared vertex data.
*
* The `faces` (n-gon) array is the canonical face storage when it is
* non-empty; `triangles` then mirrors it as a fan-triangulation kept in
* sync by `triangulateFaces()`. When `faces` is empty, `triangles` is
* canonical and the submesh is in legacy triangle-only mode.
*/
struct EditableSubMesh {
std::vector<EditableVertex> vertices;
std::vector<EditableTriangle> triangles;
std::vector<EditableFace> faces;
std::string materialName;
bool usesSharedVertices = false;
};

/**
* @brief Fan-triangulate every face in `faces` into `triangles`.
*
* Triangulation rule: each face emits `n - 2` triangles fanned from
* `indices[0]`, i.e. `(v0, v_i, v_{i+1})` for `i in [1, n-1)`. This
* matches `HalfEdgeMesh::appendFace` and `fillSelection` so a face that
* survives an HE round-trip recovers the same triangulation it started
* with.
*
* Caller's responsibility: call this whenever `faces` is mutated to
* keep `triangles` in sync. Code paths that don't yet understand n-gons
* (GPU upload, legacy topology ops) consume `triangles`.
*
* Skips invalid faces (`vertexCount < 3`) silently. Replaces any
* existing contents of `triangles`.
*
* @param sub The submesh whose `faces` will be triangulated. After
* return, `sub.triangles` contains the fan-triangulation.
*/
void triangulateFaces(EditableSubMesh& sub);

/**
* @brief Build trivial single-triangle faces from existing `triangles`.
*
* Inverse of `triangulateFaces` — used when a legacy triangle-only
* submesh needs to be promoted into the n-gon representation. After the
* call, every triangle has a corresponding 3-index `EditableFace`. The
* existing `triangles` array is preserved (the result still satisfies
* the canonical-faces invariant: `triangles[i] == fan(faces[i])`).
*
* @param sub The submesh to promote. Existing contents of `sub.faces`
* are replaced.
*/
void promoteTrianglesToFaces(EditableSubMesh& sub);

/**
* @brief Indexed mesh representation for topology queries and editing.
*
Expand Down
137 changes: 107 additions & 30 deletions src/HalfEdgeMesh.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@
#include <tuple>
#include <unordered_set>

bool HalfEdgeMesh::buildFromEditableMesh(const EditableMesh& editableMesh)

Check failure on line 40 in src/HalfEdgeMesh.cpp

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Refactor this function to reduce its Cognitive Complexity from 69 to the 25 allowed.

See more on https://sonarcloud.io/project/issues?id=fernandotonon_QtMeshEditor&issues=AZ3S6OTkAC-0TuXDv0o5&open=AZ3S6OTkAC-0TuXDv0o5&pullRequest=327
{
m_halfEdges.clear();
m_vertices.clear();
Expand Down Expand Up @@ -90,27 +90,65 @@
}
}

// Phase 2: Append a face/3-HE triangle for each input triangle, skipping
// degenerates. Vertex outgoing-HE pointers are set during this loop.
// Phase 2: Append a face for each input face. Each submesh prefers
// `faces` (n-gon canonical storage) when non-empty; otherwise it
// falls back to `triangles` (legacy storage). The two-stream rule
// mirrors EditableSubMesh's invariant: `faces` is canonical iff it
// is non-empty. Skips degenerates and sets vertex outgoing-HE
// pointers as faces are added.
auto registerVertexHE = [this](int vGlobal, int heStart) {
if (m_vertices[vGlobal].halfEdge == -1) {
m_vertices[vGlobal].halfEdge = heStart;
}
};

for (int s = 0; s < m_subMeshCount; ++s) {
const auto& sub = subMeshes[s];
for (const auto& tri : sub.triangles) {
int v0 = vertexMap[s][tri.indices[0]];
int v1 = vertexMap[s][tri.indices[1]];
int v2 = vertexMap[s][tri.indices[2]];

// Skip degenerate triangles (any two vertices identical)
if (v0 == v1 || v1 == v2 || v0 == v2)
continue;
if (!sub.faces.empty()) {
for (const auto& face : sub.faces) {
Comment on lines +108 to +109

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Fall back to triangles when canonical faces are stale

This branch unconditionally ignores sub.triangles whenever sub.faces is non-empty, but existing mesh mutators like EditableMesh::weldByPosition and collapseToSingleSubmeshAndWeld still rewrite only triangles. After a quad submesh goes through one of those paths, faces can contain pre-edit indices while triangles contain post-edit indices; rebuilding from this code then replays stale geometry (or skips faces as out-of-range), silently dropping the edit. A guarded fallback to triangles when a face stream is invalid/stale would avoid this data loss.

Useful? React with 👍 / 👎.

if (!face.isValid()) continue;

Check failure on line 110 in src/HalfEdgeMesh.cpp

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Refactor this code to not nest more than 3 if|for|do|while|switch statements.

See more on https://sonarcloud.io/project/issues?id=fernandotonon_QtMeshEditor&issues=AZ3S6OTkAC-0TuXDv0o6&open=AZ3S6OTkAC-0TuXDv0o6&pullRequest=327
std::vector<int> verts;
verts.reserve(face.indices.size());
bool degenerate = false;
for (unsigned int local : face.indices) {

Check failure on line 114 in src/HalfEdgeMesh.cpp

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Refactor this code to not nest more than 3 if|for|do|while|switch statements.

See more on https://sonarcloud.io/project/issues?id=fernandotonon_QtMeshEditor&issues=AZ3S6OTkAC-0TuXDv0o7&open=AZ3S6OTkAC-0TuXDv0o7&pullRequest=327
if (local >= vertexMap[s].size()) {
degenerate = true;
break;
}
verts.push_back(vertexMap[s][local]);
}
if (degenerate) continue;

Check failure on line 121 in src/HalfEdgeMesh.cpp

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Refactor this code to not nest more than 3 if|for|do|while|switch statements.

See more on https://sonarcloud.io/project/issues?id=fernandotonon_QtMeshEditor&issues=AZ3S6OTkAC-0TuXDv0o8&open=AZ3S6OTkAC-0TuXDv0o8&pullRequest=327
// Reject faces with any duplicated vertex (would create
// a zero-area corner). Cheap O(N²) check is fine for
// typical N <= 8.
for (size_t i = 0; i < verts.size() && !degenerate; ++i) {

Check failure on line 125 in src/HalfEdgeMesh.cpp

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Refactor this code to not nest more than 3 if|for|do|while|switch statements.

See more on https://sonarcloud.io/project/issues?id=fernandotonon_QtMeshEditor&issues=AZ3S6OTkAC-0TuXDv0o9&open=AZ3S6OTkAC-0TuXDv0o9&pullRequest=327

Check warning on line 125 in src/HalfEdgeMesh.cpp

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Refactor this loop so that it is less error-prone.

See more on https://sonarcloud.io/project/issues?id=fernandotonon_QtMeshEditor&issues=AZ3S6OTkAC-0TuXDv0pC&open=AZ3S6OTkAC-0TuXDv0pC&pullRequest=327
for (size_t j = i + 1; j < verts.size(); ++j) {
if (verts[i] == verts[j]) { degenerate = true; break; }
}
}
if (degenerate) continue;

Check failure on line 130 in src/HalfEdgeMesh.cpp

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Refactor this code to not nest more than 3 if|for|do|while|switch statements.

See more on https://sonarcloud.io/project/issues?id=fernandotonon_QtMeshEditor&issues=AZ3S6OTkAC-0TuXDv0o-&open=AZ3S6OTkAC-0TuXDv0o-&pullRequest=327

int he0 = static_cast<int>(m_halfEdges.size());
appendFace(verts, s);
for (size_t i = 0; i < verts.size(); ++i) {

Check failure on line 134 in src/HalfEdgeMesh.cpp

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Refactor this code to not nest more than 3 if|for|do|while|switch statements.

See more on https://sonarcloud.io/project/issues?id=fernandotonon_QtMeshEditor&issues=AZ3S6OTkAC-0TuXDv0o_&open=AZ3S6OTkAC-0TuXDv0o_&pullRequest=327
registerVertexHE(verts[i], he0 + static_cast<int>(i));
}
}
} else {
for (const auto& tri : sub.triangles) {
int v0 = vertexMap[s][tri.indices[0]];
int v1 = vertexMap[s][tri.indices[1]];
int v2 = vertexMap[s][tri.indices[2]];

if (v0 == v1 || v1 == v2 || v0 == v2) continue;

Check failure on line 144 in src/HalfEdgeMesh.cpp

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Refactor this code to not nest more than 3 if|for|do|while|switch statements.

See more on https://sonarcloud.io/project/issues?id=fernandotonon_QtMeshEditor&issues=AZ3S6OTkAC-0TuXDv0pA&open=AZ3S6OTkAC-0TuXDv0pA&pullRequest=327

int he0 = static_cast<int>(m_halfEdges.size());
appendTriangle(v0, v1, v2, s);
int he0 = static_cast<int>(m_halfEdges.size());

Check warning on line 146 in src/HalfEdgeMesh.cpp

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Replace the redundant type with "auto".

See more on https://sonarcloud.io/project/issues?id=fernandotonon_QtMeshEditor&issues=AZ3S6OTkAC-0TuXDv0pD&open=AZ3S6OTkAC-0TuXDv0pD&pullRequest=327
appendTriangle(v0, v1, v2, s);

// Set vertex outgoing half-edge pointers (first wins)
int verts[3] = {v0, v1, v2};
for (int i = 0; i < 3; ++i) {
if (m_vertices[verts[i]].halfEdge == -1) {
m_vertices[verts[i]].halfEdge = he0 + i;
int verts[3] = {v0, v1, v2};
for (int i = 0; i < 3; ++i) {

Check failure on line 150 in src/HalfEdgeMesh.cpp

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Refactor this code to not nest more than 3 if|for|do|while|switch statements.

See more on https://sonarcloud.io/project/issues?id=fernandotonon_QtMeshEditor&issues=AZ3S6OTkAC-0TuXDv0pB&open=AZ3S6OTkAC-0TuXDv0pB&pullRequest=327
registerVertexHE(verts[i], he0 + i);
}
}
}
Expand Down Expand Up @@ -392,23 +430,34 @@
// Per-submesh: map from HE vertex index -> local vertex index
std::vector<std::unordered_map<int, int>> vertexRemap(m_subMeshCount);

// Track whether any submesh ended up with a non-triangle face. If so,
// we mirror the face list into `EditableSubMesh::faces` so downstream
// n-gon-aware code (chunks 2+) sees the polygon structure. If every
// face is a triangle, we leave `faces` empty to keep the legacy path
// canonical.
std::vector<bool> hasNonTriangleFace(m_subMeshCount, false);

for (int f = 0; f < static_cast<int>(m_faces.size()); ++f) {
const auto& heFace = m_faces[f];
int subIdx = heFace.subMeshIndex;
auto& outSub = outSubMeshes[subIdx];

auto verts = faceVertices(f);
if (verts.size() != 3)
continue;

EditableTriangle tri;
for (int i = 0; i < 3; ++i) {
int heVertIdx = verts[i];

// Accept any face with 3+ vertices. n-gons (4+) get fan-
// triangulated into the legacy `triangles` array AND mirrored
// into `faces` so the n-gon structure survives the round-trip.
// Faces with < 3 vertices are degenerate — silently skip.
if (verts.size() < 3) continue;

// Map every face vertex into the submesh's local vertex array,
// collecting the local indices in `localIdxs` for later.
std::vector<unsigned int> localIdxs;
localIdxs.reserve(verts.size());
for (int heVertIdx : verts) {
auto it = vertexRemap[subIdx].find(heVertIdx);
int localIdx;
if (it == vertexRemap[subIdx].end()) {
// Add this vertex to the submesh
int localIdx = static_cast<int>(outSub.vertices.size());
localIdx = static_cast<int>(outSub.vertices.size());
vertexRemap[subIdx][heVertIdx] = localIdx;

EditableVertex ev;
Expand All @@ -431,13 +480,39 @@
}

outSub.vertices.push_back(std::move(ev));
tri.indices[i] = localIdx;
} else {
tri.indices[i] = it->second;
localIdx = it->second;
}
localIdxs.push_back(static_cast<unsigned int>(localIdx));
}

// Fan-triangulate the face into `triangles` (legacy storage):
// tris = (v0, v_i, v_{i+1}) for i in [1, N-1)
for (size_t i = 1; i + 1 < localIdxs.size(); ++i) {
EditableTriangle tri;
tri.indices[0] = localIdxs[0];
tri.indices[1] = localIdxs[i];
tri.indices[2] = localIdxs[i + 1];
outSub.triangles.push_back(tri);
}

outSub.triangles.push_back(tri);
// Always record the face in `EditableSubMesh::faces` so the n-gon
// round-trip is information-preserving. We finalise below by
// clearing `faces` on submeshes that turned out to be all
// triangles (legacy invariant: faces is non-empty only when
// there's actually polygonal information to preserve).
EditableFace face;
face.indices = std::move(localIdxs);
if (face.indices.size() > 3) hasNonTriangleFace[subIdx] = true;
outSub.faces.push_back(std::move(face));
}

// Finalise: drop `faces` on triangle-only submeshes. This keeps the
// legacy invariant that `faces` is empty unless the submesh really
// contains n-gons, so downstream code that doesn't yet understand
// n-gons (every consumer of EditableSubMesh today) keeps working.
for (int s = 0; s < m_subMeshCount; ++s) {
if (!hasNonTriangleFace[s]) outSubMeshes[s].faces.clear();
}

return true;
Expand Down Expand Up @@ -4833,8 +4908,10 @@
return false;
} while (he != startHE);

// Triangles should have exactly 3 half-edges
if (count != 3)
// Polygonal faces have at least 3 half-edges. Larger N-gons
// (quads etc.) are allowed by the data model — chunks 1+ of the
// quad migration drop the triangle-only assumption.
if (count < 3)
return false;
}

Expand Down
Loading
Loading