Skip to content
This repository has been archived by the owner on Mar 25, 2024. It is now read-only.

Issue 200 fix intersection #220

Closed

Conversation

danielcu888
Copy link
Contributor

Fix for failing intersection issue #200 which generates an invalid Solid containing two disjoint parts when it should yield a single two part MultiSoild.

In summary, when recovering the vertices pertaining to each Polyhedra component of the desired intersection volume, we now process each 3-cell deemed relevant to the intersection result individually rather than collectively, since they may pertain to disjoint volumes. These volumes are now combined downstream as separate primitives, preserving the disjoint nature of the components of the result if they are indeed disjoint.

Also added a standalone test to demonstrate the fix.

…fier when building intersection polyhedra.

* When recovering the vertices pertaining to each Polyhedra
  component of the desired intersection volume, we now
  process each 3-cell deemed relevant to the intersection result
  individually rather than collectively, since they may pertain
  to disjoint volumes. These volumes are now combined downstream
  as separate primitives, preserving the disjoint nature of the
  components of the result if they are indeed disjoint. This
  scenario is that particularly relevant to Issue Oslandia#200 where the
  intersection result consists of two disjoint volumes, yielding
  a MultiSolid intersection result.
@mhugo
Copy link
Contributor

mhugo commented Apr 22, 2020

Many thanks for your proposition.

However, I am very hesitant to touch files in the "CGAL_patches" directory.
This directory is probably a mistake by itself: if I remember correctly it has been added as a quick and dirty solution to comply with CGAL >= 4.13, because we did not manage to maintain compatibility with newer versions of CGAL.

However I don't know what to do with a patch to the patch. Ideally it should be proposed upstream to CGAL ...

@sloriot About the fix to corefinement, what do you think ?

@sloriot
Copy link
Contributor

sloriot commented Apr 22, 2020

Indeed you should not hack that code because it is correct :)
If you need to have exactly one connected component per solid, then you have to split that solid (it will save you the trouble of redoing that when you update the coref code).
Don't you have a split function somewhere in SFCGAL (it does remind me something)?

CGAL 5.1 will officially introduce the split function but for earlier releases you can use Face_filtered_graph, connected_components() and copy_face_graph(), like in this example that is missing the copy_face_graph() call that is done in this one.

@danielcu888
Copy link
Contributor Author

@mhugo, @sloriot thanks for the feedback, much appreciated :)

Firstly, if not done already, a ticket should be raised to remove the CGAL patches if they are undesirable.

Secondly, a disclaimer should be added to any files that the contained code should not be modified, lest anyone else wastes time attempting to do so.

Lastly, I submitted a fix for this issue a while back to use the Nef polyhedra library to post process the results from corefinement into disjoint components, but is was rejected on the basis that that library is non-performant and that corefinement was the recommended way to address this problem, of which this was an attempt, so sorry for the confusion on that. There is evidence that this fix also has other undesirable consequences so I will not re-submit it.

I will attempt to re-address this issue using the recommendations by @sloriot and close this PR.

@danielcu888
Copy link
Contributor Author

I did salvage the commit associated with fixing the WKT bench test which failed for locally on Mac OS X, and submitted that here: #221

@danielcu888 danielcu888 deleted the issue-200-fix-intersection branch April 24, 2020 13:38
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants