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

What is the benefit of separately listing the technique.parameters? #717

Closed
mre4ce opened this issue Sep 14, 2016 · 9 comments
Closed

What is the benefit of separately listing the technique.parameters? #717

mre4ce opened this issue Sep 14, 2016 · 9 comments

Comments

@mre4ce
Copy link

mre4ce commented Sep 14, 2016

I looked through the existing issues and couldn't find a clear answer. Why is there a separate technique.parameters instead of directly listing the properties under each of technique.attributes and technique.uniforms. Currently technique.parameters can or cannot list properties based on whether the parameter is a unifom or vertex attribute. This seems to add unnecessary complexity, but maybe there's a good reason to have a separate list of parameters?

@lexaknyazev
Copy link
Member

Looks like #92 could provide some history behind technique.parameters. It seems that this case is similar to animation.parameters object.

However I agree that current polymorphism of parameter object isn't perfect (restrictions were articulated in 1.0.1 to prevent possible runtime errors, while keeping some sort of backwards compatibility with ratified 1.0).

@pjcozzi
Copy link
Member

pjcozzi commented Sep 15, 2016

@mre4ce I had the same question in #92:

We might have discussed this before, but why are attributes and uniforms grouped together? Isn't this harder for the client? GL treats them separately.
-> this change was just introduced in this proposal, that's because the type can be inferred from the program. And that is actually more in line with your suggestion of not redefining the type. So we need to rework this and be consistent.

I think I've seen other formats combine uniforms and attributes into "parameters", but, as a GL developer, I find it awkward since they are so different.

I believe the current design allows parameters to define all inputs to the technique independent of the identifier names in the shader, and then the attributes and uniforms objects map the shader identifiers to these. Is the complexity worth it? I don't know, but I think we should be open to simplifying proposals.

@tparisi
Copy link
Contributor

tparisi commented Sep 15, 2016

I'm in full agreement with the sentiment here. I have always thought that our design was unnecessarily elaborate. I would be in favor of going straight to attributes and uniforms in a future release.

@lexaknyazev
Copy link
Member

I believe the current design allows parameters to define all inputs to the technique independent of the identifier names in the shader, and then the attributes and uniforms objects map the shader identifiers to these. Is the complexity worth it?

I think current complexity level gives almost no benefits, since it provides only 1:1 mapping. Maybe it would be better to split parameters into attributes (or inputs) and uniforms and provide a way to define bindings to name and/or location (latter is more important for Vulkan and WebGL 2.0 profiles).

@pjcozzi
Copy link
Member

pjcozzi commented Sep 15, 2016

@lexaknyazev up to you how you want to handle this, e.g., try to design what is right for Vulkan/WebGL 2.0 right now, try to do something forward compatible, or just wait. If we want to start now, it would be good to see some example JSON for proposals.

At least for GL (can't speak for Vulkan), I would stay with attributes, not inputs.

@mre4ce
Copy link
Author

mre4ce commented Sep 15, 2016

I think "attributes" and "uniforms" is fine for both OpenGL and Vulkan.

I have a glTF loader for Vulkan setup and for uniforms I added:

parameter.stage         // either GL_VERTEX_PROGRAM or GL_FRAGMENT_PROGRAM
parameter.bindingOpenGL // enhanced layout binding for OpenGL
parameter.bindingVulkan // enhanced layout binding for Vulkan, or push constant offset in bytes

Provided my setup is somewhat simplified because I only support the following uniform types:

GPU_PROGRAM_PARM_TYPE_TEXTURE_SAMPLED,                  // texture plus sampler bound together (GLSL: sampler)
GPU_PROGRAM_PARM_TYPE_TEXTURE_STORAGE,                  // not sampled, direct read-write storage (GLSL: image)
GPU_PROGRAM_PARM_TYPE_BUFFER_UNIFORM,                   // read-only uniform buffer (GLSL: uniform)
GPU_PROGRAM_PARM_TYPE_BUFFER_STORAGE,                   // read-write storage buffer (GLSL: buffer)
GPU_PROGRAM_PARM_TYPE_PUSH_CONSTANT_INT,                // int
GPU_PROGRAM_PARM_TYPE_PUSH_CONSTANT_INT_VECTOR2,        // int[2]
GPU_PROGRAM_PARM_TYPE_PUSH_CONSTANT_INT_VECTOR3,        // int[3]
GPU_PROGRAM_PARM_TYPE_PUSH_CONSTANT_INT_VECTOR4,        // int[4]
GPU_PROGRAM_PARM_TYPE_PUSH_CONSTANT_FLOAT,              // float
GPU_PROGRAM_PARM_TYPE_PUSH_CONSTANT_FLOAT_VECTOR2,      // float[2]
GPU_PROGRAM_PARM_TYPE_PUSH_CONSTANT_FLOAT_VECTOR3,      // float[3]
GPU_PROGRAM_PARM_TYPE_PUSH_CONSTANT_FLOAT_VECTOR4,      // float[4]
GPU_PROGRAM_PARM_TYPE_PUSH_CONSTANT_FLOAT_MATRIX2X2,    // float[2][2]
GPU_PROGRAM_PARM_TYPE_PUSH_CONSTANT_FLOAT_MATRIX2X3,    // float[2][3]
GPU_PROGRAM_PARM_TYPE_PUSH_CONSTANT_FLOAT_MATRIX2X4,    // float[2][4]
GPU_PROGRAM_PARM_TYPE_PUSH_CONSTANT_FLOAT_MATRIX3X2,    // float[3][2]
GPU_PROGRAM_PARM_TYPE_PUSH_CONSTANT_FLOAT_MATRIX3X3,    // float[3][3]
GPU_PROGRAM_PARM_TYPE_PUSH_CONSTANT_FLOAT_MATRIX3X4,    // float[3][4]
GPU_PROGRAM_PARM_TYPE_PUSH_CONSTANT_FLOAT_MATRIX4X2,    // float[4][2]
GPU_PROGRAM_PARM_TYPE_PUSH_CONSTANT_FLOAT_MATRIX4X3,    // float[4][3]
GPU_PROGRAM_PARM_TYPE_PUSH_CONSTANT_FLOAT_MATRIX4X4,    // float[4][4]

The downside is that on many platforms the push constants are limited to 256 bytes. However, if you exceed that amount of data in your shader you should probably use an uniform buffer anyway.

I also added:

shader.uriGlslVulkan    // in case you want to use VK_NV_glsl_shader or VK_LAYER_OCULUS_glsl_shader which works on all hardware
shader.uriSpirvOpenGL   // SPIR-V for OpenGL
shader.uriSpirvVulkan   // SPIR-V for Vulkan

The existing shader.uri could be renamed to shader.uriGlslOpenGL but I left the existing name for backwards compatibility. In general I'm not married to any of these names.

@pjcozzi
Copy link
Member

pjcozzi commented Dec 24, 2017

@lexaknyazev @donmccurdy should this influence KHR_technique_webgl in the near-term?

@lexaknyazev
Copy link
Member

should this influence KHR_technique_webgl in the near-term

I think yes.

@lexaknyazev
Copy link
Member

Closing, since KHR_technique_webgl includes this feedback.

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

4 participants