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

[Geometry] Update method intersectionWithEdge signature and redirect all methods to it in EdgeSetGeometryAlgorithms #4194

Merged
merged 11 commits into from
Apr 12, 2024

Conversation

epernod
Copy link
Contributor

@epernod epernod commented Sep 22, 2023

  • [Geometry] Update method signature of intersectionWithEdge to pass barycentric coordinates instead of 3D coord.
  • [Topology.Container] Update all method compute2EdgesIntersection to use the generic method in Edge class

By submitting this pull request, I acknowledge that
I have read, understand, and agree SOFA Developer Certificate of Origin (DCO).


Reviewers will merge this pull-request only if

  • it builds with SUCCESS for all platforms on the CI.
  • it does not generate new warnings.
  • it does not generate new unit test failures.
  • it does not generate new scene test failures.
  • it does not break API compatibility.
  • it is more than 1 week old (or has fast-merge label).

@epernod epernod added pr: fix Fix a bug pr: status to review To notify reviewers to review this pull-request labels Sep 22, 2023
@epernod
Copy link
Contributor Author

epernod commented Sep 22, 2023

[ci-build][with-all-tests]

Copy link
Contributor

@fredroy fredroy left a comment

Choose a reason for hiding this comment

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

getCPos() returns a const reference
and i did not see the need to cast ? 🧐

@hugtalbot hugtalbot added pr: status wip Development in the pull-request is still in progress and removed pr: status to review To notify reviewers to review this pull-request labels Oct 4, 2023
@bakpaul bakpaul added pr: status to review To notify reviewers to review this pull-request and removed pr: status to review To notify reviewers to review this pull-request labels Oct 4, 2023
@epernod epernod added pr: status to review To notify reviewers to review this pull-request and removed pr: status wip Development in the pull-request is still in progress labels Oct 4, 2023
@bakpaul bakpaul added pr: status wip Development in the pull-request is still in progress and removed pr: status to review To notify reviewers to review this pull-request labels Oct 11, 2023
@fredroy fredroy added pr: status to review To notify reviewers to review this pull-request and removed pr: status wip Development in the pull-request is still in progress labels Oct 16, 2023
@fredroy
Copy link
Contributor

fredroy commented Oct 16, 2023

[ci-build][with-all-tests]

@fredroy
Copy link
Contributor

fredroy commented Oct 16, 2023

A unit test is failing, in Sofa.Component.Topology/InciseProcessor_test.InciseTriangles
from TriangleSetGeometryAlgorithms: Orthogonal projection failed

@hugtalbot
Copy link
Contributor

Still a unit test failing @epernod

@hugtalbot hugtalbot added pr: status wip Development in the pull-request is still in progress and removed pr: status to review To notify reviewers to review this pull-request labels Oct 18, 2023
@epernod epernod added pr: status to review To notify reviewers to review this pull-request and removed pr: status wip Development in the pull-request is still in progress labels Dec 28, 2023
@epernod
Copy link
Contributor Author

epernod commented Dec 28, 2023

[ci-build][with-all-tests]

@hugtalbot
Copy link
Contributor

the rebase did not suffice @epernod
we let it to review but a unit test is still failing

@epernod epernod force-pushed the inf_2023_19_up-intersectEdges branch from b6f0ffd to 3317bb9 Compare February 9, 2024 14:40
@epernod
Copy link
Contributor Author

epernod commented Feb 11, 2024

Last regression test failing will disappear as soon as PR #4494 is merged as the new version of isquadDelaunay method is not using this method intersectionWithEdge anymore.
And for the note, the error is a false positive. The regression should be regenerated at one point.

@bakpaul bakpaul added this to the v24.06 milestone Feb 20, 2024
@fredroy fredroy force-pushed the inf_2023_19_up-intersectEdges branch from 3317bb9 to 33fb6d5 Compare February 22, 2024 00:55
@fredroy
Copy link
Contributor

fredroy commented Feb 22, 2024

Last regression test failing will disappear as soon as PR #4494 is merged as the new version of isquadDelaunay method is not using this method intersectionWithEdge anymore. And for the note, the error is a false positive. The regression should be regenerated at one point.

Regressions are OK but the error being an unit test InciseProcessor_test.InciseTriangles, it is because of [ERROR] [TriangleSetGeometryAlgorithms(GeomAlgo)] Orthogonal projection failed
I dont really see how it can be a false positive ?

@epernod
Copy link
Contributor Author

epernod commented Feb 27, 2024

can you point the test that is not wworking, it is impossible to find it in jenkins menu, even more with a low bandwidth

@fredroy
Copy link
Contributor

fredroy commented Feb 28, 2024

can you point the test that is not wworking, it is impossible to find it in jenkins menu, even more with a low bandwidth

Regressions are OK but the error being an unit test InciseProcessor_test.InciseTriangles, it is because of [ERROR] [TriangleSetGeometryAlgorithms(GeomAlgo)] Orthogonal projection failed I dont really see how it can be a false positive ?

InciseProcessor_test.InciseTriangles 😅 ?

TEST_F(InciseProcessor_test, InciseTriangles)

But for sure the CI jenkins is slow as hell

@hugtalbot hugtalbot added pr: status wip Development in the pull-request is still in progress and removed pr: status to review To notify reviewers to review this pull-request labels Feb 28, 2024
@epernod
Copy link
Contributor Author

epernod commented Apr 9, 2024

Last regression test failing will disappear as soon as PR #4494 is merged as the new version of isquadDelaunay method is not using this method intersectionWithEdge anymore. And for the note, the error is a false positive. The regression should be regenerated at one point.

Regressions are OK but the error being an unit test InciseProcessor_test.InciseTriangles, it is because of [ERROR] [TriangleSetGeometryAlgorithms(GeomAlgo)] Orthogonal projection failed I dont really see how it can be a false positive ?

Hi,
I meant, false positive because it is part of the snapping algorithm in the incision which was already buggy but the error was not catch before.
The projection was computed totally outside of the edge (the mesh is a square of size 10x10:
coord_edge1: [5.44854, 5.02682, -0.832787] - [4.86141, 4.79438, -0.760481]
coord_edge2: [4.99791, 4.99949, -0.778808] - [20676.1, 52846.7, 1933.72]

the result was a strange (but not crashing 🤔 ) incision
image

In fact the correct fix is to exit the snapping if the projection failed and continue the incision normally instead of forcing the projection and use a "random" point of the triangle to be snapped.

Here is the result after:
IncisionTrianglesProcess_00000002

@epernod epernod force-pushed the inf_2023_19_up-intersectEdges branch from 4b558fb to 9146aa4 Compare April 9, 2024 10:02
@epernod epernod added pr: status to review To notify reviewers to review this pull-request and removed pr: status wip Development in the pull-request is still in progress labels Apr 9, 2024
@epernod
Copy link
Contributor Author

epernod commented Apr 9, 2024

[ci-build][with-all-tests]

…opology/container/dynamic/EdgeSetGeometryAlgorithms.h

Co-authored-by: Paul Baksic <30337881+bakpaul@users.noreply.github.com>
@bakpaul bakpaul added pr: status ready Approved a pull-request, ready to be squashed and removed pr: status to review To notify reviewers to review this pull-request labels Apr 12, 2024
@bakpaul bakpaul merged commit df6198a into sofa-framework:master Apr 12, 2024
14 of 15 checks passed
@epernod epernod deleted the inf_2023_19_up-intersectEdges branch April 12, 2024 13:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr: fix Fix a bug pr: status ready Approved a pull-request, ready to be squashed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants