Align libigl barycentric coordinates to compas. #39

petrasvestartas · 2025-09-02T08:41:38Z

Fix to the bug described here: #34

yiqiaowang-arch · 2025-09-05T21:36:00Z

Hi @petrasvestartas , thank you very much for the work!

I found several problems here:

the output of intersection_ray_mesh still doesn't match that of compas.geometry.barycentric_coordinates (see Clearer documentation for intersection_ray_mesh #34 (comment));
the typehint in the docstring is incorrect (also see Clearer documentation for intersection_ray_mesh #34 (comment));
(maybe not directly related to this repo but) the compas.geometry.intersection_ray_mesh's docstring labels the second and third outputs as u, which is also incorrect (see https://github.com/compas-dev/compas/blob/dc6f2deee7a8eb504be20798e281fc0318ba37da/src/compas/geometry/intersections.py#L746).

tomvanmele · 2025-12-10T15:57:47Z

@petrasvestartas @yiqiaowang-arch what is the status here?

yiqiaowang-arch · 2025-12-10T17:08:41Z

@petrasvestartas @yiqiaowang-arch what is the status here?

Hi, as of now I am still using the old definition of intersection_ray_mesh. The problems (docstring and output alignment) don't seem to be resolved yet.

Copilot

Pull request overview

This PR addresses bug #34 by aligning libigl's barycentric coordinate system with COMPAS conventions. The changes introduce conversion logic to transform libigl's barycentric coordinates (1-u-v)*v0 + u*v1 + v*v2 to COMPAS's format, and modify the intersection functions to return points along with barycentric coordinates instead of just distance values.

Key Changes:

Added barycentric coordinate conversion from libigl to COMPAS format
Modified intersection functions to return (point, face_index, u, v, w) instead of (face_index, u, v, distance)
Added helper function barycenter_to_point for point reconstruction from barycentric coordinates

Reviewed changes

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

Show a summary per file

File	Description
src/compas_libigl/intersections.py	Core changes: added `_conversion_libigl_to_compas` and `barycenter_to_point` functions, updated `intersection_ray_mesh` and `intersection_rays_mesh` to return points and COMPAS-aligned barycentric coordinates
docs/examples/example_intersections_barycentric.py	New example demonstrating barycentric coordinate usage and comparison with COMPAS
docs/examples/example_intersections.py	Updated to use new API returning points directly
docs/examples/example_mapping_patterns.py	Code formatting improvements (spacing around operators)
docs/examples/example_mapping.py	Code formatting improvements (spacing around operators, trailing whitespace)
pyproject.toml	Formatting-only change (newline at end of file)
CHANGELOG.md	Documents the bug fix and API change

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

Copilot · 2025-12-11T08:07:27Z

src/compas_libigl/intersections.py

+    u : float
+        The u coordinate (weight for p2)
+    v : float
+        The v coordinate (weight for p3)
+    w : float
+        The w coordinate (weight for p1)


The variable names in the parameter documentation are inconsistent. The parameters are named u, v, w but the documentation describes them with different semantics. According to the formula P = w*p1 + u*p2 + v*p3, the parameter names should better reflect the vertex they weight (e.g., u_p2, v_p3, w_p1) or the documentation should clarify that despite the generic names, u weights p2, v weights p3, and w weights p1.

Copilot · 2025-12-11T08:07:27Z

src/compas_libigl/intersections.py

+        # To match COMPAS barycentric coordinates exactly:
+        # COMPAS expects coordinates in order [p1_weight, p2_weight, p3_weight]
+        # Our formula is P = w*p1 + u*p2 + v*p3, so COMPAS order should be [w, u, v]
+        hits_compas.append([point, idx, u, v, w])


The barycentric coordinate ordering is incorrect. The conversion computes the correct coordinate values (w=1-u_libigl-v_libigl for p1, u=u_libigl for p2, v=v_libigl for p3), and the formula P = w*p1 + u*p2 + v*p3 is correct. However, the returned tuple [point, idx, u, v, w] returns coordinates in order (u, v, w) which corresponds to weights for (p2, p3, p1).

According to the documentation claiming compatibility with compas.geometry.barycentric_coordinates, the order should be (p1_weight, p2_weight, p3_weight), which is (w, u, v). The return statement should be hits_compas.append([point, idx, w, u, v]) to match COMPAS convention.

Suggested change

hits_compas.append([point, idx, u, v, w])

hits_compas.append([point, idx, w, u, v])

Copilot · 2025-12-11T08:07:28Z

docs/examples/example_intersections_barycentric.py

+
+intersections = []
+for hit in hits_per_ray:
+    point, idx, w, u, v = hit


There's a discrepancy in how the barycentric coordinates are unpacked. Line 21 unpacks as point, idx, u, v, w, but line 25 unpacks the same hit as point, idx, w, u, v. This inconsistency suggests confusion about the coordinate ordering and will lead to incorrect results. The unpacking order should be consistent throughout the example.

Suggested change

point, idx, w, u, v = hit

point, idx, u, v, w = hit

Copilot · 2025-12-11T08:07:28Z

src/compas_libigl/intersections.py

+    for h in hits_per_ray:
+        idx, u_libigl, v_libigl, _ = h
+        w = 1.0 - u_libigl - v_libigl  # libigl's (1-u-v) coefficient
+        u = u_libigl  # libigl's u coefficient  


Trailing whitespace on this line. This should be removed for code cleanliness.

Suggested change

u = u_libigl # libigl's u coefficient

u = u_libigl # libigl's u coefficient

Copilot · 2025-12-11T08:07:28Z

src/compas_libigl/intersections.py

+        face = faces[idx]
+        p1, p2, p3 = vertices[face[0]], vertices[face[1]], vertices[face[2]]
+        point = barycenter_to_point(u, v, w, p1, p2, p3)
+


Trailing whitespace on this line. This should be removed for code cleanliness.

Suggested change

Copilot · 2025-12-11T08:07:29Z

src/compas_libigl/intersections.py

+    hits_per_ray = _intersections.intersection_ray_mesh(P, D, V, F)
+
+    # Convert libigl barycentric coordinates to COMPAS convention
+    hits_compas = _conversion_libigl_to_compas(hits_per_ray, M)
+
+    return hits_compas


This is a breaking API change. The original API returned (face_index, u, v, distance), but now returns [point, face_index, u, v, w]. While this change appears intentional to fix the bug, existing code that depends on this API will break. Consider:

Documenting this as a breaking change more prominently in the CHANGELOG

Updating all existing tests and examples to use the new API

Considering a deprecation period or version bump to signal the breaking change to users

Copilot · 2025-12-11T08:07:29Z

src/compas_libigl/intersections.py

+    return hits_compas
+
+
+def barycenter_to_point(u, v, w, p1, p2, p3):


The barycenter_to_point function is being imported in example files (line 4 and line 10 of the example files), but it's not clear if this function is intended to be part of the public API. If it is public, it should be documented as such (e.g., via __all__). If it's meant to be private/internal, it should be prefixed with an underscore (e.g., _barycenter_to_point) and the example imports should be reconsidered.

Copilot · 2025-12-11T08:07:29Z

src/compas_libigl/intersections.py

+def _conversion_libigl_to_compas(hits_per_ray, M):
+    """Convert libigl barycentric coordinates to COMPAS barycentric coordinates.
+
+    Parameters
+    ----------
+    hits_per_ray : list[tuple[int, float, float, float]]
+        Tuples of (face_index, u, v, distance) from libigl ray intersection
+    M : tuple[list[list[float]], list[list[int]]]
+        A mesh represented by a tuple of (vertices, faces)
+        where vertices are 3D points and faces are triangles
+
+    Returns
+    -------
+    list[tuple[list[float], int, float, float, float]]
+        Tuples of (point, face_index, u, v, w) in COMPAS barycentric coordinate ordering
+
+    Note
+    ----
+    libigl uses: P = (1-u-v)*v0 + u*v1 + v*v2
+    COMPAS uses: P = u*v0 + v*v1 + w*v2 where u + v + w = 1
+    This function converts libigl coordinates to match COMPAS barycentric coordinate ordering
+    """
+    vertices = M[0]
+    faces = M[1]
+
+    hits_compas = []
+    for h in hits_per_ray:
+        idx, u_libigl, v_libigl, _ = h
+        w = 1.0 - u_libigl - v_libigl  # libigl's (1-u-v) coefficient
+        u = u_libigl  # libigl's u coefficient  
+        v = v_libigl  # libigl's v coefficient
+
+        face = faces[idx]
+        p1, p2, p3 = vertices[face[0]], vertices[face[1]], vertices[face[2]]
+        point = barycenter_to_point(u, v, w, p1, p2, p3)
+
+        # To match COMPAS barycentric coordinates exactly:
+        # COMPAS expects coordinates in order [p1_weight, p2_weight, p3_weight]
+        # Our formula is P = w*p1 + u*p2 + v*p3, so COMPAS order should be [w, u, v]
+        hits_compas.append([point, idx, u, v, w])
+    return hits_compas


The new barycentric coordinate conversion logic lacks test coverage. Given that this PR fixes a coordinate system bug, it's critical to add tests that verify:

The barycentric coordinates returned match those from compas.geometry.barycentric_coordinates for the same point and triangle

The point reconstruction using the returned barycentric coordinates produces the correct intersection point

The coordinate ordering is correct (weights should be in order p1, p2, p3 to match COMPAS convention)

These tests would prevent regression of the bug this PR aims to fix.

Copilot · 2025-12-11T08:07:29Z

src/compas_libigl/intersections.py

+        - The intersection point P = u*p1 + v*p2 + w*p3 where u + v + w = 1
+        - These coordinates match those returned by compas.geometry.barycentric_coordinates


The documentation formula is inconsistent with the actual implementation. Line 115 states P = u*p1 + v*p2 + w*p3, but based on the coordinate order being returned [point, idx, u, v, w] where (u, v, w) are supposed to be weights for (p2, p3, p1) based on the conversion formula P = w*p1 + u*p2 + v*p3.

If the return order is corrected to [point, idx, w, u, v] to match COMPAS convention (weights in order p1, p2, p3), then this documentation formula should be updated to P = w*p1 + u*p2 + v*p3 where the first coordinate is w, second is u, and third is v.

Suggested change

- The intersection point P = u*p1 + v*p2 + w*p3 where u + v + w = 1

- These coordinates match those returned by compas.geometry.barycentric_coordinates

- The intersection point P = w*p1 + u*p2 + v*p3 where w + u + v = 1

- The returned tuple is (point, face_index, w, u, v), matching COMPAS barycentric coordinate ordering.

Copilot · 2025-12-11T08:07:30Z

docs/examples/example_intersections.py

 from compas_viewer import Viewer

 from compas_libigl.intersections import intersection_rays_mesh
+from compas_libigl.intersections import barycenter_to_point


Import of 'barycenter_to_point' is not used.

Suggested change

from compas_libigl.intersections import barycenter_to_point

Fix align libigl barycentrioc coordinates to compas.

b8f7ee6

petrasvestartas requested a review from tomvanmele September 2, 2025 08:41

petrasvestartas changed the title ~~Fix align libigl barycentrioc coordinates to compas.~~ Fix align libigl barycentric coordinates to compas. Sep 2, 2025

petrasvestartas changed the title ~~Fix align libigl barycentric coordinates to compas.~~ Align libigl barycentric coordinates to compas. Sep 2, 2025

petrasvestartas mentioned this pull request Sep 2, 2025

Clearer documentation for intersection_ray_mesh #34

Open

pv and others added 4 commits September 5, 2025 18:19

FIX barycentric coordinates and output a point

de6033a

Fix example file

b0939c1

CLEANUP

2cfc188

Merge branch 'main' into ray_mesh

7187563

tomvanmele approved these changes Sep 5, 2025

View reviewed changes

tomvanmele requested a review from Copilot December 11, 2025 08:02

Copilot started reviewing on behalf of tomvanmele December 11, 2025 08:02 View session

Copilot AI reviewed Dec 11, 2025

View reviewed changes

	hits_compas.append([point, idx, u, v, w])
	hits_compas.append([point, idx, w, u, v])

	u = u_libigl # libigl's u coefficient
	u = u_libigl # libigl's u coefficient

		return hits_compas


		def barycenter_to_point(u, v, w, p1, p2, p3):

		- The intersection point P = up1 + vp2 + w*p3 where u + v + w = 1
		- These coordinates match those returned by compas.geometry.barycentric_coordinates

Align libigl barycentric coordinates to compas. #39

Are you sure you want to change the base?

Align libigl barycentric coordinates to compas. #39

Conversation

petrasvestartas commented Sep 2, 2025

Uh oh!

yiqiaowang-arch commented Sep 5, 2025

Uh oh!

tomvanmele commented Dec 10, 2025

Uh oh!

yiqiaowang-arch commented Dec 10, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Dec 11, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 11, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 11, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 11, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 11, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 11, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 11, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 11, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 11, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 11, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants