fix(quads): bug-fix sweep from PR #347 review (8 of 10 items) - #352
Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ 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 (7)
✨ 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. Review rate limit: 0/1 reviews remaining, refill in 31 minutes and 39 seconds.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: 8a11e4dd08
ℹ️ 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".
| m_selectedVertices.erase(g0); | ||
| m_selectedEdges.erase({std::min(g0, g1), std::max(g0, g1)}); | ||
| } |
There was a problem hiding this comment.
When two adjacent faces are selected, ctrl-deselecting one face now unconditionally erases all of that face’s vertices/edges from m_selectedVertices and m_selectedEdges, even if those elements are still part of another selected face. This makes edge/vertex overlays and selection counts inconsistent with m_selectedFaces for multi-face selections (e.g., shared edge disappears while the neighboring face remains selected). The deselect path needs to recompute or reference-count shared elements instead of blindly erasing them.
Useful? React with 👍 / 👎.
Address the deferred items from PR #347 review (#326 follow-up). Eight discrete fixes; the remaining two (vertex-knife near-duplicate on OnVertex hits, subdivideFacesToQuads T-junctions) are heavier lifts deferred to a separate PR. 1. HalfEdgeMesh::loopCut rejects mixed quad/tri adjacency upfront. Previously walked f1 and f2 independently, so a quad+tri start edge produced a one-sided cut on the quad side and silently mutated topology. (Codex P1.) +1 unit test. 2. applyWireframeMaterials handles mixed meshes per submesh. Tri-only submeshes inside a mesh that had ANY n-gon submesh were losing wireframe entirely (boundary overlay only emits n-gon submeshes). Now PM_WIREFRAME on tri-only submeshes coexists with the boundary overlay on n-gon submeshes. (Codex P2.) 3. buildSubMeshBuffers explicitly clears the GPU vertex/index buffers when the submesh has no vertices or no triangles to draw. Prior early-return left stale buffers attached, so deleting the last face in a submesh kept the old geometry rendering forever. 4. selectedFacesAsHEFaceIndices compacts invalid faces in the per-face mapping, not just the per-submesh base offset. Without this, a selected face whose raw index is shifted by an earlier invalid face mapped to the wrong HE face — silently breaking face-mode extrude/delete/dissolve/subdivide. 5. canConvertToQuads predicate replaces !isMeshQuadBased on the toolbar gate. Mixed meshes (some submeshes quad, some tri) still have tri-only submeshes worth merging — the previous check wrongly disabled the action. 6. convertToQuads correctly returns non-zero on promote-only runs. Tracks promotion count separately from merge count, returns their sum. Previous return-of-totalMerges falsely reported 0 when only the n-gon promotion happened. 7. deselectFace mirrors selectFace's vertex/edge dilation. Without this, ctrl-deselect erased the face triangles but left perimeter vertex/edge entries — looked like a stuck partial selection. 8. Test in EditModeControllerBevelE2E uses a real (0.001f) translate instead of Vector3::ZERO so the commit-path assertion stays meaningful even if the controller ever short-circuits zero deltas. Two items remain for a follow-up PR: - Vertex knife OnVertex clicks producing near-duplicate points (the click is converted to first-incident-edge + t=0/1 then splitEdge clamps it; need a different splitFace path for true vertex hits). - subdivideFacesToQuads T-junctions on partial n-gon selections (needs adjacent-face retriangulation pass like the existing tri subdivide does). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
8a11e4d to
f28e7f4
Compare
|
1 participant
Summary
Addresses the 10 deferred review items from PR #347 epic merge. 8 fixed in this PR, the remaining 2 are heavier lifts tracked for a separate PR.
What's fixed
Major bugs
Deferred to separate follow-up PR
Test plan
🤖 Generated with Claude Code