Common Materials Extension

[tparisi]

https://github.com/KhronosGroup/glTF/tree/KHR_materials_common/extensions/Khronos/KHR_materials_common

Comments:

• The JSON examples are inconsistent. Both 1 and 1.0 are used. Arrays are sometimes on the same line and sometimes one element per line.
• index_of_refraction -> indexOfRefraction. glTF uses camel case for property names.
  • Same for constant_attenuation, linear_attenuation, falloff_angle, and falloff_exponent.
• Are we sure CONSTANT needs all those properties? How is each used?
• FLOAT_VEC4 or SAMPLER_2D -> In glTF, what is sampler 2D? Shouldn't this be a string (texture id)? We need to precisely define where the texture coordinates come from, e.g., if both diffuse and specular are texures, do they use the same or different set of texture coordinates?
• technique - why does this have a default? Perhaps it should be required. We should also make this separate from the table and say the table is for the values object.
• "relective": "texture_envmap" - glTF does not support cube maps.
• This needs to include the equations.
• This needs to include some discussion on how vertex attributes use semantics to map material inputs, e.g., normals, texture coordinates, etc.

To-do: Patrick need schema for each of the material types... can you help?

Maybe, but I did to finish the core spec reference doc first. Will let you know later this week.

[tfili]

@tparisi Also, I also don't think we need distance. That is the point of attenuation. You get the distance from the fragment to the light and using the attenuation values you compute the light's contribution to the scene.

[tparisi]

Indeed. I just forgot to remove it. On it;

[tparisi]

Fixed distance and added some language describing how direction and position are calculated (or ignored) per- light type. b03b4b8

[pjcozzi]

I think we have the following issues left to finalize this extension:

• [x] Clean up Blinn section - https://github.com/KhronosGroup/glTF/issues/424#issuecomment-148722225
• [ ] Expand parameter values - https://github.com/KhronosGroup/glTF/issues/424#issuecomment-148774205
• [ ] List attributes (could be part of the above) - https://github.com/KhronosGroup/glTF/issues/424#issuecomment-148762276

@tparisi @tfili can you confirm

[tparisi]

confirmed, hope to get to this today

[pjcozzi]

@tparisi any movement here? We want to finalize our implementation (both COLLADA2GLTF and Cesium).

[tparisi]

@pjcozzi Need some input:

• [ ] I think we should just remove Blinn and have Phong (which is typically implement Blinn-Phong anyway), Lambert and Constant.
• [ ] I have no idea what you want with these other two issues, you're going to have to explain them to me via voice...

[tparisi]

@pjcozzi ok taking another look at Blinn and clearing up the language.

[tparisi]

@pjcozzi Simplified Blinn language, I think this is right. 12910f7

[pjcozzi]

@pjcozzi Simplified Blinn language, I think this is right. 12910f7

This is perfect!

[tparisi]

@pjcozzi Great. Checked that one off the list.

On to the final two issues. I'm still not convinced. Please pow-wow with @tfili

[pjcozzi]

• [ ] Expand parameter values - https://github.com/KhronosGroup/glTF/issues/424#issuecomment-148774205

I still agree with this. For example, this:

 "KHR_materials_common" : {
  "technique" : "PHONG",
  "values": {
    "diffuse": [ 1, 1, 0, 1]
  }
}

would become:

 "KHR_materials_common" : {
  "technique" : "PHONG",
  "values": {
    "diffuse": {
      "value": [ 1, 1, 0, 1],
      "type": 35666 // FLOAT_VEC4
    }
  }
}

This avoids the implicit type checks in the code example above, which we do not rely on anywhere else in glTF (everything has an explicit type). I'm surprised you haven't ran into this in Three.js. For example, how does the code know the difference between a constant diffuse value and a diffuse texture?

I would also consider renaming values to parameters since this has much better symmetry with techniques than materials, which is OK. The older details approach still had a technique - now we do not.

[pjcozzi]

@tfili for this one:

List attributes (could be part of the above) - #424 (comment)

Instead of explicitly listing the attributes as I suggested or searching the meshes/primitives as our implementation does now, could the extension spec specify that:

• CONSTANT lighting requires only POSITION attributes.
• LAMBERT, BLINN, and PHONG require POSITION and NORMAL attributes.

In any case where a parameter is a string (e.g., a texture), then TEXCOORD_0 attribute is also required. Note that this could even happen for CONSTANT since emission can be a texture.

Given this (and ignoring skinning), couldn't the shaders be generated?

As for skinning, perhaps we could add a jointCount property to the material, which is zero for no skinning, and non-zero with skinning?

[tparisi]

@pjcozzi Adding a type sounds reasonable and it's not too onerous... but I still disagree in principle. If an implementation supports this extension, it should know what types to expect for each values and it should have built-in shaders to handle it.

Disagree about renaming values, those are values they're not parameter descriptions. We have values when techniques are instanced in the material, so the current proposal is symmetric to that.

I'm OK with the language about attributes.

Why do you need a jointCount?

[pjcozzi]

it should know what types to expect for each values and it should have built-in shaders to handle it.

Yes, but it needs to know the type. The current approach is clunky, look at the code: https://github.com/KhronosGroup/glTF/issues/424#issuecomment-148774205

I'm OK with the language about attributes.

OK, let's wait for @tfili to confirm that it will work.

Why do you need a jointCount?

So the vertex shader can declare the maximum size of the attribute array.

[tparisi]

I don't think having to do a typeof check is that big of a deal.

For the transparent idea, why not just have a transparent boolean on the material itself rather than a property of the texture? This would be separate from the transparency value, just a flag that indicates to the engine to turn on alpha blending and alpha-sort the object.

[pjcozzi]

I don't think having to do a typeof check is that big of a deal.

As I said above, it is not consistent with the rest of glTF - and is, well, a bit hacky.

For the transparent idea, why not just have a transparent boolean on the material itself rather than a property of the texture?

Might be OK. @tfili would this work?

[tfili]

@pjcozzi @tparisi Actually that may be better, since I don't care what texture has transparency, I just need to set the blend state correctly.

[tparisi]

@tfili right and that's what you see in a lot of engines, a transparent flag on the material itself. very simple so let's do that. It's separate from the transparency flag which is the actual value to multiply the alpha by when you have RGB's.

@pjcozzi agreed, hacky. but balance this against verbosity of the JSON. that being said your latest https://github.com/KhronosGroup/glTF/issues/424#issuecomment-150708983 is a pretty concise way to do it.

[pjcozzi]

@tfili right and that's what you see in a lot of engines, a transparent flag on the material itself.

Let's do it.

@pjcozzi agreed, hacky. but balance this against verbosity of the JSON. that being said your latest #424 (comment) is a pretty concise way to do it.

I agree it is reasonable. I suggest we do it and then I think we are good to go with this extension (other than finishing our implementations and the schema).

[tparisi]

@pjcozzi OK you won me over. I'll start implementing when @tfili gives the go-ahead on converter and converted content.

[tfili]

Ok, that works for me. I'm working on the implementation now.

[tfili]

I've finished the converter and Cesium implementations. The things that should be updated in the spec are:

• Added doubleSided to the material (this was discussed and implemented before but never put in the spec).
• Added transparent to the material.
• Made all values an object with the properties type & value

@tparisi @pjcozzi Is there anything else?

[pjcozzi]

Nothing else from me.

[tparisi]

Hi @tfili good catches.

Do you or @pjcozzi definitively know the render states that go with both of these flags? If so I'll also add that language. Pretty sure it's CULL_FACE disabled for doubleSided, what about for transparency? I imagine it's a combo, please advise on spec language...

[pjcozzi]

@tparisi see @tfili maps these to render states here: https://github.com/AnalyticalGraphicsInc/cesium/pull/3039/files#diff-8fd2167b1c1a0711731254f9aa4b45c9R532

[tparisi]

@tfili @pjcozzi LMK when we have the models converted and I'll start on the Three.js loader. Are we doing it in master or the materials branch?

[pjcozzi]

OK, we'll open a pull request into master with the models. The issue is #448.

[tparisi]

I think the PR should be off the materials branch and then we'll merge back into master once everything is sorted out with the Three.js importer

[pjcozzi]

OK, we'll target the pull request to the materials branch.

[tfili]

PR in #468

[tparisi]

Just checked in language for doubleSided and transparent please review @pjcozzi

[pjcozzi]

transparency update looks good. doubleSided needs to mention to flip the normal for backfacing triangles. For example, here is @tfili's implementation:

            if (khrMaterialsCommon.doubleSided) {
                fragmentShader += '  if (gl_FrontFacing == false)\n';
                fragmentShader += '  {\n';
                fragmentShader += '    normal = -normal;\n';
                fragmentShader += '  }\n';
            }

[pjcozzi]

To finish this, I believe there are the following outstanding issues:

• [ ] Remove type, #523
  • [x] Update extension spec
  • [ ] Update COLLADA2GLTF
• [ ] Specular exponent update, #510
• [ ] Add schema to spec
• [ ] Semantics/names, #509

@tparisi @tfili @mlimper is there anything else?

[mlimper]

I think that's it

[tparisi]

We can close #510 and #509 - but #523 are we sure about removing type? see my comment

On Tue, Jan 12, 2016 at 1:36 AM, Max Limper notifications@github.com wrote:

I think that's it

— Reply to this email directly or view it on GitHub https://github.com/KhronosGroup/glTF/issues/424#issuecomment-170851864.

Tony Parisi tparisi@gmail.com Follow me on Twitter! http://twitter.com/auradeluxe Read my blog at http://www.tonyparisi.com/ Learn WebGL http://learningwebgl.com/ Mobile 415.902.8002 Skype auradeluxe

Read my books! Learning Virtual Reality http://www.amazon.com/Learning-Virtual-Reality-Experiences-Applications/dp/1491922834

Programming 3D Applications in HTML5 and WebGLhttp://www.amazon.com/Programming-Applications-HTML5-WebGL-Visualization/dp/1449362966 http://www.amazon.com/Programming-Applications-HTML5-WebGL-Visualization/dp/1449362966WebGL, Up and Running http://www.amazon.com/dp/144932357X

[mlimper]

If it would be updated in the examples, it would facilitate implementation and increase compatibility (Basically, we are staying with your initial proposal https://github.com/KhronosGroup/glTF/issues/424#issuecomment-150713235)

[pjcozzi]

We are going to update the samples, see https://github.com/KhronosGroup/glTF/issues/523#issuecomment-171015245

[yyc-git]

Hello! Does KHR_materials_common support normal map? I don't see the field in KHR_materials_common. Thank you~

[tfili]

It doesn't. It's meant to mimic Collada's common profile that supports basic lighting models (constant, lambert, blinn and phong). For more advanced techniques you'll have to provide shaders.

[yyc-git]

Thanks for your reply.

For more advanced techniques you'll have to provide shaders.

provide shaders? You mean that I should use the shader generated by gltf converter? Or How can I provide shader?

In my case, I just want to use my shader generated by my engine instead of using the shader provided by gltf converter. My shader supports normal map. So How can I read normap map data from .gltf file? Could you give me some suggestions? Thanks.

[tparisi]

Hi YYC

You will need to create your own extension. Info here:

https://github.com/KhronosGroup/glTF/blob/master/extensions/README.md

Yours would be a "vendor extension."

I can envision that we would someday add normal maps to the common materials extension. So maybe you can think about designing your extension for eventual inclusion in a future version of the KHR_ extension. We can't make about promises about that right now, but please keep the idea in mind while you are designing your feature and let's talk about it soon...

[yyc-git]

OK, I get it, thanks a lot~

[pjcozzi]

@McNopper OK to close as duplicate with #947 and #945?

[pjcozzi]

I believe this it is OK to close this as now duplicate with #1163 and friends.

@donmccurdy please let me know if you think otherwise.