KhronosGroup / glTF

glTF – Runtime 3D Asset Delivery
Other
7.18k stars 1.14k forks source link

Finishing KHR_materials_common #632

Closed lexaknyazev closed 7 years ago

lexaknyazev commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

@pjcozzi @tparisi Could you review #633?

pjcozzi commented 8 years ago

@lexaknyazev I agree with your thoughts for doing things later in https://github.com/KhronosGroup/glTF/issues/632#issuecomment-228565181. Is there anything short-term here that you need input from @tparisi or me?

lexaknyazev commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

Are these all in #633?

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

pjcozzi commented 8 years ago

I can make one more PR

Yes, please.

lexaknyazev commented 8 years ago

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 commented 8 years ago

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

tfili commented 8 years ago

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 commented 8 years ago

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

@lexaknyazev is there anything else you are waiting on?

lexaknyazev commented 8 years ago

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 commented 8 years ago

Both are OK with me.

lexaknyazev commented 8 years ago

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 commented 8 years ago

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 https://github.com/KhronosGroup/glTF/issues/726.

xelatihy commented 7 years ago

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 commented 7 years ago

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

xelatihy commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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

Changelog:

Couple new questions:

Known Issues

CC @pjcozzi @tparisi @mlimper @lasalvavida

lexaknyazev commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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;

pjcozzi commented 7 years ago

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

OK with me.

lexaknyazev commented 7 years ago

@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 commented 7 years ago

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 commented 7 years ago

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


Questions

Proposals to increase compatibility

pjcozzi commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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

Moguri commented 7 years ago

@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 commented 7 years ago

@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 commented 7 years ago

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