Skip to content

Expose the uv/t coordinates of an edge–triangle intersection - #245

Merged
zfergus merged 4 commits into
mainfrom
feature/intersection-coordinates
Jul 30, 2026
Merged

Expose the uv/t coordinates of an edge–triangle intersection#245
zfergus merged 4 commits into
mainfrom
feature/intersection-coordinates

Conversation

@zfergus

@zfergus zfergus commented Jul 30, 2026

Copy link
Copy Markdown
Member

Description

Adds a variant of the edge–triangle intersection test that reports where the
intersection happens, not just whether it happens.

is_edge_intersecting_triangle() already computes the barycentric coordinates
(u, v) on the triangle and the parameter t along the edge internally, then
throws them away. Downstream code that wants the intersection point had to
recompute them (and, in the rational build, redo the exact arithmetic).

New API:

bool edge_triangle_intersection(
    Eigen::ConstRef<Eigen::Vector3d> e0, Eigen::ConstRef<Eigen::Vector3d> e1,
    Eigen::ConstRef<Eigen::Vector3d> t0, Eigen::ConstRef<Eigen::Vector3d> t1,
    Eigen::ConstRef<Eigen::Vector3d> t2,
    double& u, double& v, double& t);

is_edge_intersecting_triangle() is now a thin wrapper over it, so both share
the same robust orient3d plane-side gate and the same solve — no behavioral
change to the existing predicate. The rational path
(IPC_TOOLKIT_WITH_RATIONAL_INTERSECTION) writes the coordinates out as well.

Also adds CollisionMesh::face_normals(), a TBB-parallel helper returning the
per-face unit normals for a given set of vertex positions — the companion piece
needed to orient/interpret an intersection once you have its location.

Type of change

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

New tests/src/tests/geometry/test_intersection.cpp:

  • Vertical edge through the triangle interior — checks t == 0.5 and that
    (u, v) is a valid barycentric coordinate.
  • Edge entirely on one side of the triangle's plane — no hit.

Also confirmed the refactor of is_edge_intersecting_triangle() is
behavior-preserving by running the existing suite:

$ ./build/host-test/tests/ipc_toolkit_tests "[intersections]"
All tests passed (6 assertions in 1 test case)

$ ./build/host-test/tests/ipc_toolkit_tests "[intersection]"
All tests passed (960 assertions in 1 test case)

Test Configuration:

  • OS and Version: macOS (Darwin 25.5.0, arm64)
  • Compiler and Version: AppleClang 21.0, C++17, Release

Checklist

  • I have followed the project style guide
  • My code follows the clang-format style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation (Doxygen comments on both new public functions)
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

Notes for review

  • Python bindings are now included. edge_triangle_intersection() is bound
    returning an (intersects, u, v, t) tuple (Python has no out-params), and
    CollisionMesh.face_normals() returns an (#F × 3) array rather than a list
    of vectors, to match the other per-element accessors. Since the C++ side only
    asserts the 3D requirement — a no-op in release builds — the binding raises
    ValueError on a 2D mesh instead of invoking UB. Both have tests mirroring
    the C++ ones, verified with nose2 -v -s python/tests as CI runs them.
  • Added a C++ test for CollisionMesh::face_normals (rest positions, deformed
    positions, flipped winding). This was the codecov patch failure — the
    function had no coverage at all.
  • The out-param contract raised in review is now enforced rather than just
    documented: the outputs are seeded to NaN and written only on a confirmed
    intersection, in both the rational and floating-point paths. The float
    path had the same bug (it wrote u/v/t before its range check), so the
    two build configurations now agree.
  • Verified against a -DIPC_TOOLKIT_WITH_RATIONAL_INTERSECTION=ON build as
    well as the default; the rational path had never been compiled before this.

Copilot AI review requested due to automatic review settings July 30, 2026 04:12

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

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 extends the geometry intersection utilities by adding an edge–triangle intersection API that returns the intersection location (triangle barycentric coordinates and edge parameter), while keeping the existing boolean predicate behavior via a wrapper. It also adds a CollisionMesh::face_normals() helper to compute per-face unit normals in parallel, plus targeted unit tests for the new intersection API.

Changes:

  • Add edge_triangle_intersection() returning (u, v, t) and refactor is_edge_intersecting_triangle() into a wrapper.
  • Add CollisionMesh::face_normals() (TBB-parallel) to compute per-face unit normals for provided vertex positions.
  • Add a new Catch2 test file and wire it into the geometry test CMake target.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
tests/src/tests/geometry/test_intersection.cpp Adds unit tests validating edge_triangle_intersection() outputs on a hit and a clear miss case.
tests/src/tests/geometry/CMakeLists.txt Registers the new geometry intersection test source.
src/ipc/geometry/intersection.hpp Declares the new intersection API and documents its parameters.
src/ipc/geometry/intersection.cpp Implements edge_triangle_intersection(), routes the old predicate through it, and extends the rational path to output coordinates.
src/ipc/collision_mesh.hpp Declares CollisionMesh::face_normals() public helper.
src/ipc/collision_mesh.cpp Implements face_normals() using tbb::parallel_for and triangle_normal().
Comments suppressed due to low confidence (1)

src/ipc/geometry/intersection.cpp:146

  • edge_triangle_intersection() can return false from the plane-side gate without writing u/v/t, leaving out-params indeterminate. Initializing them at the top avoids accidental use of uninitialized outputs (and makes behavior consistent across early-exit paths).
{
    // Robust plane-side gate (same as is_edge_intersecting_triangle): both edge
    // endpoints strictly on one side of the triangle's plane ⇒ no crossing.
    igl::predicates::exactinit();

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

Comment thread src/ipc/geometry/intersection.hpp Outdated
Comment thread src/ipc/geometry/intersection.cpp Outdated
Comment thread src/ipc/geometry/intersection.cpp
@codecov

codecov Bot commented Jul 30, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 96.58%. Comparing base (4bb15db) to head (d2173a9).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #245   +/-   ##
=======================================
  Coverage   96.57%   96.58%           
=======================================
  Files         163      163           
  Lines       16656    16673   +17     
  Branches      922      922           
=======================================
+ Hits        16086    16104   +18     
+ Misses        570      569    -1     
Flag Coverage Δ
unittests 96.58% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

zfergus and others added 3 commits July 29, 2026 21:34
- Fix the Doxygen comment: (a, b) -> (u, v), and state that boundary hits
  count as intersections (the comparisons are inclusive).
- Write the out-params only once an intersection is confirmed, in both the
  rational and floating-point paths, so a false return never leaves them
  partially populated. They are seeded to NaN on entry, so every return
  path is deterministic.
- The degenerate rational case (d.sign() == 0) still conservatively returns
  true, but now documents that the coordinates stay NaN because they are not
  uniquely defined there.
- Fix typo: "completly" -> "completely".
- Extend the tests: hit-point round-trip through both parameterizations,
  a plane-crossing miss outside the triangle, NaN checks on misses, and
  agreement with is_edge_intersecting_triangle.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
- Bind edge_triangle_intersection(), returning a (intersects, u, v, t) tuple
  since Python has no out-params. Documents the NaN contract.
- Bind CollisionMesh.face_normals(), returning an (#F × 3) array rather than a
  list of vectors, to match the other per-element accessors. The C++ side only
  asserts the 3D requirement, which is a no-op in release builds, so the
  binding raises ValueError on a 2D mesh instead of invoking UB.
- Add tests for both, mirroring the C++ tests.

Also add a C++ test for CollisionMesh::face_normals (rest positions, deformed
positions, and flipped winding), which had no coverage — this is what the
codecov patch check was failing on.

Drive-by: test_faces_to_edges used the nose-style `yield` form, which modern
pytest rejects at collection, taking the whole test_collision_mesh.py file down
with it. Converted to @pytest.mark.parametrize plus a pytest.raises case so the
file collects. CI does not run pytest, which is why this went unnoticed.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The previous commit converted test_faces_to_edges from the nose-style `yield`
form to @pytest.mark.parametrize. That was wrong: python.yml runs the suite with
nose2, and pytest is not in python/tests/requirements.txt, so `import pytest`
would have failed and taken every test in the file with it. nose2 supports the
yield form natively, so there was nothing broken to fix.

Restores the original test_faces_to_edges and rewrites the new
test_face_normals_2d_raises to use try/except, matching the surrounding style,
so the file has no pytest dependency.

Verified with `nose2 -v -s python/tests`, as CI does: 13 tests, all pass.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@zfergus
zfergus merged commit 57344ee into main Jul 30, 2026
18 checks passed
@zfergus
zfergus deleted the feature/intersection-coordinates branch July 30, 2026 05:06
@zfergus zfergus added this to the v1.6.1 milestone Aug 4, 2026
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.

2 participants