UGGrid creation with boundary segments segfaults

mentioned in commit 2541de02

mentioned in merge request !145 (merged)

Hi Martin,

please try

It fixes the problem for me.

Do you need backports?

Hi Oliver,

great, thanks. This would fit my observation that a certain grid created with fewer vertices worked nicely - and if I recall correctly that grid should have had no interior vertices. I'll check this.

Probably we won't need explicit backports right now, but seeing this in 2.5.1 would of course be nice.

Yours Martin

signature.asc

closed via commit 2541de02

closed via commit 209a515c

mentioned in commit 6e3d1817

mentioned in merge request !147 (merged)

mentioned in commit 39f63611

mentioned in merge request !148 (merged)

reopened

Hi Oliver, the patch you provided seems to work by accident for my test case, but triggers a bug in other cases. In particular accessing the isBoundarySegment array yields no valid vertex indices. We've developed a patch that appears to work for at least the two cases we had problems with before. The quite simple diff against the old code is here: *******:~/Dokumente$ diff uggridfactory.cc uggridfactoryOld.cc 299,303d298 < < std::vector<unsigned int> inverseNodePermutation(nodePermutation.size()); < for (unsigned int i=0; i<isBoundaryNode.size(); ++i) { < inverseNodePermutation[nodePermutation[i]] = i; < } 334,340c329,330 < for (int j=0; j<dimworld*2-2; j++) { < if(boundarySegmentVertices_[i][j] < 0) { < vertices_c_style[j] = boundarySegmentVertices_[i][j]; < } else { < vertices_c_style[j] = inverseNodePermutation[boundarySegmentVertices_[i][j]]; < } < } --- > for (int j=0; j<dimworld*2-2; j++) > vertices_c_style[j] = boundarySegmentVertices_[i][j]; Essentially we just need to apply the same vertex index permutation as for the vertices themselves, but only the inverse permutation is stored in nodePermutation, so we create its inverse separately.

The diff is better readable if you use diff -u or even better, git diff. If you want us to apply changes, make a local commit and send the patch created with git format-patch origin/master. Even better, fork the project, push your commit and open a merge request. Just for your future reference.

moved from staging/dune-uggrid#18 (moved)

mentioned in commit abbd17dd

mentioned in merge request !152 (merged)

mentioned in commit f8570134

@martin.weiser Were those other cases in 3d? I noticed that there -1 is used as a sentinel value and should be preserved (the < 0 case in your patch), but otherwise isBoundaryNode[i] should give the same as inverseNodePermutation[i] as far as I understood.

Your patch also looks to be against a version not including @oliver.sander's changes from !145 (merged) (though non-unified diffs are hard to read).

Preserving the -1 sentinel should be fixed by !152 (merged). If that is not enough for you, could you please give a bit more details about the failing cases?

Indeed the other case was 3d with triangular faces, which caused the trouble with -1 in the fourth index of the boundary patch.

Digging into the code of BoundaryExtractor indeed suggests that isBoundaryNode[i] should be the same as inverseNodePermutation[i] for boundary nodes i, which is all that is required here.

So I guess Oliver's patch just missed the 3D case with triangular faces.

We'll try your merge request 152 next week if we manage to extract the code from gitlab.

signature.asc

added bug label

changed milestone to %DUNE 2.6.0

assigned to @ansgar

closed via commit 8f853f72

closed via commit f8570134

closed via merge request !152 (merged)

mentioned in commit df425567

mentioned in commit 4a7f9734

mentioned in commit exadune/dune-grid@8f853f72

UGGrid creation with boundary segments segfaults

Designs

Child items ...

Activity