updating the snowflake example so that it is compatible with latest gltf specifications

[bdysvik]

It seems like our "new" glTF generator were producing outdated glTF files for the line(s) demo. I have updated the generator and validated the results in the official glTF-Validator.
See issue #10526.

[cx20]

It looks good, However, I can not judgement. I would like to ask @javagl who is familiar with the glTF file to review it.

[bdysvik]

That sounds like a good idea.

[javagl]

Just a side note: The main problem was not so much that the files have been "outdated". The technique that was defined in the original file was invalid, regardless of the version. But I did not read the whole history or the full details and all changes, so I might have misunderstood this point.

---

Regarding the updated file: It indeed passes the validation, and thus, is 1.1-compliant - as far as the validator can reasonably check this:

There is still a small error

(EDIT: This applies to both, "snowflake" and "snowflakes")

The "meshProgram" defines the following attributes

"attributes": [
  "a_position",
  "a_color"
],

But in the vertex shader, they are only called position and color (without the a_).

This is something that the validator cannot detect - of course it does not parse the shader code. Something like this can only be detected at runtime, when GL reports a location of -1 when calling

int location = gl.glGetAttribLocation(glProgram, attributeName); 

It seems to be a common practice to prepend the a_ to attributes in the shader code. So the vertex shader should be changed to

precision highp float;

attribute vec3 a_position;
attribute vec3 a_color;

varying vec3 v_color;

uniform mat4 modelViewMatrix;
uniform mat4 projectionMatrix;

void main(void) {
    vec4 pos = modelViewMatrix * vec4(a_position,1.0);
    v_color = a_color;
    gl_Position = projectionMatrix * pos;
}

With this change, it passes validation, is displayed properly in JglTF, and is "valid" as far as I can say.

---

Another side note: There is an ongoing discussion about a possible change in the spec. In glTF 1.0 (and the current state of glTF 1.1), the structure is as follows:

"meshTechnique": {
  "attributes": {
    "a_position": "position", // points to the "position" defined below
    ...
  },
  "parameters": {
    ...
    "position": {
      "type": 35665,
      "semantic": "POSITION" // points to the "POSITION" attribute of the mesh primitive
    },
  },

This indirection may not be necessary. In the final version of glTF 1.1, the attributes entry might point to the mesh.primitive.attributes entry directly:

"meshTechnique": {
  "attributes": {
    "a_position": "POSITION", // DIRECTLY points to the "POSITION" attribute of the mesh primitive
    ...
  },
  "parameters": {
    ...
  },

But this is not yet finalized, and still open for discussion in KhronosGroup/glTF#789

[javagl]

BTW: Uniforms are "often" prefixed with u_, so one could consider renaming it to

uniform mat4 u_modelViewMatrix;

etc. in the shader and the glTF file (!), But this is no strict rule, and I'm also only starting to follow conventions like these....

[bdysvik]

Ok, thanks. I guess I got confused by the way the Three.js glTFLoader translates some of the the shader attribute names in 'replaceTHREEShaderAttributes()'.. Perhaps this part of the loader should be revisited soon..

I changed the attribute names to 'a_position' and 'a_color' and added the color attribute to the loader renaming scheme. I also prefixed the uniforms with 'u_'.

Regarding your 'side note': When I updated our exporter I also changed the way the technique was compiled so it should be valid now.

[mrdoob]

Thanks!

[javagl]

In

three.js/examples/models/gltf/snowflake/line_vert.glsl

Line 12 in abf63b0

vec4 pos = u_modelViewMatrix * vec4(position,1.0);

it says

vec4 pos = u_modelViewMatrix * vec4(position,1.0);

but should be

vec4 pos = u_modelViewMatrix * vec4(a_position,1.0);

(The variable is now called a_position)

[bdysvik]

Obviously..
thanks