Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Exception for the intersectionWith method #64

Open
wants to merge 18 commits into
base: develop
Choose a base branch
from

Conversation

Richiedu38
Copy link
Contributor

To avoid error messages during compilation due to the absence of the intersectionWith method for some shapes, I added an exception returning 'Intersection line capsule isn't supported'.

@Richiedu38 Richiedu38 force-pushed the feature/shapes-intersection branch from 66a1ca1 to 05face5 Compare May 10, 2024 17:01
Copy link
Member

@SylvainBertrand SylvainBertrand left a comment

Choose a reason for hiding this comment

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

I only commented on the polytope stuff


}

return false;
Copy link
Member

Choose a reason for hiding this comment

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

To be consistent with the other intersection algos, let's call intersectionToPack.setToNaN() when it's not intersecting.

Point3DBasics secondIntersectionToPack)

{

Copy link
Member

Choose a reason for hiding this comment

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

let's add here:

if (firstIntersectionToPack == null) firstIntersectionToPack = new Point3D();
if (secondIntersectionToPack == null) secondIntersectionToPack = new Point3D();


//For each face, we check the possibility of intersection with the line
for (int i=0; i< getNumberOfFaces(); i++) {
//If there isn't already a intersection found
Copy link
Member

Choose a reason for hiding this comment

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

The conditions in this loop aren't working, also you've got a bug due to a bad copy-paste. I'll let you figure out how to fix this loop.

break;
}

return numberOfIntersections;
Copy link
Member

Choose a reason for hiding this comment

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

Before returning, we need to check the ordering of the 2 intersections to be consistent with the other intersection algorithms

if (numberOfIntersections == 1)
return numberOfIntersections;

if (numberOfIntersections == 2 && EuclidCoreTools.epsilonEquals(firstIntersectionToPack, secondIntersectionToPack, 1.0e-7))
Copy link
Member

Choose a reason for hiding this comment

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

Here we already know that numberOfIntersections == 2, there is no need to test for it in that if condition.
Also, the secondIntersectionToPack should be set to NaN

// Swap intersections
Point3DBasics temp = firstIntersectionToPack;
firstIntersectionToPack = secondIntersectionToPack;
secondIntersectionToPack = temp;
Copy link
Member

Choose a reason for hiding this comment

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

This won't work, you're changing method's internal references, from the caller's perspective.
Your test should fail on this, if not, you want to update it to test the ordering the intersections.
The only solution here is to update the content of the 2 objects, so:

double tempX = firstInstersectionToPack.getX();
double tempY = firstInstersectionToPack.getY();
double tempZ = firstInstersectionToPack.getZ();
firstInstersectionToPack.set(secondInstersectionToPack);
secondIntersectionToPack.set(tempX, tempY, tempZ);

assertEquals(2, convexPolytope3D.intersectionWith(pointOnFace, direction, null, actualSecondInstersection));
actualFirstInstersection.setToZero();
actualSecondInstersection.setToZero();
assertEquals(2, convexPolytope3D.intersectionWith(pointOnFace, direction, actualFirstInstersection, actualSecondInstersection));
Copy link
Member

Choose a reason for hiding this comment

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

Ok so your test here only asserts that the method returns the expected number of intersections, it doesn't assert that the computed intersections are correct nor in the right order.

double y = alpha * A.getY() + beta * B.getY() + gamma * C.getY();
double z = alpha * A.getZ() + beta * B.getZ() + gamma * C.getZ();

Point3D pointOnFace = new Point3D(x, y, z);
Copy link
Member

Choose a reason for hiding this comment

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

I'd suggest wrapping the above code for generating a random point in a face into a static method here.
Eclipse can help you do that, select the code, then right click, refactor > extract method

assertEquals(0, convexPolytope3D.intersectionWith(pointOutside, lineDirection, null, actualSecondInstersection));
actualFirstInstersection.setToZero();
actualSecondInstersection.setToZero();
assertEquals(0, convexPolytope3D.intersectionWith(pointOutside, lineDirection, actualFirstInstersection, actualSecondInstersection));
Copy link
Member

Choose a reason for hiding this comment

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

I think we can find more constructions to cover more scenarios with no intersection

assertEquals(2, convexPolytope3D.intersectionWith(pointInside, lineDirection, null, actualSecondInstersection));
actualFirstInstersection.setToZero();
actualSecondInstersection.setToZero();
assertEquals(2, convexPolytope3D.intersectionWith(pointInside, lineDirection, actualFirstInstersection, actualSecondInstersection));
Copy link
Member

Choose a reason for hiding this comment

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

Here you've got the intersections that are known, you should easily be able to check the ones that are computed

assertEquals(2, convexPolytope3D.intersectionWith(pointInside, lineDirection, actualFirstInstersection, actualSecondInstersection));

}

Copy link
Member

Choose a reason for hiding this comment

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

Missing test for 1 intersection: through a vertex and through an edge.
Missing annoying edge-case: the line is in-plane of a face, there should be 2 intersections still

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