Updates to KHR_technique_webgl extension spec #1296
Conversation
|
@ggetz congrats on your first glTF extension work! @donmccurdy @lexaknyazev would love your feedback here. |
|
@ggetz I started to review this afternoon and should have some feedback posted tomorrow. |
There was a problem hiding this comment.
Thanks for getting this underway!
Could I talk you into writing a simple stub example? That would help me get a clearer picture of this.
About render states — Tilt Brush will need additive blending. Sketchfab is currently falling back to alpha blending for models that use additive transparency as well. Including blend states in KHR_techniques_webgl might be the quickest path to supporting that. But in the long run, I would prefer to see a GLSL-agnostic way to specify widely-supported blend operations, e.g. a KHR_blending extension... Preferences? Including blend states here would not rule out a separate extension later, but I realize time/energy are limited resources. 🙂
/cc @stevesan
|
|
||
| #### Attributes | ||
|
|
||
| The `attributes` dictionary property specifies the vertex attributes of the data that will be passed to the shader. Each attribute's name is a string that corresponds to the attribute name in the GLSL source code. Each attribute's value is an [attribute](#reference-technique.attributes) object, where the type (GL types such as a floating point number, vector, texture, etc.) and semantic of the attribute is defined. |
There was a problem hiding this comment.
nit: perhaps say "Each attribute's key" or "Each attribute's index" rather than "name", to disambiguate from the ".name" properties used for human-readable labels throughout glTF.
| @@ -310,7 +236,8 @@ Shader source files are stored in the asset's `shaders` dictionary property, whi | |||
| ] | |||
| } | |||
| ``` | |||
| ### Properties Reference | |||
|
|
|||
| ## Properties Reference | |||
There was a problem hiding this comment.
In this section, .programs, .shaders, and .techniques are now arrays with indices rather than an "ID in the global glTF namespace".
|
|
||
| #### Values | ||
|
|
||
| The `values` property defines the list of values, each of which is passed to the technique uniform of the same name, and when specified, overrides the corresponding uniform `value`. |
There was a problem hiding this comment.
Since neither .values here nor uniform.value below are required, perhaps we need to say that at least one of the two must be present?
When you say simple stub, I assume you mean the entire (but simple) glTF?
I agree the more agnostic approach is best in the long run. If tilt brush needs it now, and it doesn't rule out the longer-term approach, I'm fine with addressing it in this extension. |
Yes, not necessarily including buffers or even meant to be rendered, but an example that shows all of the top-level properties cross-referencing one another. |
| @@ -1,4 +1,4 @@ | |||
| # KHR_technique_webgl | |||
| # KHR_techniques_webgl | |||
There was a problem hiding this comment.
Rename all files to KHR_techniques_webgl as well
| @@ -1,4 +1,4 @@ | |||
| # KHR_technique_webgl | |||
| # KHR_techniques_webgl | |||
|
|
|||
| ## Contributors | |||
There was a problem hiding this comment.
Add yourself and @lexaknyazev to the contributors section. Create a TODO item in the PR description to add others as we go.
There was a problem hiding this comment.
Perhaps add a copyright notice (and appendix) like the Draco extension.
General question: should the copyright notice be added to pbrSpecularGlossiness as well?
| @@ -1,4 +1,4 @@ | |||
| # KHR_technique_webgl | |||
| # KHR_techniques_webgl | |||
|
|
|||
| ## Contributors | |||
|
|
|||
There was a problem hiding this comment.
I'm not sure what should go under the Status heading initially, but add a TODO item to change this to "Complete" once ready.
| * [`functions`](#reference-technique.states.functions) | ||
|
|
||
| * [`attributes`](#reference-technique.attributes) | ||
| * [`uniforms`](#reference-technique.uniforms) | ||
|
|
||
| ## Known Implementations |
There was a problem hiding this comment.
I don't see this heading in the other extensions so it can be removed.
| * [`functions`](#reference-technique.states.functions) | ||
|
|
||
| * [`attributes`](#reference-technique.attributes) | ||
| * [`uniforms`](#reference-technique.uniforms) | ||
|
|
||
| ## Known Implementations |
There was a problem hiding this comment.
Perhaps add the webgl spec to the References section.
|
|
||
| ### Extending Materials | ||
|
|
||
| If a `material` contains an `extensions` property and the `extensions` property defines its `KHR_techniques_webgl` property, then the specified shading technique can be used, as specified in the asset's `techniques`. |
There was a problem hiding this comment.
Consider tweaking the wording to read more like https://github.com/KhronosGroup/glTF/tree/master/extensions/2.0/Khronos/KHR_materials_pbrSpecularGlossiness#extending-materials
, as specified in the asset's
techniques.
This could be removed since it is described in the next section.
|
|
||
| #### Attributes | ||
|
|
||
| The `attributes` dictionary property specifies the vertex attributes of the data that will be passed to the shader. Each attribute's name is a string that corresponds to the attribute name in the GLSL source code. Each attribute's value is an [attribute](#reference-technique.attributes) object, where the type (GL types such as a floating point number, vector, texture, etc.) and semantic of the attribute is defined. |
There was a problem hiding this comment.
Similar to the other comment, remove texture.
|
|
||
| If no `node` property is supplied for a semantic, the semantic is implied in a context-specific manner: either to the node which is being rendered, or in the case of camera-specific semantics, to the current camera. In the following fragment, which defines a parameter named `projectionMatrix` that is derived from the implementation's projection matrix, the semantic would be applied to the camera. | ||
| In the above example, the uniform `u_light0Transform` defines the `MODELVIEW` semantic, which corresponds to the world space position of the node referenced in the property `node`, in this case the node `1`, which refers to the id of a node that contains a light source. |
There was a problem hiding this comment.
Should world space be eye space?
| If no `node` property is supplied for a semantic, the semantic is implied in a context-specific manner: either to the node which is being rendered, or in the case of camera-specific semantics, to the current camera. In the following fragment, which defines a parameter named `projectionMatrix` that is derived from the implementation's projection matrix, the semantic would be applied to the camera. | ||
| In the above example, the uniform `u_light0Transform` defines the `MODELVIEW` semantic, which corresponds to the world space position of the node referenced in the property `node`, in this case the node `1`, which refers to the id of a node that contains a light source. | ||
|
|
||
| If no `node` property is supplied for a semantic, the semantic is implied in a context-specific manner: either to the node which is being rendered, or in the case of camera-specific semantics, to the current camera. In the following fragment, which defines a uniform named `u_projectionMatrix` that is derived from the implementation's projection matrix, the semantic would be applied to the camera. |
There was a problem hiding this comment.
Change semantic to uniform:
If no
nodeproperty is supplied for a uniform
| #### Programs | ||
|
|
||
| GLSL shader programs are stored in the asset's `programs` property. This property contains one or more objects, one for each program. | ||
|
|
||
| Each shader program includes an `attributes` property, which specifies the vertex attributes that will be passed to the shader, and the properties `fragmentShader` and `vertexShader`, which reference the files for the fragment and vertex shader GLSL source code, respectively. | ||
| Each shader program includes an `attributes` property, which specifies the vertex attributes that will be passed to the shader, and the properties `fragmentShader` and `vertexShader`, which reference the files for the fragment and vertex shader GLSL source code, respectively, as defined the id of the item in the `shaders` dictionary property. |
There was a problem hiding this comment.
as defined the id of the item in the shaders dictionary property.
This terminology should be tweaked since it is an index in an array instead of an id of a dictionary.
|
Let's see if we can do blending in parallel, but keep it separate from this extension. I've started a thread for |
|
Thanks @donmccurdy and @lilleyse ! I've updated based on your comments, let me know if you have anymore feedback. |
|
I think #1279 still applies here, do we want to fix that up in this PR? |
|
@donmccurdy The one difference between |
|
In https://github.com/KhronosGroup/glTF/tree/master/extensions rename |
|
Rename |
| @@ -0,0 +1,273 @@ | |||
|
|
|||
There was a problem hiding this comment.
While this is an abridged example, KHR_techniques_webgl should be added to extensionsUsed and extensionsRequired.
|
|
||
| This extension specification is targeting WebGL 1.0, and can be supported in any WebGL 1.0-based engine if the device supports the necessary WebGL extensions. | ||
|
|
||
| * **Abridged example glTF**: [sample_techniques.gltf](examples/sample_techniques.json) |
There was a problem hiding this comment.
The link should be examples/sample_techniques.gltf
|
|
||
| #### Values | ||
|
|
||
| The `values` dictionary property defines the uniform values of a [`technique.uniform`](#Uniforms) of the same key, and when specified, overrides the corresponding uniform `value`. The value supplied must conform to the `type` and `count` properties, if present, of the corresponding `Uniform` object, and must be present if no `value` is supplied in the referenced `Uniform` object. |
There was a problem hiding this comment.
The link needs to be lowercase #uniforms to go to the section header. (A github thing?)
|
|
||
| #### Attributes | ||
|
|
||
| The `attributes` dictionary property specifies the vertex attributes of the data that will be passed to the shader. Each attribute's key is a string that corresponds to the attribute name in the GLSL source code. Each attribute's value is an [attribute](#reference-technique.attributes) object, where the type (GL types such as a floating point number, vector, etc.) and semantic of the attribute is defined. |
There was a problem hiding this comment.
The #reference-technique.attributes link isn't going to the section correctly.
|
|
||
| #### Uniforms | ||
|
|
||
| The `uniforms` dictionary property specifies the uniform variables that will be passed to the shader. Each uniform's key is a string that corresponds to the uniform name in the GLSL source code. Each uniform's value is a string that references a [uniform](#reference-technique.uniforms) object, where the type (GL types such as a floating point number, vector, etc.) and potentially the semantic and default value of the uniform is defined. |
There was a problem hiding this comment.
Same comment for #reference-technique.uniforms
|
|
||
| * **JSON schema**: [material.KHR_technique_webgl.schema.json](schema/material.KHR_technique_webgl.schema.json) | ||
|
|
||
| ### khr_techniques_webgl.material.extension.technique :white_check_mark: |
There was a problem hiding this comment.
Is it fine to drop the khr_techniques_webgl.material.extension. prefix for these? It looks like KHR_materials_pbrSpecularGlossiness doesn't have the prefix.
| "additionalProperties" : { | ||
| "$ref" : "uniform.value.schema.json" | ||
| }, | ||
| "default" : {}, |
There was a problem hiding this comment.
I forget if this came up before, but is to safe to just remove "default" : {} throughout?
| "type" : "object", | ||
| "description" : "A dictionary object of `Attribute` objects.", | ||
| "properties" : { | ||
| }, |
There was a problem hiding this comment.
Throughout, for empty property objects just condense to a single line: "properties" : {},
| ] | ||
| } | ||
| } | ||
| } |
| "$ref" : "textureInfo.schema.json" | ||
| } | ||
| ] | ||
| } |
|
Thanks @lilleyse ! Addressed your comments, should we address these TODO items now then? Who all should be added to contributors?
|
| "type" : "object", | ||
| "description" : "A dictionary object of `Uniform` objects.", | ||
| "properties" : { | ||
| }, |
There was a problem hiding this comment.
Change to "properties" : {}
| "type" : "object", | ||
| "description" : "Dictionary object of uniform values.", | ||
| "properties" : { | ||
| }, |
There was a problem hiding this comment.
Change to "properties" : {}
|
|
||
| * **JSON schema**: [glTF.KHR_techniques_webgl.schema.json](schema/glTF.KHR_techniques_webgl.schema.json) | ||
|
|
||
| ### khr_techniques_webgl.gltf.extension.programs :white_check_mark: |
There was a problem hiding this comment.
I forgot to mention, also remove the khr_techniques_webgl.gltf.extension. prefixes in this section. Related to #1296 (comment)
|
|
||
| Technique uniforms may also optionally define a *semantic*, an enumerated value describing how the runtime is to interpret the data to be passed to the shader. | ||
|
|
||
| In the above example, the uniform `u_light0Transform` defines the `MODEL` semantic, which corresponds to the world space position of the node referenced in the property `node`, in this case the node `1`, which refers to the id of a node that contains a light source. |
I wonder if at least some of the glTF 1.0 contributors should be added here since it borrows from the 1.0 spec. Maybe @pjcozzi?
This one could be addressed now. |
|
Is anyone else interested in reviewing this? @lexaknyazev, @pjcozzi, @stevesan? |
|
Will do by tomorrow's call. |
|
I won't be able to review, but looks like this is getting enough attention. For carry over credits from the 1.0 spec, probably include:
|
| "type" : "object", | ||
| "description" : "A shader program, including its vertex and fragment shader, and names of vertex shader attributes.", | ||
| "description" : "A shader program, including its vertex and fragment shaders, and names of vertex shader attributes.", |
There was a problem hiding this comment.
Remove and names of vertex shader attributes
|
@lilleyse Updated. |
extensions/README.md
Outdated
| @@ -6,12 +6,12 @@ | |||
| * [KHR_materials_pbrSpecularGlossiness](2.0/Khronos/KHR_materials_pbrSpecularGlossiness/README.md) | |||
| * [KHR_materials_unlit](2.0/Khronos/KHR_materials_unlit/README.md) | |||
| * [KHR_draco_mesh_compression](2.0/Khronos/KHR_draco_mesh_compression/README.md) | |||
| * [KHR_techniques_webgl](2.0/Khronos/KHR_techniques_webgl/README.md) | |||
There was a problem hiding this comment.
Can we mark this as draft here and in the main extension spec and then merge this?
|
@pjcozzi Marked as draft. Ready to merge? |
this one's a surprise to me — is that possible in the general case? In either case I'm OK with merging this as an updated draft. |
|
@donmccurdy How else could an engine pass |
|
How would an engine know whether the supplied GLSL shader code needs to be patched at all? If the GLSL contains a line like this: if ( myDiffuseColor.a < 0.5 ) discard;The engine can't detect that without parsing the GLSL, which I don't think is a realistic expectation. It seems more reasonable to require that the GLSL include the One implication, in any case — animating |
That line seems to enforce I was thinking about adding something like if ( gl_FragColor.a < cutOffUniformOrConstant ) discard;to the very end of fragment shader's |
I don't follow this — if the asset contains Even detecting the end of the fragment shader's One alternative might be to define a technique uniform semantic, |
|
My understanding is that there could be one shader used by different materials that define different Requiring a pre-defined uniform name would definitely simplify engines. |
|
@ggetz what is the latest here? Is this close to being merged as a draft? |
|
@pjcozzi We may want to discuss the implications of the material properties interacting with the shaders more, however, I think the consensus is this is good to merge as a draft. |
|
Removed the requirement to have the engine patch the shader at runtime. Instead, require shaders to respect these values, and specified the |
| > } | ||
| > ``` | ||
|
|
||
| The value of a material `alphaCutoff` property should be passed to the technique shaders using a uniform with the semantic `ALPHACUTOFF`. A uniform with this semantic will ignore a supplied default value and the corresponding material uniform value. |
There was a problem hiding this comment.
Perhaps say the uniform must be consistent with alphaCutoff? Then the client can treat it as a normal uniform, and I think it is more intuitive.
| > { | ||
| > normal = -normal; | ||
| > } | ||
| > ``` |
There was a problem hiding this comment.
I think this affects tangent-space calculations, too. See: https://github.com/mrdoob/three.js/blob/2f207a5d7167884d002a697b6cb15306cf621924/src/renderers/shaders/ShaderChunk/normalmap_pars_fragment.glsl#L34
But technically this applies to engines that don't support KHR_techniques_webgl, so maybe it would be better to specify it in the core spec and reference that here.
|
Added a couple more comments but I don't think they'd prevent this from being merged as a draft. |
|
@ggetz please let me know when you want this merged. |
|
Modified the Shader Requirement implementation note to instead include a TODO to reference once included in the main spec. @pjcozzi I think this is good to merge. |
|
Thanks @ggetz! |
Fixes #1217
Makes the suggestion listed in the above issue, with the most significant changes being:
technique.parametersintechnique.attributesandtechnique.uniformsto reduce indirection, and allow the objects for each to be more specific toattributesanduniformsuseprogram.glExtensionsKHR_techniques_webglinstead ofKHR_technique_webglREADMETODO:
@lilleyse let me know if you have any initial feedback.