Added generateNormals stage

[lilleyse]

This adds support for generating normals if they do not exist.

[coveralls]

Coverage increased (+0.2%) to 94.067% when pulling 19b3982 on generate-normals into f9b43f9 on master.

[coveralls]

Coverage increased (+0.2%) to 94.067% when pulling 88162d0 on generate-normals into f9b43f9 on master.

[pjcozzi]

@lilleyse consider just using the function in Cesium: https://github.com/AnalyticalGraphicsInc/cesium/blob/master/Source/Core/GeometryPipeline.js#L1059-L1192

Or improve the version in Cesium and use that.

[lilleyse]

Yeah I'll likely go that route. @JoshuaStorm is going to write a gltf primitive <-> Geometry converter so that we can use Cesium functions like these easily.

[JoshuaStorm]

@lilleyse: In generateNormalsSpec, shouldn't we be able to check for equality between box_normals and box_no_normals after it is run through generateNormals? or am I misunderstanding something?

[lilleyse]

I'm not sure if the normals in the box_normals model would be the same as those that are autogenerated.

[JoshuaStorm]

I think I might be misunderstanding:

These are just vertex normals in the geometric sense, correct? If that is the case, shouldn't they be derived, in other words be the same whether they are provided or calculated?

Sorry for the pedantic question, I just want to make sure I am understanding this.

The current result I am getting is shown below, which I am somewhat suspicious is the correct normal values but the indices are out of order somehow.

.

[lasalvavida]

Maybe I'm misreading, but does this just set the vertex normal to be the face normal? That shouldn't be the correct vector.

[lilleyse]

It adds all the face normals that the vertex is a part of, and then normalizes.

[lasalvavida]

Oh, I see thank you

[pjcozzi]

@JoshuaStorm to debug you can tweak the fragment shader (even in the Model Inspector) so the output color is the normal.

The computed normal may not be the same as a provided normal, as a user could provide whatever they want.

[JoshuaStorm]

Looking more into this, the issue does seem to be what we discussed offline @pjcozzi. In both this implementation and the GeometryPipeline implementation of calculating normals, the face normals are just averaged resulting in odd normals at sharp edge vertices. In the case above, the cube is only made up of these sharp edge vertices, resulting in its especially weird normals.

I plan on looking more into the duplicating vertices at edges solution that you also mentioned, @pjcozzi, but I was wondering if I should plan on implementing it here (and not use the GeometryPipeline) or if I should plan on implementing it in Cesium.GeometryPipeline.

[pjcozzi]

@JoshuaStorm the implementation should go into Cesium proper, then gltf-pipeline will pick it up in the next release via npm. You can, of course, code it in this repo to test it, but you'll only want commits to go to the Cesium repo.

[JoshuaStorm]

I am at a bit of a loss if anyone else has any ideas they'd be appreciated!

As a naive test to confirm the shared vertices issue, I iterated through the indices and in any case where there was multiple references to the same vertex, I duplicated that vertex and had the respective index refer to the new duplicate--effectively getting rid of shared vertices by simply duplicating all shared vertices.

Then generating normals for that, I ended up with this box:

With RGB mapped to normal value

I'm wondering if it is possibly for some of the vertices to be in the incorrect winding order?

[pjcozzi]

Maybe, perhaps hack up the code so only 3 of the 6 faces are in the model.

[coveralls]

Coverage increased (+0.2%) to 94.243% when pulling a4d9ec2 on generate-normals into 604626b on master.

[lasalvavida]

@JoshuaStorm I took a stab at this and its pretty close now.

The back normals look like they might be off.

Could you take a look at this?

[JoshuaStorm]

@lasalvavida Your box seems to mirror that of the included box_normals.gltf. Diffing the two files (after running both of them through the pipeline) seems to confirm this as they only differ in a few naming schemes and a bit of buffer data (presumably from the extra data from duplicating the vertices). I'm assuming this means generate normals is doing its job properly? Perhaps not optimally

I could start implementing this solution into Cesium's GeometryPipeline and converting this to use the GeometryPipeline implementation if you're happy with this.

A quick question about the code: what exactly is the purpose for finding the center of the model?

[lasalvavida]

@JoshuaStorm It was some scratch code we can probably take out. I was using it to check if the normals were facing the center of the model and to flip them out if they were. However, it never got called, so I took it out and must have missed the center calculation.

I'd say before doing anything, we need an answer as to why the faces are shaded black. Is it the shader, or is there still something wrong with the data?

[lilleyse]

I'm getting a crash when running the dragon model through:

dragon_no_normals.gltf.txt

[lilleyse]

Yup this and ao. I'll review soon.

[lilleyse]

You can remove these three lines.

[lilleyse]

Box with UVs and no normals (The normal accessor maxes are pretty crazy for this. You should investigate if this is a Cesium issue or a gltf-pipeline isse)

Box with no UVs and no normals Looks correct.

Dragon with no UVs and no normals Looks correct.

Dragon with UVs and no normals The dragon body is correct but the spheres are not. I'm pretty sure the body has UVs and the spheres don't, so there may be some edge case going on. The min/max values for the accessor are really strange here.
gltfs.zip

[JoshuaStorm]

Update on the current state of this, I've been smashing my head into this for a bit, so if someone wants to give it a look with fresh eyes I'd appreciate it, especially since this is holding back publishing. I wouldn't be surprised if I am overlooking something obvious.

Box with UVs and no normals: Same results as before

Models with no normals and no UVs: still working same as before, properly (or at least seemingly)

Dragon with UVs and no normals: The body is still generating normals fine, but the second primitive is not (eyes + sphere)

We suspected that the sphere and eyes had no UVs in the model, but they do in fact. However, even removing the UVs before trying to generate normals resulted in the same odd results. This is what has me most confused

Shapes test: Sean generated this for me, the cylinder and box have UVs, the cone and sphere do not. This has similar results to the dragon: when there are UVs, the primitives that have UVs generate normals properly but the primitives that do not have UVs generate the incorrect normals.

I may have just beat my head into this for too long and need to look at it with fresh eyes tomorrow.

[JoshuaStorm]

Here are the models used. butchered.gltf is the model created by removing the dragon's body primitive and removing the other primitive's UVs

Models.zip

[JoshuaStorm]

I think this is finally ready for review!

Thanks @lasalvavida for pointing out the issue with generating normals before the combine and remove stages.

[lasalvavida]

If I pull master into this branch and run with --ao I get the following exception:

Unhandled rejection TypeError: Cannot assign to read only property 'gltfWithExtras' of C:\Users\rtaglang\AGI\git\gltf-pipeline
at processJSONWithExtras (c:\Users\rtaglang\AGI\git\gltf-pipeline\lib\gltfPipeline.js:92:34)
at c:\Users\rtaglang\AGI\git\gltf-pipeline\lib\gltfPipeline.js:168:9
at c:\Users\rtaglang\AGI\git\gltf-pipeline\lib\readGltf.js:43:17
at tryCatcher (c:\Users\rtaglang\AGI\git\gltf-pipeline\node_modules\bluebird\js\release\util.js:16:23)
at Promise._settlePromiseFromHandler (c:\Users\rtaglang\AGI\git\gltf-pipeline\node_modules\bluebird\js\release\promise.js:504:31)
at Promise._settlePromise (c:\Users\rtaglang\AGI\git\gltf-pipeline\node_modules\bluebird\js\release\promise.js:561:18)
at Promise._settlePromise0 (c:\Users\rtaglang\AGI\git\gltf-pipeline\node_modules\bluebird\js\release\promise.js:606:10)
at Promise._settlePromises (c:\Users\rtaglang\AGI\git\gltf-pipeline\node_modules\bluebird\js\release\promise.js:685:18)
at Async._drainQueue (c:\Users\rtaglang\AGI\git\gltf-pipeline\node_modules\bluebird\js\release\async.js:138:16)
at Async._drainQueues (c:\Users\rtaglang\AGI\git\gltf-pipeline\node_modules\bluebird\js\release\async.js:148:10)
at Immediate.Async.drainQueues as _onImmediate
at processImmediate as _immediateCallback

[JoshuaStorm]

@lasalvavida What model was this produced with? Worked fine for box_uv_no_normals for me, awaiting dragon_uvs_no_normals now

[lasalvavida]

The above seems to be a problem in my WebStorm, it works fine from bash so you can disregard it.

edit: The above is a bug, opened #139

[lasalvavida]

Why did you make this change? If you do this, you're comparing at different levels of the glTF hierarchy.

[JoshuaStorm]

Oops, this was a naive change since processFile will now merge buffers, so gltf.buffers.CesiumTexturedBoxTest will no longer exist.

This could now expect the old buffer to be undefined and compare initialUri with gltf.buffer_0.uri (the only buffer that should be on the gltf), but that doesn't seem to be perfect test either. Thoughts?

[lasalvavida]

Since in this test, there can only be one buffer (I assume). Just get the first buffer:

var firstBufferId = Object.keys(gltf.buffers)[0];
var testBuffer = gltf.buffers[firstBufferId];

[lasalvavida]

I don't think you need to do this here, just after processJSON gets run, right? You do know the initial bufferId is CesiumTexturedBoxTest.

[lasalvavida]

I will note there are some lighting artifacts on some more complex models:

but, I'm fairly certain that's a hard vs. soft normals issue.

[lasalvavida]

A similar effect on buggy:

[lasalvavida]

I just wanted to include the above comments as a frame of reference for when we add support for generating hard normals. This looks great to me, thanks @lilleyse and @JoshuaStorm!

[lilleyse]

Thanks for reviewing @lasalvavida