Skip to content

Fix clip to padded face#106

Merged
brawer merged 2 commits into
yjh0502:masterfrom
HLennart:fix-clip-to-padded-face
Jul 16, 2026
Merged

Fix clip to padded face#106
brawer merged 2 commits into
yjh0502:masterfrom
HLennart:fix-clip-to-padded-face

Conversation

@HLennart

Copy link
Copy Markdown
Contributor

This PR fixes two maths bugs in clip_to_padded_face that caused it to return incorrect values.

  • b_tan was calculated with an inverted cross-product order norm_uvw.cross(&b_uvw). It is now computed as b_uvw.cross(&norm_uvw)
  • the second clip_destination call passed a flipped normal scaled_n * -1_f64 which inverted the collision logic for the second endpoint. it passes scaled_n now.

Reproducing the Bug
The PR includes a test case covering two edge cases:

  • A valid edge crossing the face
  • An edge completely outside the face bounds (expected to be rejected by clip_destination)

Adding some print statements to that test case (see at the end) i get

on master:

case 1: Valid crossing edge
EXPECTED: Intersects: true, A_UV: (1.0, 0.0), B_UV: (-1.0, 0.0)
ACTUAL: Intersects: false, A_UV: (2.0, 0.0), B_UV: (-2.0, 0.0)

case 2: Edge completely outside the face
EXPECTED: Intersects: false
ACTUAL: Intersects: true, A_UV: (1.0, 0.0), B_UV: (1.0, 0.0)

after the fix:

case 1: Valid crossing edge
EXPECTED: Intersects: true, A_UV: (1.0, 0.0), B_UV: (-1.0, 0.0)
ACTUAL: Intersects: true, A_UV: (1.0, 0.0), B_UV: (-1.0, -0.0)

case 2: Edge completely outside the face
EXPECTED: Intersects: false
ACTUAL: Intersects: false, A_UV: (5.0, 0.0), B_UV: (4.0, 0.0)

feel free to check out the first commit here to reproduce the failing test, which is fixed by the followup (second) commit. The code mostly follows the C++ implementation again, like returning somewhat nonsense A and B points on no intersection.

the modified test:

        #[test]
        fn test_clip_to_padded_face_edge_cases() {
            let face: u8 = 0; // +X face (U = Y, V = Z)
            let padding = 0.0;

            // Case 1: Edge crosses the face boundary (U=2.0 to U=-2.0)
            let a = Point::from_coords(1.0, 2.0, 0.0).normalize();
            let b = Point::from_coords(1.0, -2.0, 0.0).normalize();
            let (a_uv, b_uv, intersects) = super::clip_to_padded_face(a, b, face, padding);

            println!("case 1: Valid crossing edge");
            println!("EXPECTED: Intersects: true, A_UV: (1.0, 0.0), B_UV: (-1.0, 0.0)");
            println!(
                "ACTUAL: Intersects: {}, A_UV: ({:.1}, {:.1}), B_UV: ({:.1}, {:.1})\n",
                intersects, a_uv.x, a_uv.y, b_uv.x, b_uv.y
            );

            // Case 2: Edge completely outside the face bounds (U=5.0 to U=4.0)
            let a_out = Point::from_coords(1.0, 5.0, 0.0).normalize();
            let b_out = Point::from_coords(1.0, 4.0, 0.0).normalize();
            let (a_out_uv, b_out_uv, intersects_out) =
                super::clip_to_padded_face(a_out, b_out, face, padding);

            println!("case 2: Edge completely outside the face");
            println!("EXPECTED: Intersects: false");
            println!(
                "ACTUAL: Intersects: {}, A_UV: ({:.1}, {:.1}), B_UV: ({:.1}, {:.1})\n",
                intersects_out, a_out_uv.x, a_out_uv.y, b_out_uv.x, b_out_uv.y
            );

            assert_eq!(intersects, true, "Case 1: Should intersect");
            assert!((a_uv.x - 1.0).abs() < 1e-9, "Case 1: A_UV x should be 1.0");
            assert!(
                (b_uv.x - -1.0).abs() < 1e-9,
                "Case 1: B_UV x should be -1.0"
            );

            assert_eq!(
                intersects_out, false,
                "Case 2: Should correctly reject out-of-bounds edge"
            );
        }

@HLennart HLennart force-pushed the fix-clip-to-padded-face branch from 12bf977 to ce9598e Compare July 16, 2026 09:56
@brawer brawer added the bug label Jul 16, 2026
@brawer brawer merged commit ad74aa3 into yjh0502:master Jul 16, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants