Buffer binding #35

davepagurek · 2017-11-12T04:17:03Z

armcburney · 2017-11-12T04:42:39Z

src/shaderpipeline.ts

@@ -1,11 +1,149 @@
 import Shader from './shader';
+import Qualifier from './qualifier';
+import Set from './util/set';
+import InterfaceVariable from './interface';


Nitpick: could you do these imports alphabetically, please? (just a convention I like)

abhimadan

Some of my comments refer to #36, in case that gets merged first. I don't mind either way, but we should talk about how to resolve the merge conflicts beforehand, since both PRs are pretty large.

abhimadan · 2017-11-12T04:45:51Z

src/interface.ts

@@ -23,4 +23,20 @@ export default class InterfaceVariable implements Hashable {
    public hashCode(): string {
        return this.declaration();
    }
+
+    public wrapAttributeValue(value: any[]): any {


Can you rename wrapAttributeValue to something more descriptive, like wrapAttributeBufferInTypedArray? It took a while to figure out what this was actually doing.

abhimadan · 2017-11-12T04:50:28Z

src/shader.ts

@@ -22,18 +22,21 @@ export default class Shader {
            .join('\n');
    }

-    public inputs(): Set<Variable> {
+    public inputs(): Set<InterfaceVariable> {


In my pending PR for adding local variables, I remove dependencies and just use inputs() and outputs() to get VariableSources (just the variable name and type). There are other fields inputDecls and outputDecls which contain VariableDeclaration[]s to use in code generation. If I submit first, you can change these to inputVars() and outputVars() and use them the way you are now (but you would need to cast to get back that they're InterfaceVariables, so maybe there should be InterfaceVariableDeclaration and LocalVariableDeclaration nodes?)

abhimadan · 2017-11-12T05:00:53Z

src/shaderpipeline.ts

+        this.attributeBuffers = new Map();
+        this.uniformPositions = new Map();
+        this.inputs = new Map();
+        [...vertexShader.inputs().union(fragmentShader.inputs())].forEach(input => {


Do we expect there to be common inputs between vertex and fragment shaders? I think fragment shader inputs come from vertex shader outputs. We don't bind buffers for those inputs.

This only does stuff for uniforms, which you get for both, and attributes, which should only be inputs for the vertex shader (but we don't check that), and currently ignores in and out variables. Haven't thought too much yet about those but there aren't buffers being made for them anyway.

Right, that makes sense. Although I still don't understand the purpose of the inputs map, which takes inputs from both the fragment and vertex shader, and doesn't check whether or not those inputs are attributes. If you're only using it for attribute variables, can you
a) just get inputs from the vertex shader, and
b) filter by qualifier so that you only add attributes to the map?

abhimadan · 2017-11-12T05:09:36Z

src/shaderpipeline.ts

+        this.createBuffers();
+    }
+
+    public setAttribute(input: string, value: any[], usage: GLenum = this.gl.STATIC_DRAW) {


It seems awkward to key input interface variables by their declaration string. Why can't we just iterate over the set of inputs?

Also, can we change the signature of this to take all input variables, and all buffers? Maybe something like this:

public setAttributes(inputVars: InterfaceVariable[], buffer: any[], usage: GLenum);

If we do this, we can figure out the stride and offset for each attribute.

This is for the end user to bind a value to a variable, which they may not want to do all at once, e.g. shader.bindAttribute('position', [1, -1, 0, ... ]). From the user's perspective, they'd be naming their variables by strings anyway, so wouldn't it be weird to make them pass in an object as a key?

I thought the map was keyed by declaration for some reason, which is why I thought it was awkward. I'm okay with just variable names, because as you said, they're naming these variables with strings anyway.

Can we also support binding for multiple attributes at once (taking a string[] instead of InterfaceVariable[])? Binding separately in this way with multiple buffers will be less performant than interleaving the data if we want multiple attributes, so it's good to at least leave the option open.

Also, FWIW, I think we need to think more carefully about what buffer-related work we want users to do outside of our library. Right now, they have to gather each attribute's data in separate buffers, which is simple on their end, but leads to inefficient GPU cache use. On the other hand, they gather every attribute's data in a single buffer and pass it in to avoid cache problems, but this might be too restrictive to use. However, I'm fine with your current approach, and I'll make a separate issue for this.

abhimadan · 2017-11-12T05:11:54Z

src/shaderpipeline.ts

+        this.gl.enableVertexAttribArray(position);
+    }
+
+    // TODO: support variadic args


Out of curiosity, why do we want variadic args?

We don't need to, but there are the non-v versions of all the uniform functions that maybe people want to call directly instead of wrapping in an array, so our call might mirror that. Definitely not a key feature or anything, just something that we might consider doing in the name of not limiting the library's usage

abhimadan · 2017-11-12T05:12:38Z

src/shaderpipelinebuilder.ts

@@ -15,13 +17,13 @@ export default class ShaderPipelineBuilder {

        // Vertex shader outputs must be a superset of fragment shader inputs.
        // Might want to enforce equality (need to check the standard), but for now, this should be sufficient.
-        const inVars = this.fragmentShader.inputs();
-        const outVars = this.vertexShader.outputs();
+        const inVars = new Set<Variable>([...this.fragmentShader.inputs()].map(v => v.variable));


My pending PR will redefine inputs and outputs to return what you want here, so you won't need a map.

abhimadan · 2017-11-12T05:13:01Z

src/type.ts

@@ -31,4 +31,85 @@ export default class Type {

        return true;
    }
+
+    public wrapAttributeValue(value: any[]): any {


Same comment as above: can you rename to wrapAttributeBufferInTypedArray?

abhimadan · 2017-11-12T22:16:47Z

src/shaderpipeline.ts

+            .filter(input => input.variable.qualifier == Qualifier.Attribute)
+            .forEach(input => this.attributes.set(input.variable.name(), input.variable));
+        this.uniforms = new Set(
+            [...vertexShader.inputDecls]


This implies we're going to expect users to pass in uniforms to the inputs parameter in a Shader constructor. Would it be better to have a separate uniforms constructor parameter? Shader inputs and uniforms are used for different purposes (vertex/fragment-specific vs static for entire pipeline), so it could be less confusing to split them at the Shader level.

I want to think about this more in another PR I think

davepagurek · 2017-11-13T03:47:23Z

Ok so changes I made:

It's no longer just the vertex shader's inputs. The fragment shader can reference uniforms that the vertex shader doesn't because the vertex shader might not care about them. The uniforms can maybe be a separate section in the shader but I'm going to leave that until later.
Matrices multiplied by vectors is allowed, but throws an error right now, so I commented out the error and those tests until we fix.
Added code to build samples for a browser
Have to specify float precision in shaders. TODO: allow this to change.

armcburney

abhimadan · 2017-11-13T06:27:40Z

src/expressions/assignment/equal_assignment.ts

@@ -8,6 +8,7 @@ export default class EqualAssignment extends AssignmentExpression {
    }

    public source(): string {
+        console.log(this);


Remove this console.log?

armcburney reviewed Nov 12, 2017

View reviewed changes

abhimadan suggested changes Nov 12, 2017

View reviewed changes

davepagurek force-pushed the buffer-binding branch from 7dcc8d3 to d0d2e48 Compare November 12, 2017 22:11

abhimadan reviewed Nov 12, 2017

View reviewed changes

abhimadan mentioned this pull request Nov 12, 2017

Improve interface for buffer binding #43

Open

davepagurek force-pushed the buffer-binding branch from d5d9495 to cb211ce Compare November 13, 2017 03:42

davepagurek added 10 commits November 12, 2017 22:44

Initial gl setup

8e447da

Add initial buffer binding

cdbe92d

Add draw and useProgram

648c14a

Refactor

7b73ffd

Alphabetize imports

73de499

Fix error

c1fbedb

Add sample script

2edd0ea

Semi working shader

26977dd

Working rectangle

83f6c9e

Remove extra logging

1366a7b

davepagurek force-pushed the buffer-binding branch from 851453d to 1366a7b Compare November 13, 2017 03:44

Test changes

0a6f898

armcburney approved these changes Nov 13, 2017

View reviewed changes

abhimadan reviewed Nov 13, 2017

View reviewed changes

abhimadan approved these changes Nov 13, 2017

View reviewed changes

Remove logging

2d5cd2b

davepagurek merged commit 85aed02 into master Nov 13, 2017

davepagurek deleted the buffer-binding branch November 13, 2017 14:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Buffer binding #35

Buffer binding #35

davepagurek commented Nov 12, 2017 •

edited

Loading

armcburney Nov 12, 2017

abhimadan left a comment

abhimadan Nov 12, 2017

abhimadan Nov 12, 2017

abhimadan Nov 12, 2017

davepagurek Nov 12, 2017 •

edited

Loading

abhimadan Nov 12, 2017

abhimadan Nov 12, 2017

davepagurek Nov 12, 2017 •

edited

Loading

abhimadan Nov 12, 2017

abhimadan Nov 12, 2017

davepagurek Nov 12, 2017

abhimadan Nov 12, 2017

abhimadan Nov 12, 2017

abhimadan Nov 12, 2017

davepagurek Nov 13, 2017

davepagurek commented Nov 13, 2017 •

edited

Loading

armcburney left a comment

abhimadan Nov 13, 2017

Buffer binding #35

Buffer binding #35

Conversation

davepagurek commented Nov 12, 2017 • edited Loading

Choose a reason for hiding this comment

abhimadan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davepagurek Nov 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davepagurek Nov 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davepagurek commented Nov 13, 2017 • edited Loading

armcburney left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davepagurek commented Nov 12, 2017 •

edited

Loading

davepagurek Nov 12, 2017 •

edited

Loading

davepagurek Nov 12, 2017 •

edited

Loading

davepagurek commented Nov 13, 2017 •

edited

Loading