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

technique.parameter.type compatibility #789

Closed
lexaknyazev opened this issue Dec 3, 2016 · 18 comments
Closed

technique.parameter.type compatibility #789

lexaknyazev opened this issue Dec 3, 2016 · 18 comments

Comments

@lexaknyazev
Copy link
Member

lexaknyazev commented Dec 3, 2016

via @lilleyse from CesiumGS/cesium#4684 (comment)

Does a parameter's type equal the data type stored in the buffer or the type declared in the shader? If batchId is stored as unsigned shorts, should type be 5123 or 5126.

"technique": {
    "attributes": {
        "a_batchId": "batchId"
    },
    "parameters": {
        "batchId": {
            "semantic": "BATCHID",
            "type": 5123
        }
    }
}

Shader

attribute float a_batchId;

Couple things to consider here:

  • GLSL ES 1.0 doesn't support integer attributes. With vertexAttribPointer, all integers are converted to floats (exact conversion process depends on the value of accessor.normalized).
  • We must enforce compatibility of components number.

I propose to add following clarification:

technique.parameter.type must match used GLSL ES type, and its components number must match accessor.type (if applicable).

GLSL ES Type parameter.type accessor.type
float GL_FLOAT "SCALAR"
vec2 GL_FLOAT_VEC2 "VEC2"
vec3 GL_FLOAT_VEC3 "VEC3"
vec4 GL_FLOAT_VEC4 "VEC4"
mat2 GL_FLOAT_MAT2 "MAT2"
mat3 GL_FLOAT_MAT3 "MAT3"
mat4 GL_FLOAT_MAT4 "MAT4"

CC @pjcozzi

@pjcozzi
Copy link
Member

pjcozzi commented Dec 6, 2016

My intention reaction was that it should match the value stored in the buffer. If, for example, the buffer has ints and the shader declares a float, how could we access the buffer values in JavaScript since glTF says they are floats when they are really ints?

@lexaknyazev
Copy link
Member Author

accessor.type and accessor.componentType are used for accessing stored values.

@pjcozzi
Copy link
Member

pjcozzi commented Dec 6, 2016

Ah, right. Yes, this clarification is good with me. It could go into 1.1, or even 1.0 if you prefer.

@lexaknyazev
Copy link
Member Author

@pjcozzi
Btw, why do we have parameter.type for attributes at all?
While it could be useful for some asset manipulations, I can't see any runtime usage for it.

This naturally leads us to the next question: right now, with glTF 1.1 attribute parameters can have only semantic and type properties. Removing type allows us to reduce indirection and simplify implementation there (aka use parameters only for uniforms) without any feature loss:

{
    "techniques" : {
        "technique_id" : {
            "parameters": {
                "ambient": {
                    "type": 35666,
                },
                "diffuse": {
                    "type": 35678
                },
                "modelViewMatrix": {
                    "semantic": "MODELVIEW",
                    "type": 35676
                },
                "projectionMatrix": {
                    "semantic": "PROJECTION",
                    "type": 35676
                },
                "normalMatrix": {
                    "semantic": "MODELVIEWINVERSETRANSPOSE",
                    "type": 35675
                },
                "jointMatrix": {
                    "semantic": "JOINTMATRIX",
                    "type": 35676
                }
            },
            "attributes": {
                "a_position": "POSITION",
                "a_normal": "NORMAL",
                "a_texcoord0": "TEXCOORD_0",
                "a_joint": "JOINT",
                "a_weight": "WEIGHT"
            },
            "program": "program_id",
            "uniforms": {
                "u_ambient": "ambient",
                "u_diffuse": "diffuse",
                "u_modelViewMatrix": "modelViewMatrix",
                "u_projectionMatrix": "projectionMatrix",
                "u_normalMatrix": "normalMatrix",
                "u_jointMatrix": "jointMatrix"
            }
        }
    }
}

CC @lilleyse @javagl @lasalvavida

@pjcozzi
Copy link
Member

pjcozzi commented Dec 7, 2016

It would be nice to get rid of the indirection. Cesium does use type: https://github.com/AnalyticalGraphicsInc/cesium/blob/master/Source/Scene/Model.js#L2629-L2644. I also think that some runtime uses, like showing a UI to edit parameters, would also want it.

Do you think there is another way to achieve this?

@lexaknyazev
Copy link
Member Author

Sorry, but I see only uniform usages in that code. Or am I missing something?

@pjcozzi
Copy link
Member

pjcozzi commented Dec 7, 2016

Ah, yes, I agree. It is not used in Cesium!

@javagl
Copy link
Contributor

javagl commented Dec 8, 2016

I also don't see a reason to not remove this indirection

Related: #717 and (more importantly) #92 (mentioned here just as references, I'm currently reading through the latter - there has been an extensive discussion, maybe some points are relevant here as well)

@lexaknyazev
Copy link
Member Author

I've found (made up) a very controversial case for type, semantic, and even value properties for attribute parameters.

Let's say we've got two meshes: the first one with vertex positions and vertex colors, while the second with vertex positions only.

{
    "meshes": {
        "mesh_with_colors": {
            "primitives": [
                {
                    "attributes": {
                        "POSITION": "accessor_pos",
                        "COLOR_0": "accessor_col"
                    },
                    "material": "material0"
                }
            ]
        },
        "mesh_without_colors": {
            "primitives": [
                {
                    "attributes": {
                        "POSITION": "accessor_pos2"
                    },
                    "material": "material0"
                }
            ]
        }
    },
    "materials": {
        "material0": {
            "technique": "technique0",
            "values": {
                "color": [0.8, 0.8, 0.8]
            }
        }
    },
    "techniques": {
        "technique0": {
            "parameters": {
                "position": {
                    "semantic": "POSITION",
                    "type": 35665
                },
                "color": {
                    "semantic": "COLOR_0",
                    "type": 35665,
                    "value": [0.5, 0.5, 0.5]
                }
            },
            "attributes": {
                "a_pos": "position",
                "a_col": "color"
            },
            "program": "program0"
        }
    }
}

With such scheme, we would be able to use material.values / parameter.value to assign default color for the second mesh with:

gl.vertexAttrib3fv(colorLocation, material.values.color);

@javagl
Copy link
Contributor

javagl commented Dec 8, 2016

I didn't have glVertexAttrib on the screen (never used it), but if I understood this correctly, you're aiming at an application case with a pseudocode like this:

var parameter = technique.parameters[attributeValue];
if (primitive.hasAttribute(parameter.semantic)) {
    glVertexAttribPointer(... bufferView for primitive.attribute[parameter.semantic] ...);
} else {
    glVertexAttrib(parameter.value); // generic attribute from default value
}

If this is right (and technically possible) then this looks like a very good reason to keep the parameter mapping (at least for me, and I'm curious what others think).

Although such a generic attribute value at the first glance only makes sense for the color, I think that sophisticated shaders may have additional attributes that can optionally be fed with such a constant value. The alternatives would then be to create a (potentially large) accessor/bufferView that only contains constant value, or a definition of a (potentially complex) technique that only differs in how this single attribute is handled.

@lexaknyazev
Copy link
Member Author

if I understood this correctly, you're aiming at an application case with a pseudocode like this

That's correct.


this looks like a very good reason to keep the parameter mapping (at least for me, and I'm curious what others think)

@pjcozzi said once:

This would create an implementation burden for a rarely needed feature.


Maybe, it's possible to reduce indirection (and simplify finding needed object) without compromising flexibility?

@pjcozzi
Copy link
Member

pjcozzi commented Dec 14, 2016

I lean towards simplifying the spec and getting the breaking change into 1.1, but don't have a strong preference.

@lexaknyazev
Copy link
Member Author

@pjcozzi What's your perspective on "default" attribute values? Is it still the same?

@pjcozzi
Copy link
Member

pjcozzi commented Dec 14, 2016

It is the same. Certainty useful for some cases, but perhaps not enough to justify it.

@javagl
Copy link
Contributor

javagl commented Feb 4, 2017

Will the technique.parameters also become an array? (In the announcement, this only referred to top-level dictionaries, but I have seen other dictionaries being replaced as well)

@pjcozzi
Copy link
Member

pjcozzi commented Feb 4, 2017

Will the technique.parameters also become an array?

I don't think it is necessary.

@pjcozzi
Copy link
Member

pjcozzi commented Dec 24, 2017

@lexaknyazev any actions left here?

@lexaknyazev
Copy link
Member Author

We'll revisit this (or similar) issue during WebGL extension development.

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

3 participants