Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

KHR_materials_unlit #1163

Merged

Conversation

donmccurdy
Copy link
Contributor

@donmccurdy donmccurdy commented Nov 24, 2017

Updated version of #1095 as alternative to #1150 (KHR_materials_common).

@lexaknyazev
Copy link
Member

@donmccurdy
baseColorFactor and friends are defined within pbrMetallicRoughness object. Please update "compatibility" section to reflect that.

The material is intended for unlit shading. Color is calculated as:

```
color = <emissiveTerm> + <occlusionTerm> * <aL>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should an unlit mode include indirect light? How indirect does the light need to be before it gets included?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't think of any reason to diverge from the glTF 1.0 extension here, https://github.com/KhronosGroup/glTF/tree/master/extensions/Khronos/KHR_materials_common#constant

* [*From core glTF material specification.*](https://github.com/KhronosGroup/glTF/tree/master/specification/2.0#additional-maps)
* `occlusionTerm` – occlusionFactor * occlusionTexture
* [*From core glTF material specification.*](https://github.com/KhronosGroup/glTF/tree/master/specification/2.0#additional-maps)
* `aL` – Ambient light
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The glTF core spec refers to 'indirect light' and not 'ambient light' when the occlusion term is mentioned.

It's pretty vague about what indirect light means, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The distinction between ambient and direct lights is fairly well defined in most engines, yes? If there are notable exceptions to that, I would think we can leave this to interpretation.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking about engines that use pseudo-directional lighting such as a hemispherical fill or environmental cube map instead of a flat ambient light. They can just decide on their own interpretation.

### Extension compatibility and fallback materials

When possible, authoring tools should provide a fallback material definition
and mark the `KHR_materials_cmnBlinnPhong` extension as optional. By doing so,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should reference KHR_materials_cmnConstant and not BlinnPhong.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"emissiveTexture": { "index": 0 },
"baseColorFactor": [ 0.0, 0.0, 0.0, 1.0 ],
"metallicFactor": 0.0,
"roughnessFactor": 0.0,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

roughnessFactor of 0.0 gives a perfectly shiny surface, which is generally not a good default value.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and mark the `KHR_materials_cmnBlinnPhong` extension as optional. By doing so,
models created with the extension will still render correctly in all clients
that support the core glTF 2.0 specification, although clients are less likely
to optimize their shading model performance in this case.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sentence should be rewritten for Constant instead of BlinnPhong. The fallback material will probably have a different visual look for Constant, as well as worse performance.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


## Extending Materials

The common Constant material is defined by adding the `KHR_materials_cmnConstant` extension to any glTF material. When present, the extension indicates that a material should use the Constant shading model. Unrelated default values in the default PBR shading model, such as `diffuseFactor`, should be ignored.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'diffuseFactor' is called 'baseColorFactor'

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@snagy
Copy link
Contributor

snagy commented Dec 11, 2017

Pulling over a conversation from #1150 on whether the unlit values should be in base color or emissive.

  1. Putting it in emissive will instantly work in almost all the current gltf renderers (with a little specular highlighting in engines that don't support the extension). However, it probably won't really work correctly with a renderer that actually emits light from the emissive channel, and it can't be defined in vertex data.
  2. Putting it in base color will kinda work in renderers that don't support the extension; but, the visual look will diverge a lot more between renderers that are extension-aware and those that aren't.
  3. Defining it in a separate property is clean and lets the exporting program decide how non-supporting renderers will work. My main concerns with this were intent (what should a renderer do if they encounter both? is unlit an optimization or the preferred look?), and whether it actually gets us a win in most situations. Renderers still need to be aware of the extension to handle things properly, and instead of coming to a general consensus about the preferred way to handle the drawbacks of the above two cases, we're leaving it to the exporters to figure out their own solution and thus potentially making it more difficult for future renderers to consistently interpret content from different sources.

I lean towards base color, with the assumption that most renderers will support this extension, and base gives us unlit vertex colors.

@zellski
Copy link
Contributor

zellski commented Dec 11, 2017

The requirement that vertex colour functionality has a reasonable fallback seems to me to tower above the other considerations here -- so I am in favour of option 2 as well. The low-poly vertex-colour aesthetic is one of the primary use cases for an unlit extension, aye?

@donmccurdy
Copy link
Contributor Author

Updated and applied feedback, including use of base color — thanks all.

I've added an explicit note on vertex colors as well.

Unrelated default values in the default PBR shading model, such as
`roughnessFactor`, should be ignored.

```
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

put ```json here for syntax coloring

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"pbrMetallicRoughness": {
"baseColorFactor": [ 1.0, 1.0, 1.0, 1.0 ],
"baseColorTexture": { "index": 0 },
"roughnessFactor": 1.0
Copy link
Contributor

@bghgary bghgary Dec 11, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

metallicFactor defaults to 1.0 and having a fully rough and fully metal material is not typical. Might consider setting metallicFactor to 0.0.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

* **baseColorTerm** – `baseColorFactor` * `baseColorTexture` * `vertexColor`
* [*From core glTF material specification.*](https://github.com/KhronosGroup/glTF/blob/master/specification/2.0/README.md#metallic-roughness-material)
* **aL** – Indirect light
* *Constant sum of all ambient light contributions from the scene.*
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we're saying that we're expecting baseFactor/baseTexture/vertexColor to define our unlit material (while tolerating emissive), then we shouldn't be multiplying it by aL, right? Unless the term's meaning here is meant to be entirely implicit.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I second this, I don't think occlusion or ambient light should be part of this spec.

Copy link
Contributor Author

@donmccurdy donmccurdy Dec 11, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure I follow — are you asking whether an unlit material should ignore ambient light in addition to ignoring direct?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just saw @snagy's comment, the comment makes sense then —

On whether occlusion or ambient should be included, I'd point out that:

  1. We did include them in the constant material from glTF 1.0
  2. THREE.MeshBasicMaterial supports both
  3. Disallowing ambient light has no particular performance benefit.

Is there a benefit to omitting occlusion and ambient light?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It made sense to have an aL factor in the glTF 1.0 version because in that version we leaned on emissive to produce the unlit effect, and emissive is immune to scene light. That role is now to be played by baseColor & friends, instead. A material whose canonical visibility factor is multiplicatively attenuated by a light factor doesn't qualify as unlit, does it?

That is, unless we say that clients that do not have the concept of ambient light should assume a default value of 1.0... but that seems like a crazy can of worms...

The common Constant material is defined by adding the
`KHR_materials_cmnConstant` extension to any glTF material. When present, the
extension indicates that a material should use the Constant shading model.
Unrelated default values in the default PBR shading model, such as
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we be explicit about exactly which values should be ignored or is that something that is up to implementation?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added explicit comment on metallic and roughness.

I'm not sure what a client should do if it sees KHR_materials_cmnConstant and KHR_materials_pbrSpecularGlossiness. Inclined to leave that up for application choice.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about metallicRoughnessTexture? What about vertex attributes like NORMAL, TANGENT and normalTexture? Should these be ignored?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

KHR_materials_cmnConstant and KHR_materials_pbrSpecularGlossiness

Yeah, this one is a bit tricky. My gut feel is that this should not happen. If an asset has an unlit material, it probably wants to be fast, and I can't think of a good reason to have specular glossiness there also.

@donmccurdy donmccurdy changed the title KHR_materials_cmnConstant v2 KHR_materials_cmnConstant Dec 11, 2017
@donmccurdy
Copy link
Contributor Author

donmccurdy commented Dec 11, 2017

@snagy @zellski I'm moving this reply to a top-level comment for visibility —

I see what you mean; ambient in glTF 1.0 was a factor but did not allow a texture ID. In that case color should be:

color = <baseColorTerm>

And there's no point in differentiating direct vs indirect light, so that can be removed from the text as well. This seems consistent with Unreal, also.

A material whose canonical visibility factor is multiplicatively attenuated by a light factor doesn't qualify as unlit, does it? ~@zellski

Yeah that had bothered me, but "constant" still made sense with ambient light... after doing some web searches on "constant shading" I didn't find much, and what I found isn't consistent:

screen shot 2017-12-11 at 3 08 19 pm

^Essential computer animation fast

Here's a slide deck also using "constant shading" interchangeably with flat shading.

Another source writes "In the Constant shading model we compute one shade or color for the entire object, e.g., there really is no shading being done," which would still be true with ambient lighting applied.

In that case, perhaps we should rename this to something like KHR_materials_unlit?

"pbrMetallicRoughness": {
"baseColorFactor": [ 1.0, 1.0, 1.0, 1.0 ],
"baseColorTexture": { "index": 0 },
"metallicFactor": 0.0,
Copy link
Contributor

@bghgary bghgary Dec 11, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that I think about this more, I think we should note that the fallback should not have metallicRoughnessTexture and metallicFactor should be 0 and roughnessFactor should be 1. Same for normalTexture depending on what we decide for that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. Any thoughts on whether that should be normative, or simply an implementation note for recommended fallback behavior?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think since this is already in a fallback section it doesn't make sense to be an implementation note. Normative is my vote.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Following this through, does this look right?

  • metallicRoughnessTexture CANNOT be present

  • metallicFactor MUST be 0

  • roughnessFactor MUST be 1

  • occlusionTexture CANNOT be present

  • emissiveTexture CANNOT be present

  • emissiveFactor MUST be 0

  • normalTexture CANNOT be present

  • NORMAL and TANGENT attributes ...

    • cannot be present?
    • may be present, but must be ignored during lighting calculations?

Inclined to say NORMAL attribute should not be disallowed, but ignored in shading. Arguably it could be used for things other than shading e.g. for aligning a reticle against the surface. Same is arguably true of normalTexture though...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inclined to say NORMAL attribute should not be disallowed, but ignored in shading. Arguably it could be used for things other than shading e.g. for aligning a reticle against the surface. Same is arguably true of normalTexture though...

+1 for the NORMAL attribute. I don't think the same is true for the normalTexture. It would be weird if there is no visible changes for the normalTexture but something else (like a reticle or physics) did something with it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why should we dictate what the factors should be for the fallback case? If I want a metallic or nonmetallic fallback, or shiny or rough, or whatever, I should be able to define that, right?

If you handle the extension you ignore everything that's not part of the lighting calculation above, and if you don't (or don't wish to) use the extension, you can use the fallback values.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, huge +1 to renaming this KHR_materials_unlit

I never even considered that constant would mean anything but unlit, so maybe we should be explicit in the name and remove any references to 'constant'.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps this should be an implementation note then

@snagy
Copy link
Contributor

snagy commented Dec 13, 2017

Does anyone have expected use cases for this extension that they'd like to share? It might give us a little more clarity on exactly how we should suggest people handle things like fallback cases.

@msfeldstein
Copy link

We have two use cases we envision. The first is the most certain that we need.

  1. Things like photogrammetry, where the lighting of the world is baked in since it's being captured via a camera. These shouldn't have any extra lighting applied to it (unless we have some amazing method of removing world light).

  2. Models which have super high quality ray-traced SSS caustic etc lighting baked in

@zellski
Copy link
Contributor

zellski commented Dec 16, 2017

One of the projects I'm involved has the requirement that framerates stay high (in VR), even on weaker mobile devices. Rather than design for the high-end, and then be forced to implement progressively shittier fallbacks for old phones, we just rolled with the situation and opted into a low-poly, unlit vertex-colour art style from the beginning. And yet we've standardized completely around glTF 2.0, so I'm really eager for an extension that allows us to formally encode that art style decision in the model files, and have it be clear what it's intended to communicate: no dynamic light effects at all.

@donmccurdy donmccurdy changed the title KHR_materials_cmnConstant KHR_materials_unlit Dec 18, 2017
@donmccurdy
Copy link
Contributor Author

Updates:

  • Renamed as KHR_materials_unlit
  • Removed occlusion, emissive, and ambient light terms
  • Added implementation note on recommended fallback property values

"baseColorFactor": [ 1.0, 1.0, 1.0, 1.0 ],
"baseColorTexture": { "index": 0 },
"metallicFactor": 0.0,
"roughnessFactor": 1.0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

roughnessFactor defaults to 1.0 and thus is not needed. I believe it will be flagged as a warning in the validator. Same goes for baseColorFactor, but I would recommend changing the values so that they are not the default like in the "red_unlit_material" above.

Copy link
Contributor

@zellski zellski Dec 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree it would be better to choose a fallback example here that doesn't use values as extreme as roughness 1.0. The fallback materials shouldn't necessarily try to look as close to individually "unlit" as possible, right? Rather, we know the fallback materials are only used in the situation where we're in a fully lit, PBR environment. Better to accept and embrace that fact and pick a fallback material that looks attractively natural in such a setting. The artist has many other stylistic options available if she wants to accomodate the spirit of the original 'unlit' intent.

In that vein, while keeping it simple, perhaps drop roughness to 0.8, include a normal map?

Copy link
Contributor

@bghgary bghgary Jan 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe all the scenarios for an unlit material can be categorized into two buckets.

  1. The unlit material is the primary material and the core PBR material is the fallback. In this case, if the client implementation can render unlit, it should render with unlit. The fallback core PBR material should be authored to match as closely as possible to simulate what an unlit material would do.

  2. The core PBR material is the primary material and the unlit material is the fallback. In this case, if the client implementation can render core PBR metallic/roughness model, then it should render with PBR metallic/roughness model, even if there is an unlit extension. The fallback, likely due to runtime resources, is to render using unlit material as it is much cheaper to render. In this scenario, the core PBR material may have additional maps, various metallic/roughness values, etc. to support the higher fidelity material.

All of the scenarios that have been mentioned so far, including photogrammetry, seems to point to 1, but I think 2 is still possible and we shouldn't restrict someone from doing it.

Maybe we can incorporate this kind of language into this section? It would all be non-normative.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fallback core PBR material authored to match as closely as possible to simulate what an unlit material would do

To be clear, what I mean here is that the core PBR material should match what this PR currently already states:

Implementation Note: For best fallback behavior in clients that do not
implement the KHR_materials_unlit extension, authoring tools may use:

  • metallicFactor and emissiveFactor are 0.
  • roughnessFactor is 1.
  • Omit metallicRoughnessTexture, occlusionTexture, emissiveTexture,
    and normalTexture.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sounds good to me

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To add to this a bit more, in scenario 2, there is no reason to use the unlit extension at all. The client implementation can fallback to unlit if necessary on its own.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am content to withdraw my hesitation above re: roughness 1.0 vs 0.8 and so forth. It's fine for this spec to recommend a somewhat extreme fallback material.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed roughnessFactor=1.0 from the example, as that's the default value anyway.

Perhaps we just want to loosen the recommendation in the implementation note?

From:

roughnessFactor is 1.

to something like:

roughnessFactor is > 0.5

@Nehon
Copy link
Contributor

Nehon commented Dec 18, 2017

Does anyone have expected use cases for this extension that they'd like to share?

Sky boxes,
Could be useful for particles... sometimes
Could be useful for GUI.

@robertlong
Copy link
Contributor

robertlong commented Dec 21, 2017

Other than @bghgary's comment, looks good to me. Thanks Don!

I'll update my ThreeJS loader and FBX2glTF work

@amenzies
Copy link

amenzies commented Jan 2, 2018

I just want to second @msfeldstein's use case of Photogrammetry. In this case we often have textures which have been painstakingly constructed to accurately represent the object as it was lit in the real world at the time of capture. I suspect that there are a number of other cases in which an "unlit" material would be useful, such as achieving particular effects or for iconography.

Another case I can think of is tilesets which have been pre-rendered. For example, imagine you want to render the moon with accurate shadows. You often have low incident light creating shadows from tall mountains that can be quite far away. Real time shadows are impractical due to the distance and precision characteristics of the shadow caster. In this case you might choose to bake in pre-rendered shading and shadow into your dataset. I can also imagine tilesets that want to achieve a particular aesthetic, such as the 3D equivalent of street map tiles, that might want to have full control over their lighting.

It's such a dead simple material it would be a shame if it were impossible to achieve.

@donmccurdy
Copy link
Contributor Author

Implementation in Blender exporter:

KhronosGroup/glTF-Blender-Exporter#160

@lexaknyazev
Copy link
Member

@donmccurdy
Could the extension spec have an implementation note about other material extensions (i.e., SpecGloss)? Should such combination be disallowed or should unlit extension override all other material extensions?

@donmccurdy
Copy link
Contributor Author

donmccurdy commented Feb 21, 2018

I'm not sure how to reason about all other possible material extensions. No material extension exists today that would sensibly be used alongside KHR_materials_unlit, and without such an extension I wouldn't feel comfortable prohibiting use of KHR_materials_unlit with another material extension, or claiming that this should always take precedence... For the example of spec/gloss, specifically, I can't imagine why an author would include both, or what behavior they would expect.

Does it seem reasonable to leave this explicitly undefined? E.g.

When the KHR_materials_unlit is included with another extension specifying a shading model on the same material, the result is undefined.

@lexaknyazev
Copy link
Member

I'd say that explicitly undefined behavior is fine for now (/cc @pjcozzi).
My point is that JSON could contain anything and engines don't know anything about author's intentions, so they would have to make a decision which material model to use. Consider something like this:

if (material.specGloss)
    useSpecGloss()
else if (material.unlit)
    useUnlit()
else
    useMR()

@donmccurdy
Copy link
Contributor Author

Yes, makes sense. In the future I could imagine us doing something like specifying that order of precedence is based on order in extensionsUsed array, but it feels premature to decide now.

@bghgary
Copy link
Contributor

bghgary commented Feb 22, 2018

@donmccurdy

How about:

... ignoring all properties properties of the default PBR model related to lighting or color. Alpha coverage and doubleSided still apply to unlit materials.

If not this, could you propose the wording?

That's probably good for now. We can always update it if it is unclear.

@donmccurdy
Copy link
Contributor Author

Sounds good. Added language about alpha coverage, doubleSided, and undefined behavior when used alongside other material extensions.

"pbrMetallicRoughness": {
"baseColorFactor": [ 1.0, 1.0, 1.0, 1.0 ],
"baseColorTexture": { "index": 0 },
"metallicFactor": 0.0
Copy link

@fernandojsg fernandojsg Feb 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about adding "roughnessFactor": 0.5 as fallback value for the example as recommended on the implementation note below?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, using 0.9 here 👍

> **Implementation Note:** For best fallback behavior in clients that do not
> implement the `KHR_materials_unlit` extension, authoring tools may use:
>
> * `metallicFactor` and `emissiveFactor` are `0`.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe is clear enough already but maybe being even more explicit as emissiveFactor is a vector?: metallicFactor is 0 and emmisiveFactor is [0, 0, 0].

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done 👍

@lexaknyazev
Copy link
Member

Additional properties on the extension object are allowed, but may lead to undefined behaviour in conforming viewers.

For formal completeness, we should provide a simple JSON-Schema like this:

{
    "$schema": "http://json-schema.org/draft-04/schema",
    "title": "KHR_materials_unlit glTF extension",
    "type": "object",
    "description": "glTF extension that defines the unlit material model.",
    "allOf": [ { "$ref": "glTFProperty.schema.json" } ],
    "properties": {
        "extensions": { },
        "extras": { }
    }
}

@donmccurdy
Copy link
Contributor Author

For formal completeness, we should provide a simple JSON-Schema like this:

Done ✅

@lexaknyazev
Copy link
Member

Online validator has initial support for this extension. Please, drop some models into it.

@donmccurdy Is there any merit in warning on non-recommended fallback values?

@donmccurdy
Copy link
Contributor Author

Great thanks! Getting some errors in Firefox and Chrome, Cannot read property 'style' of null, though. I'd lean against warning about the fallback values, but curious what others think.

@lexaknyazev
Copy link
Member

lexaknyazev commented Feb 25, 2018

Getting some errors in Firefox and Chrome

Please force-reload web page.

@donmccurdy
Copy link
Contributor Author

Working now, thanks!

@fernandojsg
Copy link

I'd say also to avoid warning about the fallback, as with the current traction of engines implementing gltf I believe most of them would at least use the fallback values in their implementation itself and hopefully in a near future most of them will support the extension, so we don't need to waste space on the files for these values.

@@ -8,7 +8,7 @@

#### Draft Khronos extensions
_Draft Khronos extensions are not ratified yet._
* KHR_materials_common *(in progress)*
* [KHR_materials_unlit](2.0/Khronos/KHR_materials_unlit/README.md)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please move this to the non-draft section above. 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. ✅🎉

@donmccurdy donmccurdy force-pushed the feat-khr-materials-cmnConstant branch from 95318a2 to 1f5bbe7 Compare April 24, 2018 18:17
@donmccurdy donmccurdy force-pushed the feat-khr-materials-cmnConstant branch from 1f5bbe7 to 3462b74 Compare April 24, 2018 18:20
@pjcozzi pjcozzi merged commit 2fff547 into KhronosGroup:master Apr 24, 2018
@donmccurdy donmccurdy deleted the feat-khr-materials-cmnConstant branch March 7, 2019 23:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.