Combine primitives #162

lasalvavida · 2016-08-12T17:54:01Z

This is worth an initial look and some testing while I add tests for everything.

This pull request contains a few things:

Re-adds combine primitives, closing out Combine Primitives needs to be fixed #108 and by proxy BrainStem sample model combinePrimitives throws exception #85 and Buggy model gets mangled by pipeline #103.
Splits out some primitive helper functions from combineNodes into PrimitiveHelpers.
Removes underscore usage Remove underscore usage from combineNodes #160.
mergeBuffers will no longer fail if the new buffer name is the same as an already existing one.
Fixes an error in removeDuplicatePrimitives.removePrimitivesFromMesh
Adds a second pass of light optimization

Some preliminary testing with the buggy model:

	Original	Master	Master w/ combine primitives
File size	10,414 KB	6,091 KB	6,046 KB
Nodes	208	179	179
Meshes	109	163	68
Draw Calls	236	273	236

lilleyse · 2016-08-12T19:57:27Z

lib/PrimitiveHelpers.js

+
+var AccessorReader = require('./AccessorReader');
+var readAccessor = require('./readAccessor');
+var writeAccessor = require('./writeAccessor');


Same note about include order from the other PR.

lilleyse · 2016-08-12T19:58:36Z

Looks great! I did some simple testing and it seems to work well.

pjcozzi · 2016-08-12T21:21:11Z

@lasalvavida can you verify that it makes sense for the buggy model to have 95 fewer meshes, but only 37 fewer draw calls?

pjcozzi · 2016-08-12T21:23:36Z

lib/PrimitiveHelpers.js

+var readAccessor = require('./readAccessor');
+var writeAccessor = require('./writeAccessor');
+
+module.exports = {


@lilleyse @lasalvavida can you two please review the design of this class? I don't think it is needed. Some functions could be private in the files they are used in; others could be standalone functions.

Let me know if you need help.

lasalvavida · 2016-08-12T21:26:53Z

@pjcozzi Master currently bloats the number of meshes in cases where two meshes that are in node endpoints that can't be combined contain identical primitives. That is what drives up the draw call count. The output with this PR removes that bloating by recombining meshes after primitives get combined. The primitives are consolidated to fewer mesh endpoints, but there is still the same number of primitives.

tl;dr It has the same number of draw calls as the version in the sampleModels, the current output of master is adding more unnecessary draw calls.

pjcozzi · 2016-08-13T13:18:36Z

Gotcha.

…ne into combine-primitives

lilleyse · 2016-10-03T17:17:08Z

@lasalvavida Is there more to do for this? The latest merge commit has a few issues.

lasalvavida · 2016-10-07T17:03:43Z

There was a bad merge; it still needs tests. I'll add those now.

lasalvavida · 2016-10-07T19:57:21Z

@lilleyse This is updated and ready for a look

…f-pipeline into combine-primitives

lasalvavida · 2016-10-28T14:59:46Z

I have removed the second mergeDuplicateVertices call. With the light optimization, it is no longer needed; the files produced with and without it are the same size.

pjcozzi · 2016-10-31T18:25:40Z

@lilleyse will you be able to review this soon? We have a lot of open PRs in this branch and I would like to get them merged.

lilleyse · 2016-10-31T20:06:09Z

Yes I will review this today.

lilleyse · 2016-11-01T02:55:42Z

lib/Pipeline.js

+    MergeDuplicateProperties.mergeAccessors(gltfWithExtras);
+    removeDuplicatePrimitives(gltfWithExtras);
+    combinePrimitives(gltfWithExtras);
+    combineMeshes(gltfWithExtras);


I think we talked offline about this a while ago, but I forget the conversation...

If possible we should try to avoid the second cleanup, and ideally our stages shouldn't be producing unoptimized gltf's. Sorry if I forgot the reason behind this.

lilleyse · 2016-11-01T02:55:58Z

lib/PrimitiveHelpers.js

+}
+
+function transformPrimitives(gltf, primitives, transform) {
+    var inverseTranspose = new Matrix4();


Can't you just do a simple loop over the position and normal arrays, transforming each set of 3 values with the transform and then writing back into the buffer. I don't see the need for involving indices.

No, you cannot. Primitives may share large attribute accessors of which they only index into a portion. Those primitives in turn may have different transforms. We cannot assume that a primitive uses its entire accessor, we must only transform the positions and normals that it uses.

However, this should use AccessorReader instead of readAccessor. I will make that change.

Ah right, thanks for the correction.

lilleyse · 2016-11-01T02:56:02Z

lib/PrimitiveHelpers.js

+function generateIndices(v, k) {
+    void(v); // v is an unused parameter
+    return k;
+}


If still needed, just put this code in transformPrimitives.

lilleyse · 2016-11-01T02:56:13Z

lib/combineNodes.js

-            if (primitivesShareAttributeAccessor(primitive, comparePrimitive)) {
-                if (primitivesHaveOverlappingIndexAccessors(gltf, primitive, comparePrimitive)) {
+            if (PrimitiveHelpers.primitivesShareAttributeAccessor(primitive, comparePrimitive)) {
+                if (PrimitiveHelpers.primitivesHaveOverlappingIndexAccessors(gltf, primitive, comparePrimitive)) {


Create a function for this in PrimitiveHelpers that is called here and inside markPrimitiveConflicts.

lilleyse · 2016-11-01T02:56:27Z

lib/combinePrimitives.js

-                    var primitives = mesh.primitives;
-                    if (defined(primitives)) {
-                        mesh.primitives = combineMeshPrimitives(gltf, meshId, primitives);
+    PrimitiveHelpers.markPrimitiveConflicts(gltf, PrimitiveHelpers.getAllPrimitives(gltf));


It's a little awkward to save the conflicts in the pipeline object. If possible try to keep it in here.

lilleyse · 2016-11-01T03:00:30Z

Thanks for the update @lasalvavida
It's been a while since I looked at this so fill me in case my comments don't make sense. I'll do some testing tomorrow.

lilleyse · 2016-11-02T18:32:30Z

I'm having issues converting this model:
dragon_low.gltf.txt

…f-pipeline into combine-primitives

… this was first written

lasalvavida · 2016-11-04T22:23:26Z

This is mostly done now. I'm still getting slightly smaller models if I run them through the pipeline twice, and I'm still trying to pinpoint exactly why.

lasalvavida · 2016-11-11T15:18:08Z

This is ready for review. Referring to my original benchmarks the stats are now:

	Original	Master	Master w/ combine primitives
File size	10,414 KB	6,091 KB	6,023 KB
Nodes	208	179	179
Meshes	109	163	61
Draw Calls	236	273	196

lilleyse · 2016-11-11T16:06:46Z

lib/cesiumGeometryToGltfPrimitive.js

-        }
+    var semantics = getPrimitiveAttributeSemantics(primitive, semantic);
+    if (semantics.length <= 0) {
+        primitive.attributes[semantic] = createAccessor(gltf, packedLength, 'VEC3', WebGLConstants.FLOAT, WebGLConstants.ARRAY_BUFFER);


Is hardcoding VEC3 a problem for texture coordinates?

Ah yes, you're right

lilleyse · 2016-11-11T16:07:59Z

This is really close. Every model I've tested works fine. Those numbers are nice!

lasalvavida · 2016-11-11T16:18:19Z

Updated

lilleyse · 2016-11-11T16:38:45Z

Nice, I'm glad to finally get this in! Also because of adjustments here the pipeline just runs a lot faster.

lasalvavida added 4 commits August 12, 2016 13:17

Changes for combinePrimitives

ec48eec

Fixes for push

fc6670c

Add deep-equal to package.json

4b17931

Remove underscore from package.json

198cacb

lilleyse reviewed Aug 12, 2016
View reviewed changes

Removed intermediate removeUnusedVertices, it has no practical effect

d484793

pjcozzi reviewed Aug 12, 2016
View reviewed changes

Added some test skeletons

7df552d

Merge branch 'master' of github.com:AnalyticalGraphicsInc/gltf-pipeli…

ea5ef83

…ne into combine-primitives

lasalvavida added 2 commits October 7, 2016 15:46

Updated with tests

59495ae

Removed underscore from packages

fd8e508

lasalvavida added 3 commits October 28, 2016 09:52

Merge branch 'master' of https://github.com/AnalyticalGraphicsInc/glt…

17f2978

…f-pipeline into combine-primitives

Fixed a few merge issues

f128ad1

Updated doc

050375f

Merge branch 'master' of https://github.com/AnalyticalGraphicsInc/glt…

75d5f5b

…f-pipeline into combine-primitives

lilleyse reviewed Nov 1, 2016

View reviewed changes

lasalvavida added 3 commits November 4, 2016 10:04

Merge branch 'master' of https://github.com/AnalyticalGraphicsInc/glt…

df60b6e

…f-pipeline into combine-primitives

Lots of changes and cleanup with new utilities that didn't exist when…

c60397b

… this was first written

Changes to pipeline optimization stages

9d9069b

lasalvavida added 2 commits November 11, 2016 10:14

Finalized minimal pipeline order

8eac6aa

Need first combinePrimitives

0ff673f

lilleyse reviewed Nov 11, 2016

View reviewed changes

Handle TEXCOORD VEC2 case

30648a4

lilleyse merged commit 6ca0f8c into master Nov 11, 2016

lilleyse deleted the combine-primitives branch November 11, 2016 16:38

This was referenced Nov 11, 2016

Combine Primitives needs to be fixed #108

Closed

BrainStem sample model combinePrimitives throws exception #85

Closed

Buggy model gets mangled by pipeline #103

Closed

Remove underscore usage from combineNodes #160

Closed

Review npm dependencies #178

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Combine primitives #162

Combine primitives #162

lasalvavida commented Aug 12, 2016 •

edited

Loading

lilleyse Aug 12, 2016

lilleyse commented Aug 12, 2016

pjcozzi commented Aug 12, 2016

pjcozzi Aug 12, 2016

lasalvavida commented Aug 12, 2016 •

edited

Loading

pjcozzi commented Aug 13, 2016

lilleyse commented Oct 3, 2016

lasalvavida commented Oct 7, 2016

lasalvavida commented Oct 7, 2016

lasalvavida commented Oct 28, 2016

pjcozzi commented Oct 31, 2016

lilleyse commented Oct 31, 2016

lilleyse Nov 1, 2016

lilleyse Nov 1, 2016

lasalvavida Nov 4, 2016

lasalvavida Nov 4, 2016

lilleyse Nov 4, 2016

lilleyse Nov 1, 2016

lilleyse Nov 1, 2016

lilleyse Nov 1, 2016

lilleyse commented Nov 1, 2016

lilleyse commented Nov 2, 2016

lasalvavida commented Nov 4, 2016

lasalvavida commented Nov 11, 2016

lilleyse Nov 11, 2016

lasalvavida Nov 11, 2016

lilleyse commented Nov 11, 2016

lasalvavida commented Nov 11, 2016

lilleyse commented Nov 11, 2016

Combine primitives #162

Combine primitives #162

Conversation

lasalvavida commented Aug 12, 2016 • edited Loading

Choose a reason for hiding this comment

lilleyse commented Aug 12, 2016

pjcozzi commented Aug 12, 2016

Choose a reason for hiding this comment

lasalvavida commented Aug 12, 2016 • edited Loading

pjcozzi commented Aug 13, 2016

lilleyse commented Oct 3, 2016

lasalvavida commented Oct 7, 2016

lasalvavida commented Oct 7, 2016

lasalvavida commented Oct 28, 2016

pjcozzi commented Oct 31, 2016

lilleyse commented Oct 31, 2016

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

lilleyse commented Nov 1, 2016

lilleyse commented Nov 2, 2016

lasalvavida commented Nov 4, 2016

lasalvavida commented Nov 11, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lilleyse commented Nov 11, 2016

lasalvavida commented Nov 11, 2016

lilleyse commented Nov 11, 2016

lasalvavida commented Aug 12, 2016 •

edited

Loading

lasalvavida commented Aug 12, 2016 •

edited

Loading