KhronosGroup / glTF

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

Updates for KHR_technique_webgl #1217

Closed donmccurdy closed 6 years ago

donmccurdy commented 6 years ago

Let's get the draft KHR_technique_webgl extension finished up. For background on why GLSL was removed from the core specification into an extension with glTF 2.0, see https://github.com/KhronosGroup/glTF/issues/733. The current draft is based almost exactly on glTF 1.0, so if we were to freeze it as-is there would be no significant GLSL-related changes for users upgrading from glTF 1.0. A few changes have been suggested that should also be considered:

donmccurdy commented 6 years ago

Some initial thoughts:

(1) Supporting shaders in binary buffers seems like a good, simple addition. @lasalvavida already made the syntax change in the draft extension, I think we just need to add a few more details to the Shader object.

(2) My preference would be not require particular extensions to specify vertex or fragment, so long as it's a valid URI. The type (vert/frag) is specified in the JSON, and doing more than that feels redundant.

(3–5) No preference, except that I think that perhaps blending could be handled through a WebGL-agnostic syntax elsewhere (https://github.com/KhronosGroup/glTF/issues/1189).


Also — the extension text mentions shaders, programs, and techniques as being in the asset's global glTF namespace. I'm a little worried about that as a precedent for other extensions. Supposing we introduce a KHR_technique_webgl2 extension in the future, would it share the global namespace?

It seems potentially more forward-compatible to list these resources under a top-level extensions object:

{
  "asset": { ... },
  "extensions": {
    "KHR_technique_webgl": {
      "shaders": [ ... ],
      "techniques": [ ... ],
      "programs": [ ... ]
     }
   }
}
takahirox commented 6 years ago

(2) My preference would be not require particular extensions to specify vertex or fragment, so long as it's a valid URI. The type (vert/frag) is specified in the JSON, and doing more than that feels redundant.

Agreed. I don't think we need to restrict extensions. We could add implementation note if we have recommended extensions.

pjcozzi commented 6 years ago

Thanks for kicking off the conversation @donmccurdy!

(1) Supporting shaders in binary buffers seems like a good, simple addition. @lasalvavida already made the syntax change in the draft extension, I think we just need to add a few more details to the Shader object.

+1

(2) My preference would be not require particular extensions to specify vertex or fragment, so long as it's a valid URI. The type (vert/frag) is specified in the JSON, and doing more than that feels redundant.

+1

(3) Consider changing listing of technique.parameters (#717)

OK either way, but if this is truly redundant we should probably fix it now.

(4) Consider support for program pipelines (#840)

I don't think it is in scope.

(5) Consider removing or simplifying the render states portion of the extension (For example, keep blending and face culling, leave the rest to another extension or later versions.)

OK either way, but our use cases are limited to blending and culling.

CC @ggetz @lilleyse if they have any more feedback from the Cesium team.

lilleyse commented 6 years ago

It seems potentially more forward-compatible to list these resources under a top-level extensions object:

I think this is the right way to go too.

Sorry if this is obvious, but one thing I noticed about the draft extension is it isn't super clear that technique is a property of a material. It should probably look something like:

"materials": [
  {
    "extensions": {
      "KHR_technique_webgl": {
        "technique": 0
      }
    }
  }
]
lilleyse commented 6 years ago

As it is now the extension is basically written against GLSL 1.0. We'll have to scrub through the text to make sure the terminology isn't incompatible with GLSL 3.0 ES and beyond.

Some differences:

The uniforms property specifies the uniform variables that will be passed to the shader. Each uniform's name is a string that corresponds to the uniform name in the GLSL source code.

This works for below:

uniform vec4 color;
uniform vec3 lightPosition;
uniform float scale;

But not necessarily something like:

uniform ModelData
{
    vec4 color;
    vec3 lightPosition;
    float scale;
};

Would the uniform name have to be ModelData.color? What happens if the uniform block is an array or has an instance name?

lilleyse commented 6 years ago

I'm happy to make any edits like this once a PR is open.

donmccurdy commented 6 years ago

Maybe a higher level question first... should the extension be allowed to contain WebGL2+ features like GLSL 3.0 ES? Or should we aim to ensure that KHR_technique_webgl can be supported in any WebGL1-based engine if the device supports the necessary WebGL extensions, and leave the rest for some future KHR_technique_webgl2 or KHR_technique_webgpu extensions?

pjcozzi commented 6 years ago

My impressive was this is targeting WebGL 1.0 as 2.0 would increase the scope quite a bit.

lilleyse commented 6 years ago

Alright targeting just WebGL 1.0 sounds good to me. The extension should make sure to mention that.

lexaknyazev commented 6 years ago

Proper WebGL 2 support would require more efforts than just aligning terms - GLSL versioning, UBOs, integer attributes, and instancing. (MRT? TF?) On the other hand, what do you think about including common WebGL 1.0 extensions?

E.g., here're support numbers from webglstats.com:

1d2d3d commented 6 years ago

+1 for the extensions, and a general thanks for getting this underway - "waiting with baited breath".

lilleyse commented 6 years ago

Another minor thing to add - glTF 1.0 had asset.premultipliedAlpha which would make sense to include in some way for this extension.

lexaknyazev commented 6 years ago

For reference, here's the current schema design:

{
    "asset": {
        "version": "2.0"
    },
    "meshes": [
        {
            "primitives": [
                {
                    "attributes": {
                        "POSITION": 0
                    },
                    "material": 0
                }
            ]
        }
    ],
    "materials": [
        {
            "extensions": {
                "KHR_technique_webgl": {
                    "technique": 0,
                    "values" :{
                        "parameter3": 0.75
                    }
                }
            }
        }
    ],
    "extensions": {
        "KHR_technique_webgl": {
            "shaders": [
                {
                    "type": 35632,
                    "uri": "technique0.frag"
                },
                {
                    "type": 35633,
                    "uri": "technique0.vert"
                }
            ],
            "programs": [
                {
                    "fragmentShader": 0,
                    "vertexShader": 1
                }
            ],
            "techniques": [
                {
                    "program": 0,
                    "parameters": {
                        "parameter1": {
                            "type": 5126,
                            "semantic": "POSITION"
                        },
                        "parameter2": {
                            "type": 35676,
                            "semantic": "MODEL"
                        },
                        "parameter3": {
                            "type": 5126,
                            "value": 0.9
                        }
                    },
                    "attributes":{
                        "a_position": "parameter1"
                    },
                    "uniforms": {
                        "u_model": "parameter2",
                        "u_custom": "parameter3"
                    },
                    "states": {
                        "functions": {
                            "blendColor": [0.0, 0.0, 0.0, 0.0],
                            "blendEquationSeparate": [32774, 32774],
                            "blendFuncSeparate": [1, 0, 1, 0],
                            "colorMask": [true, true, true, true],
                            "cullFace": [1029],
                            "depthFunc": [513],
                            "depthMask": [true],
                            "depthRange": [0.0, 1.0],
                            "frontFace": [2305],
                            "lineWidth": [1.0],
                            "polygonOffset": [0.0, 0.0]
                        },
                        "enable": [
                            3042, 2884, 2929, 32823, 32926
                        ]
                    }                    
                }
            ]
        }
    }
}
lexaknyazev commented 6 years ago
lilleyse commented 6 years ago

Given that premultipliedAlpha is enabled on the WebGL context level, does it mean that engines won't be able to easily load assets with different values of that flag into one scene? (Sure, they could workaround this limitation via intermediate framebuffers.)

In Cesium if we see that a 1.0 model has premultiplied alpha we end up patching the shader to unpremultiply it. I'm not sure how other engines handle it, or if they even care about it at all.

pjcozzi commented 6 years ago

Do we expect re-usage of shaders between different programs? If not (or if it's rare), top-level shaders array could be inlined into program objects.

No strong preference, but I think some vertex shaders would be reused and the original design follows the GL API.

Should we allow default value for attributes?

I don't think it is needed.

This is a rarely used GL feature, Cesium is the only engine I know that uses it and only in rare cases.

donmccurdy commented 6 years ago

One minor thought... we've used KHR_materials_* elsewhere, with plural "materials". Perhaps this should be KHR_techniques_webgl rather than "technique"?

ggetz commented 6 years ago

So to move this discussion along, this appears to be the consensus, please correct me otherwise:

Also KHR_techniques_webgl +1

lilleyse commented 6 years ago

I'm starting to feel torn about the render states and premultiplied alpha.

They're helpful to have for upgrading models from 1.0 to 2.0, but in practice these values will be rarely set. And we already have alphaMode and doubleSided for the most common cases.

So to simplify I think we should actually scratch those off the list unless there's a good reason to keep them.

lilleyse commented 6 years ago

Otherwise I'm in agreement with the list above.

If there aren't any major concerns I think we can get started with a pull request.

ggetz commented 6 years ago

@lilleyse To clarify, by scratching render states off the list, do you mean leaving them as is, or prefer removing them completely?

lilleyse commented 6 years ago

I'd prefer removing them.

ggetz commented 6 years ago

OK, I'm good with that. There doesn't seem to be any conflicting opinions, I've updated my previous comment.

pjcozzi commented 6 years ago

I think I am misunderstanding something, but I would caution against completely removing render states as we need them at least for culling and blending.

lilleyse commented 6 years ago

Culling and blending are already mostly handled by doubleSided and alphaMode in the material.

There could be a case for wanting to support non-standard blending modes, but I don't know how important that is.

pjcozzi commented 6 years ago

Ah, that could work then. OK with me if no one objects.

emackey commented 6 years ago

As someone who hand-edited those culling and blending render states in far too many glTF 0.8/1.0 files, I think the move to doubleSided/alphaMode would be welcomed by users.

donmccurdy commented 6 years ago

I think it may be worthwhile to support more blending modes eventually (https://github.com/KhronosGroup/glTF/issues/1189), but I'd prefer to do that in a renderer-agnostic way rather than the WebGL extension if/when we do. So, no objection to omitting that from KHR_techniques_webgl.

@stevesan can you comment on what Tilt Brush might need? Any concerns about render state features?

ggetz commented 6 years ago

PR with edits opened in https://github.com/KhronosGroup/glTF/pull/1296

stevesan commented 6 years ago

Woops, sorry for the delay. I'll check in more detail tomorrow, but TB definitely needs additive blending.

UPDATE: Just confirming that TB only uses render states for face culling and additive blending control. So I think we're good here.

birdimus commented 6 years ago

I'd encourage preserving render states if possible. They are well supported since SM 3.0 / DX 9, and are useful for a wide variety of graphics techniques.

ggetz commented 6 years ago

@birdimus The idea was to tackle render states in another extension or later versions in order to keep this as extension as WebGL-agnostic as possible.

For example, blending techniques are being covered by the proposed EXT_blend by @donmccurdy.