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.

Sign up for GitHub

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

Add matching "Exclusions" sections to spec/gloss and unlit. #1772

Closed
wants to merge 1 commit into from
Closed

Add matching "Exclusions" sections to spec/gloss and unlit. #1772

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions extensions/2.0/Khronos/KHR_materials_clearcoat/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,10 @@ Written against the glTF 2.0 spec.

## Exclusions

* This extension must not be used on a material that also uses `KHR_materials_pbrSpecularGlossiness`.
* This extension must not be used on a material that also uses `KHR_materials_unlit`.
This extension must not be used on a material that also uses any of the following extensions:

- `KHR_materials_pbrSpecularGlossiness`
- `KHR_materials_unlit`

## Overview

Expand Down
10 changes: 9 additions & 1 deletion extensions/2.0/Khronos/KHR_materials_pbrSpecularGlossiness/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
# KHR\_materials\_pbrSpecularGlossiness

## Khronos 3D Formats Working Group
## Contributors

* Saurabh Bhatia, Microsoft [@iamsbtron](https://twitter.com/iamsbtron)
* Gary Hsu, Microsoft [@bghgary](https://twitter.com/bghgary)
* Patrick Cozzi, Cesium, [@pjcozzi](https://twitter.com/pjcozzi)
Expand All @@ -22,6 +23,13 @@ Complete

Written against the glTF 2.0 spec.

## Exclusions

This extension must not be used on a material that also uses any of the following extensions:

- `KHR_materials_clearcoat`
- `KHR_materials_unlit`
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure we discussed adding exclusions between KHR_materials_unlit and KHR_materials_pbrSpecularGlossiness recently. I think when we first added KHR_materials_unlit, we said it was okay to have both on the same material and engines can choose to use either one for fallbacks.

Copy link
Member Author

Choose a reason for hiding this comment

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

The bottom of the unlit extension describes fallback behavior. Pairing with spec/gloss is already called out as being undefined.

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

  • metallicFactor is 0 and emissiveFactor is [0, 0, 0].
  • roughnessFactor is greater than 0.5.
  • Omit metallicRoughnessTexture, occlusionTexture, emissiveTexture,
    and normalTexture.

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

Copy link
Member

Choose a reason for hiding this comment

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

I think the plan was to declare SpecGloss and Unlit incompatible with all other material extensions by default, unless explicitly allowed. We should not update these two extensions for every PBR Next extension.

Copy link
Member Author

Choose a reason for hiding this comment

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

I see. Unlit already has that as an implementation note at the bottom, perhaps that note should be copied to spec/gloss?

Copy link
Member

Choose a reason for hiding this comment

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

I'd suggest changing "undefined" to something more concrete. If we cannot directly disallow that, another option could be to say that incompatible material extensions have fixed priorities.

For example, Unlit always overrides everything else so the result is not undefined but an asset would still produce a validation warning.

Copy link
Contributor

Choose a reason for hiding this comment

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

The intention behind "always overrides everything else" seems equally applicable to Unlit and SpecGloss, and putting such a notice on both would seem confusing to me...

Perhaps something like:

The KHR_materials_<...> extension alters the shading model of only core glTF metal/rough PBR materials. If applied to a material alongside another extension, it is considered an alternative to — not an addition to — that extension, unless otherwise specified. Choice among alternate material extensions is up to the client implementation, based on device capabilities and the intended visual effect.

Copy link
Member

Choose a reason for hiding this comment

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

it is considered an alternative to — not an addition

How should client implementations treat a material that have: core MR, clearcoat, SpecGloss, and Unlit extensions defined? Can we declare that strictly invalid?

Copy link
Contributor

@donmccurdy donmccurdy Feb 28, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I would interpret that as a mutually exclusive choice between:

a. core + KHR_materials_clearcoat
b. KHR_materials_pbrSpecularGlossiness
c. KHR_materials_unlit

I agree it's a nonsensical combination, but I don't see any generalizable rule to prohibit it without also ruling out more reasonable combinations:

a. NINTENDO_materials_toon
b. KHR_materials_unlit (fallback)
c. core (last resort)

Or we could maintain a matrix of exactly which material extensions are allowed alongside other extensions? That seems easier than maintaining pairwise compatibility lists in each spec.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure about toon falling back to unlit falling back to core, I think that's a bit much. I think all material extensions should fall back directly to core, without going down a prioritized list.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't mean to suggest a prioritized list, per the "choice among alternate material extensions is up to the client implementation" language above. I do mean to say that Unlit shouldn't pre-emptively prohibit use with extensions that don't exist yet. Whether SpecGloss and Unlit (as a pair) are specifically prohibited, seems less important than the general guideline.


## Overview

This extension defines the specular-glossiness material model from Physically-Based Rendering (PBR). This extensions allows glTF to support this additional workflow.
Expand Down
7 changes: 7 additions & 0 deletions extensions/2.0/Khronos/KHR_materials_unlit/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,13 @@ Complete

Written against the glTF 2.0 spec.

## Exclusions

This extension must not be used on a material that also uses any of the following extensions:

- `KHR_materials_clearcoat`
- `KHR_materials_pbrSpecularGlossiness`

## Overview

This extension defines an unlit shading model for use in glTF 2.0
Expand Down