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

Fix earcut bug - triangulating faces with +4 vertices #32

Merged
merged 12 commits into from
Nov 30, 2020

Conversation

eyalperry88
Copy link
Contributor

As mentioned here in this PR as well as this earcut issue - the triangles returned by earcut do not correspond to the original face order

This fix adds a small check per face - the first triangle is verified that it contains at least one edge in the correct order from the original face. If not, flip the triangle.

The way to verify this is using this closed model of a Dodecahedron (fully folded):
dodecahedron.fold.zip

Also, I changed is2d from always true to always false. But to be honest I don't know what are the effects of this. It did look very weird that earcut gets a 2d projection of the faces for triangulation (I can try to make a model where this fails, something like a pentagon that is on the Y plane)

@eyalperry88
Copy link
Contributor Author

Unfortunately, this file: http://robbykraft.github.io/Origami/files/fold/crane.fold is still unhappy :(

@eyalperry88
Copy link
Contributor Author

Okay and now checked with issue #21 and we also get flipped faces... (it actually happens before you save and load as FOLD - you see the flipped faces even when you load the SVG)

Is that a good thing or a bad thing?

@eyalperry88
Copy link
Contributor Author

A somewhat more minimal example.

test.svg.zip

When you load the following SVG (small excerpt from the maze) into OrigamiSimulator-master with vertex merge tolerance of 1pixel - it folds flat.

When you do it on my branch, exactly one pentagon face is flipped and it cannot fold flat.

To be continued

@eyalperry88
Copy link
Contributor Author

eyalperry88 commented Nov 18, 2020

Ok, found a problem - I was checking for the orientation using the faceEdges and edges structures but it's was not actually oriented correctly. I suspect this might lead to other problems (maybe #21 ) but for this PR I could just check the orientation using the original face structure, which is correctly oriented.

Now the Dodecahedron, the Crane and the OrigamiSimulator Map are all loaded correctly into a flat folded state (!)

If I try to save to FOLD and load the origami simulator map - I still get exactly three missing triangles. Which seems quite better than before, though not perfect:

missing

…triangualation (even when dim=3), it is important we give it a non degenrate face.. this commit checks which coordinates have the largest variance and rearanges the dimensions accordingly. this doesnt affect the model in anyway
@eyalperry88
Copy link
Contributor Author

An important update.

Diving into the earcut code (both the one in the repo as well as the most updated for github) - there is something very odd, while it does support multiple dimensions, for triangulation only the first two dimensions are being used! You can see this behavior in lines 50 & 52:

https://github.com/mapbox/earcut/blob/master/src/earcut.js#L50

The new node only contains X and Y (regardless of dim)

This used to work fine when "everything" was 2d but when you load a FOLD format that was exported from SVG it's actually 3d with the second coordinate always zero and then you get the holes seen in issue #21 . I verified this by opening the .FOLD file in fold viewer and you can see the five-vertices face is there and correctly oriented.

As a fix to this behavior, I added quite a large chunk of code that re-orders the coordinates so the first two always have the largest variance. To be honest, it might be worth considering switching to a different triangulation library, or doing it ourselves.

Now the OrigamiSimulator map is loading fine, including after saving and loading from SVG. There still is some problem with the Dodecahedron though, will update soon

@eyalperry88
Copy link
Contributor Author

Ok maybe last update :) using the variance didn't work because the dodecehadron actually had two faces that had some variance but were co-linear. Instead, in my last commit I just check all options for dimension ordering until we get a triangulation back from earcut.

This now works for the Dodecahdren and fix the OrigamiSimulator map bug described in #21

* Correct is2d detection: 2D only if all vertices are 2D,
  and treat 0 the same as an absent third coordinate.
* Always swap y and z coordinates when importing/exporting FOLD,
  independent of 2D status, for consistent behavior in all cases.
  (To get there, `rawFold` also has swapped coordiantes.)
* To decide `needsFlip`, instead of looking for a face edge in the first
  triangle, which can fail if the first triangle is a fully interior triangle,
  look for the first edge of the face in all the triangles.
@edemaine edemaine linked an issue Nov 28, 2020 that may be closed by this pull request
@edemaine
Copy link
Collaborator

edemaine commented Nov 28, 2020

Awesome, thanks for this. Trying all three pairs of coordinates seems ugly, but it seems like a good workaround for now.

I did a revision pass both for style and for correctness; see commit messages and diff for description:

  • Fix indentation to match coding style; remove unnecessary changes
  • Correct is2d and needsFlip detection and y/z flipping
    • Correct is2d detection: 2D only if all vertices are 2D,
      and treat 0 the same as an absent third coordinate.
    • Always swap y and z coordinates when importing/exporting FOLD,
      independent of 2D status, for consistent behavior in all cases.
      (To get there, rawFold also has swapped coordiantes.)
    • To decide needsFlip, instead of looking for a face edge in the first
      triangle, which can fail if the first triangle is a fully interior triangle,
      look for the first edge of the face in all the triangles.

I think this should be good to go now, but it would be good to test everything again, as I made significant changes to the algorithms you were using. The dodecahedron example seems to work, as does Robbie's crane.fold, and all built-in examples I tested, some other SVGs I had lying around, and maze folding save and load (for 0% folded -- getting incorrect target angles for folded versions, but that seems to be another issue). No more missing/flipped triangles! Could you please test more before we merge?

js/pattern.js Outdated Show resolved Hide resolved
Swapping y/z flips the coordinate system between left- and right-handed,
effectively reversing the notions of cw/ccw. This isn't an issue in 2D,
because there it would be the same as mapping (x, y, z) → (x, -z,
y) (because z=0) which doesn't flip handedness, but it's a problem in 3D.
… (because of it using only the first two coordinates, and possible degeneracies there). this update makes sure there are at least enough triangles before breaking
}
triangles = earcut(faceVert, null, 3);
// make sure we got *enough* triangle to cover the face
if (triangles.length >= 3 * (face.length - 2)) break;
Copy link
Contributor Author

@eyalperry88 eyalperry88 Nov 29, 2020

Choose a reason for hiding this comment

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

Just added this update. I found another problem, here is how to recreate:

  • Open OrigamiSimulator Map SVG (Vertex tolerance: 1px)
  • Save as FOLD (do not triangulate)
  • Load FOLD
    On my end, I see exactly two triangles missing (I see this in a previous comment too if you notice)

Here is the FOLD file for reference:
origamisimulator.fold.zip

Debugging it, it seems that again because of earcut using the first two coordinates, for some faces (very few..) we get less triangles than needed to cover the face. In this FOLD, there are two 5 vertices faces that earcut returns only two triangles (one vertex is out of the game).

My fix is checking that the number of triangles is at least the number of vertices on the face - 2. That is always right, right? Using this check, we try giving earcut a different coordinate system and it eventually returns the correct triangulation. I definitely think replacing earcut would be a better option in the future though..

We're getting close... :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, assuming no added vertices (which must be the case for earcut, because it has no mechanism to return additional vertices), the number of triangles should be exactly face - 2, so this seems like a good change.

What do you think about also changing the order that we try the dimension pairs to start with x and z, like we do in 2D? It shouldn't matter, but for files that were originally flat and imported, this seems like a better starting point, and kind of nice to try the is2d algorithm and then continue? We could just reverse the loop...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good to me. Pushed a commit where I simply reversed the loop and checked with the FOLD and SVG models and it all seems fine

Copy link
Collaborator

@edemaine edemaine left a comment

Choose a reason for hiding this comment

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

OK, I ran another series of tests, and everything is looking great. Thanks for working so hard on this. In hindsight, I've worked around this bug a lot in the past. This will make my and many people's lives easier!

@edemaine edemaine merged commit b3fffe6 into amandaghassaei:master Nov 30, 2020
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.

Bug in FOLD save or load
2 participants