Support concave and/or n-vertex faces #85

rahwang · 2017-06-14T21:02:34Z

Fixes #79 and #55.

Modify face-loading logic to support faces that are n-gons that may be concave.

Also replacing the box-triangles obj test file, because it didn't actually have triangular faces.

Can you review, @lilleyse ?

pjcozzi · 2017-06-14T21:14:16Z

rahwang · 2017-06-14T21:22:43Z

@lilleyse and I discussed the broken tests -- looks like they differ only in the long generated string. Not sure what causes this? The test objs look fine though.

Also, my changes introduce quite a bit more computation per face. I'm not familiar with the sorts of usage this converter gets, but if really large models are common (and the n-gon / concave face cases are uncommon), it might be worth making this an option and using a simple face method for triangulation the default case.

pjcozzi · 2017-06-14T21:36:30Z

Performance will definitely be important here. Instead of relying on user input, can't we do a quick test to see if this is needed or not?

erich666 · 2017-06-14T22:20:31Z

Right, I suspect 90% of the time it's a quick out, "there are three vertices." Another 9.9% of the time it'll be a quad, and maybe there's a quick out for those, but I don't know one off-hand. I'd use:

Graphics Gems IV: Schorn, Peter, and Fisher, Frederick, Testing the Convexity of a Polygon, p. 7-15, code: p. 11-15, convex_test/.

Code is here:
http://www.realtimerendering.com/resources/GraphicsGems/gemsiv/convex_test/ - two versions, and I assume _opt.c is newer and better.

There might be something newer somewhere, but this is the one I know, and I recall it's pretty optimal (it can also identify degenerate polygons, which you can simply discard if you know they're part of a surface).

pjcozzi · 2017-06-14T22:25:04Z

Thanks for the reference, @erich666!

rahwang · 2017-06-15T23:23:36Z

Hey @lilleyse , would you like to start looking at this? I'm not sure what to do about winding ordering.

rahwang · 2017-06-16T18:09:55Z

Ok, I added winding order sanitization for when normals are provided.

I haven't added degenerate polygon sanitization, but this PR has already gotten a lot bigger than I originally intended, so perhaps that's best left to a follow up PR.

@lilleyse

pjcozzi · 2017-06-19T17:19:22Z

Bump @lilleyse

pjcozzi · 2017-06-21T18:04:33Z

Bump @lilleyse

mramato · 2017-06-23T14:47:54Z

@rahwang The reason the tests just look like really long string differences is because jasmine is doing a comparison of 2 gltf objects, this test fails because the output has changed. Jasmine doesn't know how to do anything other than stringify the objects as part of the error message (however this is enough for us to go on.)

If you diff the two strings provided by the error message, what you find is that the glTF buffers are different between master and this branch (since they are base64 encoded it's hard to say at a glance exactly what is different). Long story short, (too late) if you convert specs/data/box/box.obj in this branch, you end up with a different glTF than in master.

It's possible that your changes are still correct and that the test is too fragile and needs to be updated, but the important next step for you here is to convert box.obj both in master and in this branch and determine exactly what is different and why and which one is more correct. (I would also recommend you merge master into this branch before doing that just in case).

Hope that helps!

rahwang · 2017-06-26T12:38:09Z

Thank @mramato! I'll try to hunt down the differences once I get another chance to talk to Sean -- we were also considering moving the winding order correction into gltf pipeline.

lilleyse · 2017-06-26T13:51:53Z

lib/loadObj.js

+                parseFaceLine(line, true, true);
+            } else if (facePattern4.test(line) || (continuedLine && facePattern8.test(line))) { // format "v//n v//n v//n ..."
+                parseFaceLine(line, false, true);
+            }


After looking at the multi-line parsing a bit, I think it could be simplified by buffering the string rather than buffering the parsed data. The face parsing code should take a single string regardless of whether it originally spanned multiple lines or not.

Then with some modifications to the first four regexes it should be possible to capture all the attributes like before, with addFace just handling them in a more generic fashion.

Ah, that would definitely be neater -- I'll modify this to use line buffering now.

lilleyse · 2017-06-26T13:58:34Z

It's possible that your changes are still correct and that the test is too fragile and needs to be updated

Originally those tests acted as "integration" tests, but they are too fragile and should probably be removed.

rahwang · 2017-06-26T17:19:58Z

Thanks for the suggestion, @lilleyse ! I reorganized the line buffering. This is ready for further review.

lilleyse · 2017-06-26T19:52:05Z

lib/loadObj.js

+    // facePosition : indices into position array
+    // faceUvs : indices into uv array
+    // faceNormals : indices into normal array
+    function parseFaceLine(line, hasUvs, hasNormals) {


It might be best to remove this function entirely and put the logic back inside each of the cases within parseLine. That way we can avoid separate areas having similar if-else chains.

Along with this, remove faceSpacePattern and faceSpaceOrSlashPattern and capture attributes in the four main regex's instead.

After reading up on this more, it may not be possible to capture everything since it's looking for n occurrences...

Yeah, as far as I can tell, you have to just iterate through the string and grab attributes manually if there are n of them -- capture groups don't work with n elements.

As for getting rid of parseFaceLine, this will create a bunch of repeated code in each of the face cases --is that ok?

I haven't been following this thread too closely, but a single regex match can capture all occurrences of a given pattern and will be way faster than manual string manipulation (which should always be avoided if possible). See https://stackoverflow.com/a/14707522/3191039

lilleyse · 2017-06-26T20:00:32Z

lib/loadObj.js

-                result[10], result[11], undefined, result[12]
-            );
+        } else { // face line or invalid line
+


Remove empty line.

lilleyse · 2017-06-26T20:11:49Z

lib/loadObj.js

+
+        var facePositions = [];
+        var faceUvs = [];
+        var faceNormals = [];


These are good to have but will allocate for every face line that is parsed. Instead put these at global scope and call facePositions.length = 0 for each at the end of the parsing.

To clean up some of the code here and in addFace don't set these to undefined if the face doesn't use that attribute, just pass the arrays as-is and check the length if required. It's fine to index out-of-bounds like below since the result will just be undefined.

addVertex(vertices[0], positions[0], uvs[0], normals[0]);

lilleyse · 2017-06-26T20:12:45Z

lib/loadObj.js

+            } else if (facePattern4.test(lineBuffer)) { // format "v//n v//n v//n ..."
+                parseFaceLine(line, false, true);
+            }
+            lineBuffer = "";


Style: use single quotes.

lilleyse · 2017-06-26T20:12:51Z

lib/loadObj.js

@@ -91,6 +101,9 @@ function loadObj(objPath, options) {
    // All mtl paths seen in the obj
    var mtlPaths = [];

+    // Buffers for face data that spans multiple lines
+    var lineBuffer = "";


Style: use single quotes.

lilleyse · 2017-06-26T20:14:05Z

lib/loadObj.js

-
+var vertexPattern = /v( +[\d|\.|\+|\-|e|E]+)( +[\d|\.|\+|\-|e|E]+)( +[\d|\.|\+|\-|e|E]+)/;     // v float float float
+var normalPattern = /vn( +[\d|\.|\+|\-|e|E]+)( +[\d|\.|\+|\-|e|E]+)( +[\d|\.|\+|\-|e|E]+)/;    // vn float float float
+var uvPattern = /vt( +[\d|\.|\+|\-|e|E]+)( +[\d|\.|\+|\-|e|E]+)/;                              // vt float float


Can these three also benefit from the {3} syntax like below?

I don't think they do, because unlike the face line parsing, there are a set number of them them, each with a capture group. The face lines are different because there are n items. The {3,} just specifies at least 3.

lilleyse · 2017-06-26T20:23:02Z

lib/loadObj.js

        }
    }

+    var scratchNormal = new Cartesian3();
+    function addFace(vertices, positions, uvs, normals) {


How easy would it be to move winding order correct / triangulation code into a separate file?

Definitely doable, but the file logic will be pretty tangled though, because it will essentially be all the logic from addFace, which uses addVertex, the global normals / position buffers and the face position and normal buffers. Don't know if it's worth it?

rahwang · 2017-06-27T20:47:47Z

Ok @lilleyse , ready for another look!

lilleyse

I'm really late here, but the approach is a lot cleaner now.

lilleyse · 2017-07-24T18:30:55Z

lib/loadObj.js

+    var faceVertices = []; // names of vertices for caching
+    var facePositions = []; // indices into position array
+    var faceUvs = []; // indices into uv array
+    var faceNormals = []; // indices into normal array


The inline comments probably aren't needed.

lilleyse · 2017-07-24T19:51:39Z

lib/loadObj.js

@@ -135,6 +152,94 @@ function loadObj(objPath, options) {
        primitive.material = getName(name);
    }

+    var intPoint = new Cartesian3();


Rename to scratchIntersectionPoint. Also prepend scratch to the other variables.

lilleyse · 2017-07-24T20:13:21Z

lib/loadObj.js

+    var intPoint = new Cartesian3();
+    var xAxis = Cesium.Cartesian3.UNIT_X.clone();
+    var yAxis = Cesium.Cartesian3.UNIT_Y.clone();
+    var zAxis = Cesium.Cartesian3.UNIT_Z.clone();


Do these need be cloned from UNIT_X and others? I think they can just be new Cesium.Cartesian3().

lilleyse · 2017-07-24T20:22:22Z

lib/loadObj.js

+    var ray = new Ray();
+    var plane = new Plane(Cesium.Cartesian3.UNIT_X, 0);
+    function projectTo2D(positions) {
+        var positions2D = new Array(positions.length);


It would be best to use a scratch array for positions2D too. There should also be some bank of scratch Cartesian2 that can be used in place of the allocations below. The same applies to the cartesians that are allocated for positions3D.

lilleyse · 2017-07-24T20:24:31Z

lib/loadObj.js

+    var normal = new Cartesian3();
+    var ray = new Ray();
+    var plane = new Plane(Cesium.Cartesian3.UNIT_X, 0);
+    function projectTo2D(positions) {


Move this function down below a bit, maybe right above get3DPoint.

rahwang · 2017-07-27T18:09:58Z

Thanks for reviewing, @lilleyse ! Did requested cleanup edits.

lilleyse · 2017-08-08T14:41:53Z

@rahwang could you test out the model in #97? The triangulation is working but I think the winding order may be producing inconsistent results for 4-sided polygons.

In the second image I removed back face culling from the gltf.

rahwang · 2017-08-08T16:02:22Z

Thanks for catching that @lilleyse ! It was a small bug with some scratch cart3s. Fixed. The converted model now looks like this:

03lafaye · 2017-08-09T18:41:04Z

Awesome! Thanks!

lilleyse · 2017-08-09T21:54:14Z

Thanks a lot @rahwang!

rahwang added 2 commits June 13, 2017 20:48

get n-many vertices for a face

133e428

loads concave faces with more than 3 vertices

50d26f4

rahwang force-pushed the more-vertices-faces branch from fbcd28b to 50d26f4 Compare June 14, 2017 21:09

adding convexivity test and fan method for convex polygon triangulation

7e82e90

rahwang force-pushed the more-vertices-faces branch 2 times, most recently from 9697980 to 259bdd5 Compare June 15, 2017 22:58

refactored face line parsing to support multiline faces

2396899

rahwang force-pushed the more-vertices-faces branch from 259bdd5 to 2396899 Compare June 16, 2017 16:59

adding winding order sanitization when normals are provided

2105e7b

rahwang force-pushed the more-vertices-faces branch from 58823f6 to 2105e7b Compare June 16, 2017 18:08

mramato mentioned this pull request Jun 23, 2017

Keep diffuse intact so that shader generation for generateNormals works #89

Merged

lilleyse reviewed Jun 26, 2017

View reviewed changes

rahwang added 2 commits June 26, 2017 12:23

refactor line buffering

a07ef6a

correct cross product calculation

2706b95

rahwang force-pushed the more-vertices-faces branch from 3da0063 to 2706b95 Compare June 26, 2017 16:52

lilleyse reviewed Jun 26, 2017

View reviewed changes

refactor regexes and correct line buffering bug

f526e9b

rahwang force-pushed the more-vertices-faces branch from 6b5ebb7 to f526e9b Compare June 28, 2017 14:05

lilleyse reviewed Jul 24, 2017

View reviewed changes

lilleyse and others added 2 commits July 24, 2017 16:42

Style edits

9da383e

cleanup edits

563d524

rahwang force-pushed the more-vertices-faces branch from aff89ff to 563d524 Compare July 27, 2017 18:05

lilleyse mentioned this pull request Aug 8, 2017

Rendering issue with converted Google Blocks OBJ models #97

Closed

correct winding order check bug

3293621

lilleyse merged commit b20df62 into CesiumGS:master Aug 9, 2017

lilleyse mentioned this pull request Aug 23, 2017

Handle objs with more than 4 vertices per face #55

Closed

Support concave and/or n-vertex faces #85

Support concave and/or n-vertex faces #85

Conversation

rahwang commented Jun 14, 2017 • edited

pjcozzi commented Jun 14, 2017

rahwang commented Jun 14, 2017

pjcozzi commented Jun 14, 2017

erich666 commented Jun 14, 2017

pjcozzi commented Jun 14, 2017

rahwang commented Jun 15, 2017

rahwang commented Jun 16, 2017

pjcozzi commented Jun 19, 2017

pjcozzi commented Jun 21, 2017

mramato commented Jun 23, 2017

rahwang commented Jun 26, 2017

lilleyse Jun 26, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lilleyse commented Jun 26, 2017

rahwang commented Jun 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rahwang commented Jun 27, 2017

lilleyse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rahwang commented Jul 27, 2017

lilleyse commented Aug 8, 2017

rahwang commented Aug 8, 2017

03lafaye commented Aug 9, 2017

lilleyse commented Aug 9, 2017

rahwang commented Jun 14, 2017 •

edited

lilleyse Jun 26, 2017 •

edited