Fix OBJLoader to support more than three vertices in one face #10917
Conversation
|
|
||
| for ( var j = 2; j < ivs.length; j ++ ) { | ||
|
|
||
| this.addVertex( ivs[ 0 ], ivs[ j - 1 ], ivs[ j ] ); |
There was a problem hiding this comment.
As far as I can see this builds triangles by connecting each neighbouring pair of face vertices with the first vertex. So I believe this only works for convex faces ? And can potentially produce very high aspect ratio triangles ?
Is three ok with zero area triangles (when the three vertices are colinear) ?
There was a problem hiding this comment.
So I believe this only works for convex faces ? And can potentially produce very high aspect ratio triangles ?
Yes. The former OBJLoader connect neighboring pair of vertices with last vertex.
Is three ok with zero area triangles (when the three vertices are colinear) ?
I think the model editor need to take care of this. (broken model)
|
Could you remove the builds from the PR? |
|
@mrdoob Of course, done. |
|
|
||
| } | ||
|
|
||
| state.addFace( vs, us, ns ); |
There was a problem hiding this comment.
Have you considered the performance here? Have you ran tests how much this slows down different sized obj file loads. This seems to throw out out a lot of the tuning that has been done to make common obj load faster.
You are doing a secondary regexp exec (that I tried to minimize to the bone) var rawdata = /^f\s+/.exec( line );. Then slicing, splitting and doing multiple for loops.
Surely this slows down the exporter for the common case users? Isn't there another way to make a separate code path for just the > 3 vertices case?
There was a problem hiding this comment.
As far as I can tell from my earlier testing the slow parts in the loader are array manipulations. This adds a bunch more. Just wondering about the loader perf impact here.
There was a problem hiding this comment.
Haven't considered performance issue. The regexp should only do once. It should be optimized. I'll try my best.
There was a problem hiding this comment.
Yeah once per f line, but potentially hundreds of thousands of lines for large files. Everything adds up. Temp arrays will trash GC etc.
Consedering what this tries to fix afaik is rather rare, it seems bad to put a perf penalty for all the apps out there that use obj as their assets.
There was a problem hiding this comment.
@jonnenauha, how were you doing your performance testing? IMO this is a pretty valuable change and it would be worth getting it right.
|
Ended up merging #11871, which was similar to this PR but a bit simpler. |
|
Many thanks for the PR though! Would be curious to know if the current version of the |
Reference to #10438 and http://stackoverflow.com/questions/41154498/threejs-some-material-texture-faces-triangles-not-rendering/41257937#41257937
Though it should be done at the exporter side to make 3 vertices per face. Some application (ArichCAD) do not have that option.
I saw there's some triangulation code in OBJLoader already for supporting 4 vertices per face. So update a little to support more.