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

Finishing KHR_materials_common #632

Closed
13 tasks
lexaknyazev opened this issue Jun 25, 2016 · 36 comments
Closed
13 tasks

Finishing KHR_materials_common #632

lexaknyazev opened this issue Jun 25, 2016 · 36 comments

Comments

@lexaknyazev
Copy link
Member

lexaknyazev commented Jun 25, 2016

Main discussion was in #424.

Small fixes:


Since Extension hasn't been ratified yet, maybe it's possible to discuss a few more things:

  1. Should Extension define light's affection distance (to turn it off for all objects that are outside of affection range)?
  2. Should Extension define per-object light affection matrix (which lights affects which objects)?
  3. Should Extension support "Projector" lights with texture instead of a simple color?
  4. Maybe, we can allow light.color values over 1.0?
@lexaknyazev
Copy link
Member Author

Should Extension define light's affection distance (to turn it off for all objects that are outside of affection range)?

This can be easily done via node.boundingVolume (#507) later (1.1).

Should Extension define per-object light affection matrix (which lights affects which objects)?

Can be done later (1.1+) via something like node.layer.

Should Extension support "Projector" lights with texture instead of a simple color?

A separate extension should be proposed for such case to keep KHR_materials_common aligned with COLLADA.

@lexaknyazev
Copy link
Member Author

@pjcozzi @tparisi
Could you review #633?

@pjcozzi
Copy link
Member

pjcozzi commented Jun 29, 2016

@lexaknyazev I agree with your thoughts for doing things later in #632 (comment). Is there anything short-term here that you need input from @tparisi or me?

@lexaknyazev
Copy link
Member Author

Is there anything short-term here that you need input from @tparisi or me?

This one (btw, COLLADA doesn't have such limits):

Maybe, we can allow light.color values over 1.0?

There are several still unaddressed small fixes (regarding jointCount, transparent, doubleSided properties, language for CONSTANT shading and missing materials JSONs).

@pjcozzi
Copy link
Member

pjcozzi commented Jun 29, 2016

Maybe, we can allow light.color values over 1.0?

I am OK with this. @tfili do you remember if this was discussed at all for KHR_materials_common?

There are several still unaddressed small fixes (regarding jointCount, transparent, doubleSided properties, language for CONSTANT shading and missing materials JSONs).

Are these all in #633? If not, do some need more discussion here?

@lexaknyazev
Copy link
Member Author

Are these all in #633?

They aren't. I can make one more PR.
Comments from the extension contributors would be appreciated.

@pjcozzi
Copy link
Member

pjcozzi commented Jun 29, 2016

I can make one more PR

Yes, please.

@lexaknyazev
Copy link
Member Author

Just to be clear:

  1. jointCount, transparent and doubleSided must be in the extension object, not in values.
  2. CONSTANT shading definition matches COLLADA (almost 1:1). Maybe, we should keep it intact.

COLLADA has also transparent color (or texture) property and reflection-related props for all common materials:
image

@pjcozzi
Copy link
Member

pjcozzi commented Jul 4, 2016

(1) OK and (2) up to you.

@tfili
Copy link

tfili commented Jul 5, 2016

I am OK with this. @tfili do you remember if this was discussed at all for KHR_materials_common?

@pjcozzi I don't remember this being discussed. I'm fine with it.

@pjcozzi
Copy link
Member

pjcozzi commented Jul 6, 2016

@tparisi can you please do a final review of this and #633?

@lexaknyazev is there anything else you are waiting on?

@lexaknyazev
Copy link
Member Author

A little comment about transparent flag:

As far as I understand, its purpose is to assist client runtime in alpha-blending/sorting operations. So maybe we can add two restrictions:

  1. transparent must be true if values.transparency is defined.
  2. transparent must be true if a material references texture with alpha (A, RGBA, LA).

What do you think?

@pjcozzi
Copy link
Member

pjcozzi commented Jul 7, 2016

Both are OK with me.

@lexaknyazev
Copy link
Member Author

In the core glTF spec there is no requirements on attributes types. This makes sense when using custom shaders but can lead to runtime confusion when using KHR_materials_common or other similar extension. At first glance, types for KHR_materials_common could be defined like this:

Semantic name Type
POSITION VEC3
NORMAL VEC3
TEXCOORD_0 VEC2
JOINT VEC4
WEIGHT VEC4

Not sure about usage of TEXCOORD_1,2,... and COLOR_%d.
Also, it's possible to allow more options for JOINT: in theory, vertex can have from 1 to 16 affecting joints.

@lexaknyazev
Copy link
Member Author

FRAUNHOFER_materials_pbr extension uses different parameters for "diffuse texture" and "diffuse factor".
Using the same approach here could tighten up property types and help with cases like #726.

@xelatihy
Copy link
Contributor

One comment since the extension is not being ratified yet.

There is an inconsistency in the handling of material types with light types (or for that matter other types like camera).

The main issue is that the material values are copied into the values dictionary, but that makes it very loose and hard for an automatic validation. Compare that to the handling of light types, that uses different objects for the different light types. Camera types works the same way.

The current spec follows the technique specification for values where arbitrary values makes are defined to support binding uniforms for arbitrary shaders. This extension though defines fixed values per-type, so placing the values into the values dictionary might not be optimal.

Let me elaborate.

  1. JSON Validation would require special handling, since the values dictionary has per-type values but that cannot be easily specified in JSON validation schema. In fact, no such schema as beein produce yet to validate materials common.
  2. Code generated automatically from the schema cannot have strong types for the values (I wrote one such generator for C++ which is ready expect for materials common). The do so though for light and camera types without any change to codegen functions.
  3. Translation to other formats would require interpreting the values dictionary manually, as opposed to just look up the value per material type.

Let me suggest a simple modification that is backward compatible with the previous model but allows for schema validation and automatic codegen from the schema. I follow the same semantic as light/camera type.

  1. Define a type parameter (as in light type) that takes all values for the materials common technique. If desired add an additional enum values, say SHADER, to indicate arbitrary shading.
  2. Defines schemas for each material type, like lights/cameras. Include those schema as optionally named properties.
  3. If desired, to allow for simpler integration in shading based systems, allow/require shading values to be duplicated into the values dictionary and the technique name to be specified.
  4. PBR extension, and future material types, can then be easily added as an additional typed properties.

If this has any hope for traction, I would be happy to write a proposal and schema, test the schema and update Collada2gltf converter myself within the next month.

Please let me know what you think.

@pjcozzi
Copy link
Member

pjcozzi commented Nov 23, 2016

@xelatihy thanks for the offer. To start, I think it would be useful to see some short before/after examples.

@xelatihy
Copy link
Contributor

xelatihy commented Nov 23, 2016

Here is a first, trivial example, taken from Samples/Box/gltf-MaterialsCommon/Box.gltf.

Current JSON for material, reformatted to make it clearer to compare.

"KHR_materials_common": {
    "doubleSided": false,
    "jointCount": 0,
    "technique": "PHONG",
    "transparent": false,
    "values": {
        "diffuse": [0.8,0,0,1],
        "shininess": 256,
        "specular": [0.2,0.2,0.2,1]
    }
}

Proposed new format to match the above definition.

"KHR_materials_common": {
    "type": "PHONG",
    "phong": {
        "doubleSided": false,
        "jointCount": 0,
        "transparent": false,
        "diffuse": [0.8,0,0,1],
        "shininess": 256,
        "specular": [0.2,0.2,0.2,1]
    }
}

The difference is minimal in the example, but the schema definition can now be explicit. For example, I can have a schema file for the object phong, like material.phong. I would then have a different schema for LAMBERT, for example for the following example.

"KHR_materials_common": {
    "type": "LAMBERT",
    "lambert": {
        "doubleSided": false,
        "jointCount": 0,
        "transparent": false,
        "diffuse": [0.8,0,0,1],
    }
}

@xelatihy
Copy link
Contributor

The one concern with the above definition is that material values are not shared between material types, and this might not be desirable. A different approach is to keep the values dictionary, but make the value object strongly typed via a schema for value. For example, one such schema would define properties for diffuse, specular, etc. by giving them strong types, like for example the translation/rotation/scaling in the node type, but disallowing to have any value contained in the values array (like it is supported for values in techniques).

To be more clear, the values in technique.parameters are defined as

{
    "properties" : {
        "value" : {
            "anyOf" : [{"type" :["number", "boolean", "string"]}, { "$ref" : "arrayValues.schema.json" }]
        }
    }
}

My second proposal would be to define the schema for this extension as

{
    "properties" : {
        "value" : {
            "allOf" : [{"$ref" : "material.value.schema.json" }]
        }
    }
}

and defined each variable specifically in another file, like

{
    "properties" : {
        "diffuse" : {
            "type" : "array",
            "description" : "The node's non-uniform scale.",
            "items" : {
                "type": "number"
            },
            "minItems" : 4,
            "maxItems" : 4,
            "default" : [ 0.0, 0.0, 0.0, 0.0 ]

        }
        ....
    }
}

The difference between this and my previous post is just that

  1. this matches the files already existing (but adds and unneeded indirection in the dictionary values)
  2. this assumes that all materials have the same variables

The first proposal instead specified each material type independently.

Both, to my eyes, are better than having and arbitrary list of variables like techniques.parameter.

Hope this helps.

I would be happy to write down the spec following this proposal too.

@xelatihy
Copy link
Contributor

xelatihy commented Nov 23, 2016

One last common on values definition.

One concern I have is that many values are either colors or textures (in the schema either arrays with min and max length equals to 4), or strings. This makes is very hard to do two things:

  1. write a simple schema like of the other ones in glTF;
  2. automatically generate a statically typed parser (since we have to inject code that creates variants, which are not so hot in C++);
  3. hard to translate models from OBJ that supports defining both.

Similar concerns have been voiced in the PBR thread.

Would be great to have something like "diffuse" and "diffuse_texture".

Hope this helps.

@lexaknyazev
Copy link
Member Author

lexaknyazev commented Dec 20, 2016

Big re-edit of this extension.
See Live version, because diff is too complicated.

Changelog:

  • All fields are documented now.
  • Separation of "factors" and "textures" for material components. This enables static type validation and simplifies runtime loading. Old fields names aren't used anymore, so existing implementations can easily convert previous polymorphic properties at run-time (to support existing assets).
  • Enforcement of accessor's types.
  • Lots of small edits.

Couple new questions:

  • Should we limit usage of this extension to triangle-based meshes only?

  • Does ambient texture mean AO? Do we have tooling/models/pipeline for it? If so, this must be documented. Not AO, see the next comment below.

  • COLLADA treats all textures as RGBA. How exactly should implementations use four (ambient, emission, diffuse, and specular) alpha channels?

  • It's quite strange to have support for AO(?) maps, but not having support for Bump or Normal maps. With those features, this extension could be even closer to three.js's MeshPhongMaterial.
    Should we align (or just become closer) with three.js's MeshPhongMaterial (AO, Bump, Normal maps)?

  • Should we have separate texture for transparency (think of three.js's .alphaMap or COLLADA's transparent/transparency properties)?

Known Issues

  • Elaborate on transparent dependencies
  • Update JSON Schemas
  • Update Contributors

CC
@pjcozzi @tparisi @mlimper @lasalvavida

@lexaknyazev
Copy link
Member Author

Does ambient texture mean AO?

It's definitely not AO in COLLADA's terms (it's just a dedicated "receiver" of ambient light sources).
Since such approach (separate ambient texture) isn't common in runtimes, should we remove it?

@pjcozzi
Copy link
Member

pjcozzi commented Dec 28, 2016

Thanks @lexaknyazev. Note that I am not the authority here, but I will provide some input to try to help move this forward.

Should we limit usage of this extension to triangle-based meshes only?

No, it is reasonable to shade things like point clouds, and I don't think disallowing this buys much.

Since such approach (separate ambient texture) isn't common in runtimes, should we remove it?

OK with me.

Should we align (or just become closer) with three.js's MeshPhongMaterial (AO, Bump, Normal maps)?

I dunno. Sounds like a lot of work. Is it?

In practice, when will people use this extension? I would rather everyone move to PBR. Will people use this often enough to justify the work for these maps (not just spec and tools, think of the engines: how much code can be shared with the two rendering paths).

Should we have separate texture for transparency (think of three.js's .alphaMap or COLLADA's transparent/transparency properties)?

Go with whatever you think the industry runtime standard practice is. In Cesium, we usually have an RGBA texture with diffuse/transparency; I don't know that this is done broadly.

@lexaknyazev
Copy link
Member Author

No, it is reasonable to shade things like point clouds, and I don't think disallowing this buys much.

OK

Will people use this often enough to justify the work for these maps (not just spec and tools, think of the engines: how much code can be shared with the two rendering paths).

My initial thought was that engines already have such features, so adding them to spec shouldn't be much bother for implementations. However making that stuff reliable and consistent across different runtimes can take some time.

Go with whatever you think the industry runtime standard practice is.

COLLADA's definition of opaque/transparency is very vague (look at related collada2gltf issues).
Things like alpha channel in specular map or separate alpha channel for each RGB channel aren't common.

In Cesium, we usually have an RGBA texture with diffuse/transparency; I don't know that this is done broadly.

Single alpha-channel in diffusion map works usually, but such setup prevents user from having emission-only material with transparent parts.

Here's what could be done to make alpha handling simpler;

  • Make all *factors RGB only (number[3]), allow values over 1.0 to give some flexibility.
  • Add separate alpha map.
  • Allow embedded alpha channel only in diffuse.
  • All existing alpha channels (transparency property, RGBA diffuse, and alpha map) must be multiplied for rendering.

@pjcozzi
Copy link
Member

pjcozzi commented Dec 28, 2016

Here's what could be done to make alpha handling simpler...

OK with me.

@lexaknyazev
Copy link
Member Author

@pjcozzi

OK with me.

Should I update the spec or wait for more comments on removing ambient textures and redefining alpha handling?


One more question:
Extension spec says:

The common materials in this extension shall support the semantics POSITION, NORMAL, TEXCOORD, COLOR, JOINT, and WEIGHT.

What's expected treatment of COLOR attribute? How does it interact with other material properties?
E.g., COLOR attribute values could replace mesh diffuse color (factor), like in three.js.

@pjcozzi
Copy link
Member

pjcozzi commented Dec 29, 2016

Should I update the spec or wait for more comments on removing ambient textures and redefining alpha handling?

I'm not sure how much more feedback you are going to get given how long this extension has been in draft. Let's just move forward.

What's expected treatment of COLOR attribute? How does it interact with other material properties?
E.g., COLOR attribute values could replace mesh diffuse color (factor), like in three.js.

Sounds reasonable to me.

@lexaknyazev
Copy link
Member Author

Some more comments regarding compatibility with existing engines (so this extension could be rendered with engine's existing materials):

Material models:

KHR_materials_common three.js Babylon JS
CONSTANT MeshLambertMaterial, use emission only StandardMaterial
LAMBERT MeshLambertMaterial, Gouraud (per-vertex) shading StandardMaterial
PHONG MeshPhongMaterial, Phong (per-pixel) shading, actually uses Blinn approximation StandardMaterial
BLINN MeshPhongMaterial, Phong (per-pixel) shading StandardMaterial

Properties:

KHR_materials_common Type three.js Type BJS BJS Type
transparent boolean .transparent boolean - -
transparency float, 0.0..1.0 .opacity float, 0.0..1.0 alpha float
doubleSided boolean .side enum: FrontSide, BackSide, DoubleSide backFaceCulling boolean
alpha texture ? .alphaMap Texture with g channel sampling opacityTexture Texture with A channel or weighted RGB sum
diffuseFactor float[3], >= 0.0 .color float[3], 0.0..1.0 diffuseColor float[3], 0.0..1.0
diffuseTexture RGBA texture .map RGBA texture diffuseTexture RGBA texture, alpha channel could be disabled
emissionFactor float[3], >= 0.0 Product of .emissive and .emissiveIntensity float[3], 0.0..1.0; float emissiveColor float[3], 0.0..1.0
emissionTexture RGB texture .emissiveMap RGB texture emissiveTexture RGB texture
shininess float, >= 0.0 .shininess float specularPower float
specularFactor float[3], >= 0.0 .specular float[3], 0.0..1.0 specularColor float[3], 0.0..1.0
specularTexture RGB texture .specularMap Texture with r channel sampling specularTexture RGBA texture with specularPower additionally sampled in a channel

Additional notes

  • Babylon supports alpha values for vertices colors, Three.JS doesn't.
  • Babylon multiplies "baseColor" with vertex color, Three.JS replaces.
  • Babylon supports ambient color and texture, Three.JS doesn't.
  • Three.JS allows specifying textures colorspaces. Not sure about Babylon.

Questions

  • Both engines support only Blinn approximation. Can we remove PHONG?
  • Do we care about Gouraud / Phong interpolation?
  • Should diffuseTexture and emissionTexture be treated as sRGB?
  • Should we allow auto-generated normals (both engines could do it) via additional property with FLAT / SMOOTH enum?

Proposals to increase compatibility

  • Rename *Factors to *Colors and clamp them to [0, 1]
  • Store alpha (called opacityTexture?) and specular maps as grayscale in LUMINANCE or RGB format

@pjcozzi
Copy link
Member

pjcozzi commented Dec 30, 2016

Both engines support only Blinn approximation. Can we remove PHONG?

OK with me.

Do we care about Gouraud / Phong interpolation?

Perhaps not...or require Phong.

Should diffuseTexture and emissionTexture be treated as sRGB?

I don't know enough to say. Why?

Should we allow auto-generated normals (both engines could do it) via additional property with FLAT / SMOOTH enum?

No, sounds like a significant implementation burden for what is supposed to be a simple material.

Rename *Factors to *Colors and clamp them to [0, 1]

OK

Store alpha (called opacityTexture?) and specular maps as grayscale in LUMINANCE or RGB format

OK

@lexaknyazev
Copy link
Member Author

Should diffuseTexture and emissionTexture be treated as sRGB?

I don't know enough to say. Why?

It's about documenting rendering pipeline. Specifying colorspace for images helps achieving consistency across implementations. Otherwise, we could just add informal note.

@pjcozzi
Copy link
Member

pjcozzi commented Dec 30, 2016

It's about documenting rendering pipeline. Specifying colorspace for images helps achieving consistency across implementations. Otherwise, we could just add informal note.

Whatever you think is reasonable.

@Moguri
Copy link
Contributor

Moguri commented Feb 10, 2017

Unless I'm missing something, this extension has the same multi-uv issue as the PBR extension (#742). Are there plans to add multi-uv support to this extension, or just the PBR one?

@lexaknyazev
Copy link
Member Author

glTF 2.0 will have multi-UV support in the core spec, so this extension will be updated to reflect that.

@Moguri
Copy link
Contributor

Moguri commented Feb 10, 2017

@lexaknyazev
As far as I can tell, glTF 1.0 already has multi-UV support in the core spec when using shaders. It's just the material extensions that do not support using these multiple UV layers.

Also, should this extension support normal maps? Normal maps are fairly common, but, if you're using this extension, I don't think there is a way to supply them via glTF.

@sbtron
Copy link
Contributor

sbtron commented Feb 17, 2017

@Moguri normal maps will be part of the core spec. The thinking is that this extension can also use the maps from the core spec similar to what the specular-glossiness extension in #842 is doing.
For example:
https://github.com/sbtron/glTF/tree/KHRpbrSpecGloss/extensions/Khronos/KHR_materials_pbrSpecularGlossiness#additional-maps

@pjcozzi
Copy link
Member

pjcozzi commented Jul 16, 2017

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

@pjcozzi pjcozzi closed this as completed Jul 16, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants