[KMC-extension] reworked intro, equation, shininess note

[andreasplesch]

This PR is also available as https://github.com/UX3D-nopper/glTF/pull/1 since it was based on the UX3D-nopper fork. However, the suggestion was to provide the PR against KhronosGroup/glTF .

Please feel free to edit freely or ignore all suggestions and just use as input for discussion on phrasing, language and content.

[pjcozzi]

Fixes #945.

[McNopper]

-1

This extension should become KHR_cmn_lights extension, as it does not fit well to PBR. For common materials, we can take it 1:1.

[andreasplesch]

Would it work better to complete this extension in the https://github.com/UX3D-nopper/glTF/ fork to avoid confusion ?

[McNopper]

Basically, the extension is fine for for the common materials. But for PBR materials, we need too many exceptions, that I will suggest a separate one. Sorry for the confusion.

[warkid]

typo: I guess "positional" is supposed to be a dictionary? should be curly brackets then not square...

[donmccurdy]

This extension should become KHR_cmn_lights extension, as it does not fit well to PBR

In the interest of expediting a material model that runs more efficiently on mobile devices, could we consider splitting this PR in two, and finishing up KHR_cmnBlinnPhong before we tackle lighting? Anecdotally, most users are using glTF for individual models, so I see blinn-phong as the more important (and perhaps less complex?) extension here.

[donmccurdy]

I assume this means specular is RGB channels, and shininess is A. We've discussed a bit in the past, and I don't remember where we ended up on this question, but there was some concern that because using RGB+A requires PNG, transmission size might actually end up worse than if two JPGs had been used (e.g. RGB + R).

[donmccurdy]

Generally the blinn-phong proposal looks good to me.

One note, three.js and A-Frame can currently support shininessFactor but not shininessTexture. There is some ongoing discussion about whether to change that.

[donmccurdy]

s/specular-glossiness/Blinn-Phong

[donmccurdy]

where is <ambient> defined?

[stevepg]

on one of the other threads i did suggest adding the following (it is submitted as a PR against what i suspect is now a dead fork)

diffuseTerm = diffuseFactor * diffuseTexture
specularTerm = specularFactor * specularShininessTexture.rgb
shineTerm = shininessFactor * specularShininessTexture.a
emissiveTerm = emissiveFactor * emissiveTexture // core Materials spec
occlusionTerm = occlusionFactor * occlusionTexture // core Materials spec

color = emissiveTerm + occlusionTerm * (diffuseTerm * max(N * L, 0) + specularTerm * max(H * N, 0)^shineTerm)

where
N – Normal vector
L – Light vector
H – Half-angle vector,calculated as halfway between the unit Eye and Light vectors, using the equation H= normalize(I+L)

Textures are optional, and if provided, will modulate the base Factor value. Note that specularShininessTexture contains the specular modulation as the rgb components and shininess modulation as the alpha component.

[zellski]

If I'm reading correctly, the discussion in #947 ended with:

• The absence of a term is a signal to the engine to downgrade to a shader that doesn't understand that term; e.g. no specular term means (at best) diffusive & ambient/emissive; no diffusive term means ambient/emissive, or constant.
• We've actually scrapped the ambient factor (which I think is good.)

The logical conclusion seems to be that a constant material is flagged as such by a completely empty cmnBlinnPhong extension? In other words, just a somewhat wordy boolean opt-in.

Was the explicit type really problematic? I'm all for simplified APIs, but I thought the type added clarity rather than complexity.

Of course it could be argued that we should then have a cmnLambert and a cmnConstant, with correspondingly tighter schemas -- indeed, like what @donmccurdy just suggested in #1095.

[donmccurdy]

Thinking about this more, I am also worried about the wording:

This extension defines a single material type, Blinn-Phong. Other or simpler materials types such as Lambert or Constant can be described by having zero factors.

This implies the correct way to specify a Constant material would be:

{
  "name": "RedConstant",
  "emissiveFactor": [1, 0, 0],
  "extensions": {
    "KHR_materials_cmnBlinnPhong": {
      "diffuseFactor": [0, 0, 0, 0],
      "specularFactor": [0, 0, 0],
      "shininessFactor": 0
    }
  }
}

This doesn't feel very natural. The Constant material is identified by the presence of a blinn-phong extension, even though no factors from the blinn-phong extension are actually used.

Also, we may someday want to allow animation of materials (say, opacity or color). In that case, you could define an initial diffuse state of [1,0,0,1] and animate it to [0,0,1,1]. But an initial state of [0,0,0,0] would imply an entirely different shading model, and introduces an edge case for loaders.

I think we should probably be more explicit about the intended shading model, rather than having things implicitly in cmnBlinnPhong. By comparison, a "KHR_materials_cmnConstant": {} extension with no parameters (simplify defining a new shading model with a subset of parameters already available in the core pbr material) would be clearer.

---

EDIT: So to be specific, I would recommend that we do one of two things:

A. Rename this extension KHR_materials_common and include an explicit "type": BLINNPHONG | LAMBERT | CONSTANT parameter.
B. Remove the suggestion that renderers infer the shading model from parameter values, and create separate KHR_materials_cmnLambert and KHR_materials_cmnConstant extensions, each specifying a shading model.

[javagl]

Some of the questions here seem to be related to what I asked about in #947 (comment) (and I hope that what I wrote there was not considered as a "proposal", considering the disclaimer: I don't know which performance implications this might have).

But the point mentioned above, about the animations, is IMHO an important conceptual one: If the official statement will be that zero factors imply a different shading model, then loaders/viewers might be tempted to use (pseudo) code like

if (material.everything == 0) loadConstantShader();
else if (material.something == 0) loadLambertShader();
else loadBlinnPhongShader();

(The goal of this would be (performance) optimizations, of course. I still wonder whether there will really be noticable performance differences between these imlementations, considering that even the most complex shader here is comparatively simple. But regardless of that: )

If some material value changed afterwards (be it through animations or something like a "material editor" UI component), then this would cause problems.

I'm not so deeply involved here to make any proposals, but think that it might be possible to cover both directions with implementation notes:

• For different material types, one could mention: "For simplicity, viewers may choose to implement all these types with a single blinn/phong shader, where they only zero-out the unused properties to emulate the lambert/constant case"
• For a single material type, one could mention: ~"Even though some values might be 0 at the beginning, the viewer/shader has to anticipate that these values might change to non-zero values at runtime"

But this is just a vague idea.

[steveghee]

i much prefer the idea of each extension being specific to its function e.g. material_blinnphong does only that. If you want a _constant material, then define another material type.

[donmccurdy]

I'm not so deeply involved here to make any proposals, but think that it might be possible to cover both directions with implementation notes:

• For different material types, one could mention: "For simplicity, viewers may choose to implement all these types with a single blinn/phong shader, where they only zero-out the unused properties to emulate the lambert/constant case"
• For a single material type, one could mention: ~"Even though some values might be 0 at the beginning, the viewer/shader has to anticipate that these values might change to non-zero values at runtime"

Yes, these notes would clear up any ambiguity. The downside of the second is that (unless we add an explicit indication of shading model) there is then no option to switch to something cheaper.

I still wonder whether there will really be noticable performance differences between these imlementations, considering that even the most complex shader here is comparatively simple.

I'm asking around, and while all of these shaders are comparatively simple, there are implementation differences that make a bigger difference (at least in three.js).

1. In the three.js Phong material, lighting is computed per-fragment unless material.lighting is turned off. But for the same reason as we can't switch to a Constant material based on zero parameters, it's probably not safe for the loader to turn lighting off without some explicit indication in the asset.
2. In the three.js Lambert material, lighting is computed per-vertex. This is an implementation detail, but means that devs may want to target Lambert for cheaper lighting. As a point of reference, Daydream provides a custom Unity renderer which (among other things) uses per-vertex lighting instead of per-fragment.
3. In the three.js Constant material, lighting is never computed.

For the reasons above, devs will have reasons to target a specific material type. I think it's fine for engines to have Blinn-Phong and use that in all cases (let's include this implementation note), but I don't think we should omit any indication of the shading model from the format, for implementations that need to distinguish between shading models.

i much prefer the idea of each extension being specific to its function e.g. material_blinnphong does only that. If you want a _constant material, then define another material type.

I'm fine with this, or with a single extension that is explicit about the shading model. Note that if split out, constant material would just be a boolean flag like KHR_materials_cmnConstant: true or KHR_materials_cmnConstant: {}.

[zellski]

I will just add a note here, redundant perhaps, but for emphasis: that the relationship between performance and shader choice is far from predictable, as it passes through the domain of aesthetic consideration and art style. I think this extension should carefully avoid accidentally encoding any assumptions about how an application might choose to downgrade shader complexity; it should simply retain as much expressive power as possible.

To take our case as an example: we decided early on that the user base we're hoping to attract early on is going to be largely on mid-grade android devices. In VR, reduced frame rate is not a reasonable path for graceful adaptation -- only scene complexity is. So we embraced in all our art a low-poly, unlit, flat-shaded style. This seems like exactly the kind of deliberate decision that you would want to encode explicitly in the asset file, not leave it as an optional optimization path to the loader/viewer.

[andreasplesch]

Although I fear that I am one of the least suitable persons to keep editing this PR, I went ahead and removed the lights extension language as it was included here by accident and adopted most (all?) of the specific suggestions made. Apologies to those I overlooked.
This PR should be seen solely as a way to continue to evolve this extension. I would love if a more responsive editor would take this draft over, in a new PR or fork, since I will only be able to contribute very sporadically and slowly.

[donmccurdy]

I think I would just omit the second two sentences, so that this paragraph becomes:

Given the long history of the Blinn-Phong model and the vast amount of collective experience with applying this model, the demand for convenient use of such materials is still exists. This extension defines a single material type, Blinn-Phong.

(Mentioning that simpler materials can be emulated via Blinn-Phong can be mentioned in the extensions for those materials)

[donmccurdy]

@andreasplesch thanks for helping to move this forward! 🙂 Your changes look good to me; I just added one additional suggestion.

There's already a KHR_materials_common draft in the repo, so I would vote to go ahead and merge this PR, then in a new PR we can iterate on whichever we decide to use. I'll update #1095 to discuss the unlit/constant material.

[andreasplesch]

I could also resubmit this PR against another branch if that would be preferable.

[donmccurdy]

I could also resubmit this PR against another branch if that would be preferable.

@andreasplesch Thanks, that might be best — could you resubmit this against a gltf2-common-materials branch?

[andreasplesch]

Closing this PR after resubmitting to as #1111 against the gltf2-common-materials branch. Happy to reopen here for any reason.