[Topology] Check indices out-of-bound in TriangleSetTopologyContainer #4242

olivier-goury · 2023-10-18T08:23:45Z

Check that triangles index match the size of the triangleAroundVertexArray (Like it is done in EdgeSetTopologyContainer for EdgeAroundVertexArray)

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).

github-actions · 2023-10-18T08:23:57Z

⚠️ ⚠️ ⚠️
@sofa-framework your PR title is not following the required format ✏️
- PR title should provide a prefix scope, e.g. [MechanicalLoad]
⚠️ ⚠️ ⚠️

github-actions · 2023-10-18T08:24:08Z

⚠️ ⚠️ ⚠️
@sofa-framework your PR does not include any status label 🏷️
Make sure to add one (wip, to review or ready).
⚠️ ⚠️ ⚠️

github-actions · 2023-10-18T08:24:08Z

⚠️ ⚠️ ⚠️
@sofa-framework your PR does not include any status label 🏷️
Make sure to add one (wip, to review or ready).
⚠️ ⚠️ ⚠️

hugtalbot · 2023-10-18T08:42:31Z

...ainer/Dynamic/src/sofa/component/topology/container/dynamic/TriangleSetTopologyContainer.cpp

+        {
+            msg_warning() << "trianglesAroundVertex creation failed, Triangle buffer is not concistent with number of points: Triangle: " << m_triangle[i] << " for: " << getNbPoints() << " points.";
+            continue;
+        }


should the componentState be invalid then?

should it not break the loop ?

It could yes! I just copy-pasted (included the typo on consistent!) what was done from EdgeSetTopologyContainer where there is exactly the same loop but for edges.

what? My télor is riche! 😆

...ainer/Dynamic/src/sofa/component/topology/container/dynamic/TriangleSetTopologyContainer.cpp

epernod

Ok for me, but could you explain in which case you have this problem?
Setting the component to invalid is a good idea. Breaking the loop might create some bigger issue in components using the topology as it won't be set.

olivier-goury · 2023-10-19T14:54:05Z

Ok for me, but could you explain in which case you have this problem? Setting the component to invalid is a good idea. Breaking the loop might create some bigger issue in components using the topology as it won't be set.

We have this problem when using subtopologies defined using BoxROIs. The subTopo has fewer points than the original topology (Since it is a subset of it) but the triangles in that subtopo refer to nodes indexed with the numbering in the original topology that are out of bounds within the subtopo world. Not sure that is clear!

olivier-goury · 2023-10-19T15:10:33Z

But for the component state set to invalid, I would actually leave it valid.
Indeed, it is just that the m_trianglesAroundVertex array won't be fully created, but the rest of the component remains valid and for example a forcefield can be defined on this container.

epernod · 2023-10-20T07:43:34Z

Ok for me, but could you explain in which case you have this problem? Setting the component to invalid is a good idea. Breaking the loop might create some bigger issue in components using the topology as it won't be set.

We have this problem when using subtopologies defined using BoxROIs. The subTopo has fewer points than the original topology (Since it is a subset of it) but the triangles in that subtopo refer to nodes indexed with the numbering in the original topology that are out of bounds within the subtopo world. Not sure that is clear!

There is something un-catholic there... So if I understand well, you have triangle in your topology that refer to point out of bound to your current node mechanicalObject?
I don't understand how it can work. It will crash as soon as a component will try to access a dof passing through the topology no?

hugtalbot · 2023-10-25T07:52:44Z

Small up regarding Erik's question @olivier-goury

your answer will help us deciding whether we pass the component as invalid or not in the loop

hugtalbot · 2023-10-25T08:00:28Z

@olivier-goury could you try using a SubSetTopologyMapping to ignore out of the box triangles?
See the SubSetTopologyMapping scene example

olivier-goury · 2023-11-21T14:05:26Z

Ok for me, but could you explain in which case you have this problem? Setting the component to invalid is a good idea. Breaking the loop might create some bigger issue in components using the topology as it won't be set.

We have this problem when using subtopologies defined using BoxROIs. The subTopo has fewer points than the original topology (Since it is a subset of it) but the triangles in that subtopo refer to nodes indexed with the numbering in the original topology that are out of bounds within the subtopo world. Not sure that is clear!

There is something un-catholic there... So if I understand well, you have triangle in your topology that refer to point out of bound to your current node mechanicalObject? I don't understand how it can work. It will crash as soon as a component will try to access a dof passing through the topology no?

Sorry for the late reply!
So it is a case where the node where the subtopology is defined does not contain a MechanicalObject. It is only defined to add a ForceField applying onto a subpart of the node of the MechanicalObject. An example is in this scene: https://github.com/SofaDefrost/ModelOrderReduction/blob/master/tools/test/sofa_test_scene/quadruped.py

alxbilger · 2023-12-11T07:40:22Z

I switched this PR back to "to review". It was in WIP, that's why the lack of activity. @epernod can you take a look on @olivier-goury 's answer?

epernod · 2023-12-11T22:24:11Z

ok for me if it fix your case and it had a warning if we encounter the wrong behavior.

epernod · 2023-12-11T22:25:02Z

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

…Array (Like it is done in EdgeSetTopologyContainer for EdgeAroundVertexArray)

…#4242) * Check that triangles index match the size of the triangleAroundVertexArray (Like it is done in EdgeSetTopologyContainer for EdgeAroundVertexArray) * Fix some typos

olivier-goury added the pr: fix Fix a bug label Oct 18, 2023

olivier-goury requested review from alxbilger and epernod October 18, 2023 08:23

alxbilger added the pr: status to review To notify reviewers to review this pull-request label Oct 18, 2023

olivier-goury changed the title ~~Fix TriangleSetTopologyContainer~~ [Topology] Fix TriangleSetTopologyContainer Oct 18, 2023

hugtalbot changed the title ~~[Topology] Fix TriangleSetTopologyContainer~~ [Topology] Fix indices out-of-bound in TriangleSetTopologyContainer Oct 18, 2023

hugtalbot changed the title ~~[Topology] Fix indices out-of-bound in TriangleSetTopologyContainer~~ [Topology] Check indices out-of-bound in TriangleSetTopologyContainer Oct 18, 2023

hugtalbot reviewed Oct 18, 2023

View reviewed changes

fredroy requested changes Oct 18, 2023

View reviewed changes

...ainer/Dynamic/src/sofa/component/topology/container/dynamic/TriangleSetTopologyContainer.cpp Outdated Show resolved Hide resolved

fredroy approved these changes Oct 18, 2023

View reviewed changes

epernod reviewed Oct 19, 2023

View reviewed changes

epernod 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 Nov 3, 2023

alxbilger 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 11, 2023

hugtalbot added the pr: backport todo This PR will be backported into the release preceeding its milestone. label Dec 11, 2023

hugtalbot added this to the v23.12 milestone Dec 11, 2023

epernod approved these changes Dec 11, 2023

View reviewed changes

epernod removed the pr: status to review To notify reviewers to review this pull-request label Dec 11, 2023

epernod added the pr: status ready Approved a pull-request, ready to be squashed label Dec 11, 2023

olivier-goury added 2 commits December 12, 2023 09:02

Check that triangles index match the size of the triangleAroundVertex…

3416dfd

…Array (Like it is done in EdgeSetTopologyContainer for EdgeAroundVertexArray)

Fix some typos

575c4c7

alxbilger force-pushed the fix_TriangleSetTopologyContainer branch from 390f842 to 575c4c7 Compare December 12, 2023 08:02

epernod merged commit 3249be8 into master Dec 12, 2023
10 checks passed

epernod deleted the fix_TriangleSetTopologyContainer branch December 12, 2023 09:15

bakpaul added pr: backport done This PR has been backported into the release before its milestone. and removed pr: backport todo This PR will be backported into the release preceeding its milestone. labels Feb 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Topology] Check indices out-of-bound in TriangleSetTopologyContainer #4242

[Topology] Check indices out-of-bound in TriangleSetTopologyContainer #4242

olivier-goury commented Oct 18, 2023

github-actions bot commented Oct 18, 2023

github-actions bot commented Oct 18, 2023

github-actions bot commented Oct 18, 2023

hugtalbot Oct 18, 2023

hugtalbot Oct 18, 2023

olivier-goury Oct 18, 2023

hugtalbot Oct 23, 2023

epernod Oct 23, 2023

epernod left a comment

olivier-goury commented Oct 19, 2023

olivier-goury commented Oct 19, 2023

epernod commented Oct 20, 2023

hugtalbot commented Oct 25, 2023 •

edited

Loading

hugtalbot commented Oct 25, 2023

olivier-goury commented Nov 21, 2023

alxbilger commented Dec 11, 2023

epernod commented Dec 11, 2023

epernod commented Dec 11, 2023

[Topology] Check indices out-of-bound in TriangleSetTopologyContainer #4242

[Topology] Check indices out-of-bound in TriangleSetTopologyContainer #4242

Conversation

olivier-goury commented Oct 18, 2023

github-actions bot commented Oct 18, 2023

github-actions bot commented Oct 18, 2023

github-actions bot commented Oct 18, 2023

hugtalbot Oct 18, 2023

Choose a reason for hiding this comment

hugtalbot Oct 18, 2023

Choose a reason for hiding this comment

olivier-goury Oct 18, 2023

Choose a reason for hiding this comment

hugtalbot Oct 23, 2023

Choose a reason for hiding this comment

epernod Oct 23, 2023

Choose a reason for hiding this comment

epernod left a comment

Choose a reason for hiding this comment

olivier-goury commented Oct 19, 2023

olivier-goury commented Oct 19, 2023

epernod commented Oct 20, 2023

hugtalbot commented Oct 25, 2023 • edited Loading

hugtalbot commented Oct 25, 2023

olivier-goury commented Nov 21, 2023

alxbilger commented Dec 11, 2023

epernod commented Dec 11, 2023

epernod commented Dec 11, 2023

hugtalbot commented Oct 25, 2023 •

edited

Loading