CG/CG_elem: rework point/plane intersection evaluation #427

ksamouchos · 2023-10-18T09:48:31Z

This pull request:

adds a new function for evaluating the point-line intersection
improves the test checking the point-segment intersection
micro-optimizes the function evaluating the point-segment intersection (there is one less square root).

This pull-request completes task IMM-728.

edoardolombardi · 2023-10-18T10:20:25Z

src/CG/CG_elem.cpp

-    array3D xP;
-    if ( intersectLinePlane(Q1, n, P2, n2, xP, distanceTolerance) && intersectPointSegment(xP, Q1, Q2, distanceTolerance) ) {
-        P = xP;
+    if ( intersectLinePlane(Q1, n, P2, n2, P, distanceTolerance) && intersectPointSegment(P, Q1, Q2, distanceTolerance) ) {


From documentation:

P intersection point if intersect, else unaltered

With this modification it will be altered even if there is no actual intersection between plane and segment, but only between line and plane, @ksamouchos isn't it?

Correct! I have to change the documentation as well. This change has only to do with personal preferences. So, if you don't agree, I can retrieve the initial behavior of the function.

This change doesn't seem related to the bug you are fixing. Please move it to another commit.

I would leave the function as it was. Let's not change the public API without a valid reason.

If the change is not really needed for some reason, for compatibility issues on possible applications using this function, personally, I'd keep the original behavior.

src/CG/CG_elem.cpp

test/integration_tests/CG/test_CG_00002.cpp

src/CG/CG_elem.cpp

andrea-iob

Before merging please change the commit message from "CG/CG_elem: ..." to just "CG: ...". You should be able to that directly from GitHub ("Squash and Merge").

edoardolombardi · 2023-10-19T07:56:14Z

src/CG/CG_elem.cpp

-    if(distancePointSegment(P, P1, P2) > distanceTolerance){
-        return false;
-    }
+    array3D xP     = projectPointSegment( P, P1, P2 );


What's the difference w.r.t. the old implementation?
Does not distancePointSegment function the same computation?

double distancePointSegment( array3D const &P, array3D const &Q0, array3D const &Q1) { array3D xP = projectPointSegment( P, Q0, Q1); return norm2(P-xP); }

In the old implementation, this point was considered intersecting the segment:

But you may be right, distancePointSegment seems the function that contains the bug.

I think @edoardolombardi is right, but I still see different (and more correct) results in immerflow with the new implementation. I have to investigate more to see what's going on.

To have an exact match between the two implementations, the updated one should check for "<=" rather than for just "<". However, I don't think this will make any difference for your test case.

I'm studying a case where a plane is almost intersecting a segment. The distance of the projection point P from the segment edge P2 (see above figure) is of order 10^-20. In that case, the order of operations plays a significant role. So, bitpit and an algorithm I've developed to compute intersections were giving me contradictive results. That's why I got this false alarm.

Since bitpit is not wrong, I suggest not to merge this branch and cancel the pull-request. I can continue my work by changing the tolerance used in bitpit to compare distances.

This pull request is still worthwile:

there is a new function (intersectPointLine);

the tests are improved;

the function intersectPointSegment is marginally faster (there is one less square root).

If you could split it in three commit, we can still merge it. Something like that:

CG: introduce a function to evaluate line-point intersections
(This first commit will also introduce the tests for that function)

CG: improve the tests that check point intersections

CG: micro optimize the function that evaluates line-segment intersections

(Please also replace the < with a <= to make the results as close as possible to the previous implementation.)

Thanks! I did what you asked.

ksamouchos added the bug label Oct 18, 2023

ksamouchos requested review from marcocisternino and andrea-iob October 18, 2023 09:48

ksamouchos self-assigned this Oct 18, 2023

edoardolombardi reviewed Oct 18, 2023

View reviewed changes

andrea-iob reviewed Oct 18, 2023

View reviewed changes

src/CG/CG_elem.cpp Outdated Show resolved Hide resolved

andrea-iob reviewed Oct 18, 2023

View reviewed changes

test/integration_tests/CG/test_CG_00002.cpp Show resolved Hide resolved

andrea-iob reviewed Oct 18, 2023

View reviewed changes

src/CG/CG_elem.cpp Outdated Show resolved Hide resolved

andrea-iob self-requested a review October 18, 2023 13:16

ksamouchos force-pushed the CG.fix.segment.plane.intersection branch 2 times, most recently from c862962 to bfe60cb Compare October 19, 2023 07:43

andrea-iob approved these changes Oct 19, 2023

View reviewed changes

edoardolombardi reviewed Oct 19, 2023

View reviewed changes

ksamouchos force-pushed the CG.fix.segment.plane.intersection branch from bfe60cb to 64b8a58 Compare October 19, 2023 07:59

Konstantinos added 3 commits October 19, 2023 15:59

CG: introduce function to evaluate point-line intersections

e579520

CG: improve tests that check point-segment intersections

2d5a74b

CG: micro-optimize function evaluating point-segment intersections

c050f42

ksamouchos force-pushed the CG.fix.segment.plane.intersection branch from 64b8a58 to c050f42 Compare October 19, 2023 14:11

ksamouchos changed the title ~~CG/CG_elem: correct point/plane intersection evaluation~~ CG/CG_elem: rework point/plane intersection evaluation Oct 19, 2023

ksamouchos added M - CG and removed bug labels Oct 19, 2023

andrea-iob self-requested a review October 19, 2023 15:59

andrea-iob approved these changes Oct 19, 2023

View reviewed changes

ksamouchos requested a review from edoardolombardi October 20, 2023 10:21

edoardolombardi approved these changes Oct 25, 2023

View reviewed changes

ksamouchos merged commit 0ba5545 into master Oct 25, 2023
1 check passed

ksamouchos deleted the CG.fix.segment.plane.intersection branch October 25, 2023 08:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CG/CG_elem: rework point/plane intersection evaluation #427

CG/CG_elem: rework point/plane intersection evaluation #427

ksamouchos commented Oct 18, 2023 •

edited

Loading

edoardolombardi Oct 18, 2023

ksamouchos Oct 18, 2023

andrea-iob Oct 18, 2023

andrea-iob Oct 18, 2023

edoardolombardi Oct 18, 2023

ksamouchos Oct 18, 2023

andrea-iob left a comment •

edited

Loading

edoardolombardi Oct 19, 2023

andrea-iob Oct 19, 2023

andrea-iob Oct 19, 2023

ksamouchos Oct 19, 2023

andrea-iob Oct 19, 2023

ksamouchos Oct 19, 2023

andrea-iob Oct 19, 2023

andrea-iob Oct 19, 2023

ksamouchos Oct 19, 2023

CG/CG_elem: rework point/plane intersection evaluation #427

CG/CG_elem: rework point/plane intersection evaluation #427

Conversation

ksamouchos commented Oct 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrea-iob left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ksamouchos commented Oct 18, 2023 •

edited

Loading

andrea-iob left a comment •

edited

Loading