Skip to content

Merge some squares#498

Open
julialongtin wants to merge 4 commits intomasterfrom
merge_some_squares
Open

Merge some squares#498
julialongtin wants to merge 4 commits intomasterfrom
merge_some_squares

Conversation

@julialongtin
Copy link
Member

@julialongtin julialongtin commented Mar 2, 2026

fixes: #98

remove redundant otherwise.
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR enhances the rendering pipeline’s quad handling by (1) emitting explicit quads during loop tessellation and (2) iteratively merging adjacent quads on the same plane to reduce triangle count, supported by type updates to carry original quad corner coordinates.

Changes:

  • Emit Sq quads for 4-edge loops (rectangle/parallelogram detection) and store original corner coordinates.
  • Update square-merging logic to iteratively merge adjacent quads and triangulate using stored original corners.
  • Extend core render types with additional fields/derivations to support equality checks and debugging.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.

File Description
Graphics/Implicit/Export/Render/TesselateLoops.hs Adds active quad construction for 4-point loops with a computed basis/intervals and stored original corners.
Graphics/Implicit/Export/Render/HandleSquares.hs Refactors merging into a fixpoint loop and updates merge rules to validate adjacency via vertex proximity; triangulates from stored corners.
Graphics/Implicit/Export/Render/Definitions.hs Expands TriSquare.Sq into a record including original quad coordinates; derives Show/Eq; updates NFData.
Graphics/Implicit/Definitions.hs Derives Show/Eq for Triangle and TriangleMesh to support comparisons/debugging.

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

Comment on lines +72 to +75
(~=) u v = quadrance (u - v) <= eps
-- Our fudge factor.
eps :: ℝ
eps = 1e-8
Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

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

quadrance returns a squared distance, but the comparison uses a linear epsilon (quadrance (u - v) <= eps). If eps is intended as a distance tolerance, it should be compared against eps*eps (or use norm/distance instead). As written, the effective distance tolerance is sqrt eps, which is easy to mis-tune and can accept much larger deviations than intended.

Suggested change
(~=) u v = quadrance (u - v) <= eps
-- Our fudge factor.
eps ::
eps = 1e-8
(~=) u v = quadrance (u - v) <= eps * eps
-- Our fudge factor: linear distance tolerance.
eps ::
eps = 1e-4

Copilot uses AI. Check for mistakes.
mergeAdjacent :: [TriSquare] -> [TriSquare]
mergeAdjacent (pres@(Sq _ _ (V2 x1a x2a) _ (pa1, pa2, pa3, pa4)) : next@(Sq _ _ (V2 x1b x2b) _ (pb1, pb2, pb3, pb4)) : others)
-- Note: we used to have two cases here, was the other one just not needed?
| x2a ~= x1b && pa1 .= pb2 && pb3 .= pa4 = mergeAdjacent (Sq b z (V2 x1a x2b) yS (pb1, pa2, pa3, pb4) : others)
Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

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

In joinYaligned, the adjacency vertex checks and the merged _origCoords tuple look inconsistent with the earlier joinXaligned convention (where (a,b) is the bottom edge and (c,d) is the top edge). For x-adjacent squares (x2a ~= x1b), the shared edge should be pres’ right edge (pa2,pa3) matching next’s left edge (pb1,pb4), but the current guard checks pa1 ~= pb2 and pb3 ~= pa4, and the merged coord tuple (pb1, pa2, pa3, pb4) can become degenerate on typical inputs. Please align the vertex-equality checks and the merged coordinate selection with the intended shared edge for x-direction merges.

Suggested change
| x2a ~= x1b && pa1 .= pb2 && pb3 .= pa4 = mergeAdjacent (Sq b z (V2 x1a x2b) yS (pb1, pa2, pa3, pb4) : others)
-- Merge two squares that are adjacent along the x-direction, sharing
-- pres's right edge (pa2,pa3) with next's left edge (pb1,pb4).
| x2a ~= x1b && pa2 .= pb1 && pa3 .= pb4 =
mergeAdjacent (Sq b z (V2 x1a x2b) yS (pa1, pb2, pb3, pa4) : others)

Copilot uses AI. Check for mistakes.
Comment on lines +116 to 119
| y2a ~= y1b && pa3 .= pb2 && pa4 .= pb1 = mergeAdjacent (Sq b z xS (V2 y1a y2b) (pa1, pa2, pb3, pb4) : others)
-- Note: we used to have two cases here, was the other one just not needed?
| y1a ~= y2b = error $ "Other path chosen.\n" <> show pres <> "\n" <> show next <> "\n"
| otherwise = pres : mergeAdjacent (next : others)
Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

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

This debug error branch will crash at runtime if the reverse-adjacency case is encountered (even due to a degenerate/near-zero interval or unexpected ordering). It would be safer to either handle the symmetric merge case (as the previous implementation did) or fall back to leaving the squares unmerged instead of terminating the whole render.

Copilot uses AI. Check for mistakes.
Comment on lines +142 to +143
| x2a ~= x1b && pa1 .= pb2 && pb3 .= pa4 = mergeAdjacent (Sq b z (V2 x1a x2b) yS (pb1, pa2, pa3, pb4) : others)
| x1a ~= x2b = error $ "Other path chosen.\n" <> show pres <> "\n" <> show next <> "\n"
Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

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

This debug error branch will crash at runtime if the reverse-adjacency case is encountered. Consider handling the symmetric merge case (like the pre-PR code) or treating it as a non-mergeable pair instead of terminating the entire render.

Copilot uses AI. Check for mistakes.
-- Then repeat.
finishedSquares :: [TriSquare]
finishedSquares = concat $ until (\xs -> attemptJoin xs == xs) attemptJoin <$> planeAligned
-- Merge them back together, and we have the desired reult!
Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

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

Typo in comment: “reult” should be “result”.

Suggested change
-- Merge them back together, and we have the desired reult!
-- Merge them back together, and we have the desired result!

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Floating polygons in output

2 participants