feat(bevel): carve concave/convex profile into adjacent faces - #301
Conversation
Previously the profile curve was only visible on the chamfer strip itself. The faces perpendicular to the beveled edge (the "adjacent" faces whose corners the bevel cuts) kept a straight diagonal cut, hiding the profile. This commit splices the chain intermediates into the adjacent face's triangulation so the cut polyline follows the chamfer curve. The approach: - Chain building moves before Phase 5 so the neighbor-face retriangu- lation can reference the intermediates. - In the group-merge path of processRingNeighbors, insert the chain interior between uRight and the implicit close-back-to-uLeft. - In processNeighborFace's "both on-edge offsets" branch, do the same for single-face groups (the asymmetric v=v2 case on cubes). - Since the spliced polygon is concave, fan-from-[0] would emit a straight diagonal across the arc. Replaced with a generic ear-clipping helper (triangulatePolygon) that projects into the face's plane, ensures CCW orientation, and runs O(n²) ear-clip. Winding is restored to the caller's original orientation. - Direction of chain insertion on each side is chosen so its chain edges are emitted opposite to the strip's — yielding manifold. - The endpoint corner cap (buildCorner) is skipped when the profile is shaped, since the cap's flat triangle would mask the arc. The original corner vertex (v1 / v2) becomes unreferenced in this case, which is harmless. Tests: 4 new cases covering adjacent-face referencing of intermediates (concave + convex), manifold flat-bevel regression guard, and cap- skip verification for shaped profiles. All 39 bevel tests pass. Version bump 2.28.1 → 2.28.2. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 30 minutes and 38 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe pull request updates the QtMeshEditor project version and implements robust polygon triangulation for mesh beveling operations, replacing fan triangulation with ear-clipping to handle concave polygons. Adds shape detection, chain intermediate generation, and new regression test coverage. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6c120cc9bb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const bool shaped = isV1 ? v1Shaped : v2Shaped; | ||
| if (shaped) return; |
There was a problem hiding this comment.
This return drops the only fallback cap in the polygon.size() == 2 path, which is specifically reached when no ring-edge offsets were emitted (the smooth/no-crease case noted in the surrounding comments). For segmented shaped bevels (segments > 1), the strip does not contribute the direct innerA-innerB edge, so skipping the cap here can leave the chamfer end unclosed (or force the generic hole-filler to patch it unpredictably), regressing manifoldness on smooth character-style meshes.
Useful? React with 👍 / 👎.
| const bool shaped = isV1 ? v1Shaped : v2Shaped; | ||
| if (shaped) return; |
There was a problem hiding this comment.
In the ring.empty() branch there is no neighbor-ring retriangulation step to consume chain edges, so this early return can remove the only endpoint closure triangle for shaped profiles. For interior beveled edges whose endpoint ring fails (e.g., endpoint incident to boundary/non-manifold topology), this creates an uncapped chamfer end and introduces new boundary edges.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/HalfEdgeMesh_test.cpp`:
- Around line 2701-2724: interZpos/interZneg currently collect all new vertices
on the ±Z plane including endpoint offset verts (the v1a/v1b end offsets), which
can make probeFaceAtPlane(refsIntermediate) pass falsely; change the collection
to filter out endpoint-offset vertices so only true chain interior vertices are
included (i.e., when iterating newVerts, skip vertices whose position matches
the known endpoint offset positions or otherwise identify them as the
end-offsets) before calling probeFaceAtPlane for probeFront/probeBack; apply the
identical filtering change in the convex-case code path referenced around lines
2794-2805 so both tests only pass interior chain verts to probeFaceAtPlane.
In `@src/HalfEdgeMesh.cpp`:
- Around line 2245-2250: The early return when ring.empty() and shaped (using
isV1 ? v1Shaped : v2Shaped) is too aggressive: instead of unconditionally
skipping shaped endpoint caps, verify that every segment of the corresponding
vChain (each chain[i]→chain[i+1]) is already emitted/covered before returning;
if any chain edge is not covered, do not return—emit a per-segment cap fan for
each chain segment (rather than relying only on the polygon.size() == 2
innerA-innerB chord check). Update the logic in the ring.empty() branch (and the
analogous block at lines ~2364-2375) to iterate the chain segments and gate the
shaped-skip on all being emitted, otherwise fall back to the topology-safe cap
per-segment emission.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 76d5c988-2728-42f9-81cf-23fd153269ac
📒 Files selected for processing (3)
CMakeLists.txtsrc/HalfEdgeMesh.cppsrc/HalfEdgeMesh_test.cpp
Codex + CodeRabbit flagged that the blanket cap-skip in buildCorner can leave shaped bevel endpoints open in two cases: 1. ring.empty() — no neighbor-face retriangulation runs, so the chain edges have no partner from the face side. For shaped profiles, fan v across the full chain polyline so each chain edge gets paired with a cap triangle. Flat profile still emits the original 3-vertex cap. 2. polygon.size() == 2 (smooth/no-crease case) — before skipping, walk the chain and verify every chain[i]↔chain[i+1] is already an emitted edge. If yes, the splice + strip covered the end, skip. If not, fan v across the chain as in case (1). Also tighten the CarvesAdjacentFaces tests (CodeRabbit minor): filter to strict chain-interior intermediates (positions off the two bevel- boundary cube edges at the endpoint) so the probe can't be satisfied by the end offsets alone. Convex test uses 0.75 instead of 1.0 because max convex pushes intermediates past the face plane's own edge, which isn't useful for the probe. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
1 participant
Summary
Details
Previously the chamfer strip showed the profile curve correctly, but the two faces perpendicular to the beveled edge kept a straight diagonal cut. That's because Phase 5 neighbor retriangulation ran before the chain intermediates were built, and its fan-from-vertex triangulation covered the cut region with a straight spoke that hid the arc.
This PR:
triangulatePolygon) so the resulting concave polygons triangulate without artificial diagonals.buildCorner) when the profile is shaped, since the flat cap would otherwise mask the arc.Tests
4 new unit tests verify:
Test plan
UnitTests --gtest_filter="*Bevel*"— 39 bevel tests pass.Summary by CodeRabbit
Chores
Bug Fixes
Tests