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

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

Closed
wants to merge 1 commit into from

Conversation

emackey
Copy link
Member

@emackey emackey commented Feb 27, 2020

No description provided.

@emackey
Copy link
Member Author

emackey commented Feb 27, 2020

To be clear, these extensions are allowed to appear together in the same file, just not on the same glTF material.

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

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.

@pjcozzi
Copy link
Member

pjcozzi commented Mar 4, 2020

@emackey any update here?

@emackey
Copy link
Member Author

emackey commented Mar 4, 2020

I'm not certain this PR is needed. The unlit extension in master already has a section on fallback and compatibility behavior, so it may be fine as-is. What are we trying to change here?

@donmccurdy donmccurdy added this to the PBR Next milestone Apr 28, 2020
@emackey
Copy link
Member Author

emackey commented Jun 11, 2020

Closing for now. We may need a better strategy as more of PBR Next is fleshed out.

@emackey emackey closed this Jun 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants