Constant attribute does not update

[dy]

Consider simple setup:

let regl = require('regl')()

let draw = regl({
	attributes: {
		pos: [0,0, .1,.1],
		active: regl.prop('active')
	},
	depth: {enable: false},
	count: 2,
	primitive: 'points',

	frag: `void main() {gl_FragColor = vec4(0,0,0,1);}`,

	vert: `
	precision highp float;

	attribute vec2 pos;
	attribute float active;

	void main() {
	  if (active == 0.) return;

	  gl_PointSize = 10.;
	  gl_Position = vec4(pos, .5, 1);
	}
	`
})

draw({active: [0,1]})

draw({active: {constant: [1]}})

regl.clear({color: true})

// uncomment this to fix issue
// regl._refresh()
draw({active: [1,0]})

The last draw call does not recognize new active array and keeps constant attribute [1].
The need to insert regl._refresh() reduces performance in plotly regl-* components (around 6 times for regl-splom).

Related: plotly/plotly.js#2586, possibly #427.

[rreusser]

This seems very similar to #483. In that issue, attributes are left in a possibly-active state so that if you destroy the buffers previous commands have referenced, webgl complains, even though the attributes and now-destroyed buffers are unused for the current command. It seems like the checks near lib/core.js:2250 might need to be beefed up a bit in order to catch more subtle changes.

[rreusser]

Nope. Only superficially similar. #483 was fixed long ago, while this remains an issue.

[rreusser]

@dy After (finnallly) digging into this a fair amount, I believe the issue is that the decision about whether an attribute is constant or not seems to be made here in the parseAttributes section:

regl/lib/core.js

Line 1623 in 6204ca3

function parseAttributes (attributes, env) {

while that state is then used in the emitAttributes section:

regl/lib/core.js

Lines 2296 to 2307 in 6204ca3

	if (STATE === ATTRIB_STATE_POINTER) {
	emitBuffer()
	} else if (STATE === ATTRIB_STATE_CONSTANT) {
	emitConstant()
	} else {
	scope('if(', STATE, '===', ATTRIB_STATE_POINTER, '){')
	emitBuffer()
	scope('}else{')
	emitConstant()
	scope('}')
	}
	}

The result is that I think the (in my opinion very highly desirable) decision of whether an attribute is constant or not simply can't be made at runtime.

The problem then is that it doesn't prevent you from trying and (I haven't totally debugged this part but) it seems like the state gets corrupted.

So strictly speaking what you've tried to do is invalid usage and it just happens that refreshing the state resets things to what in this particular case is the desired state—but it would be very easy to come up with a situation in which it resets things to what's not the desired state.

Personally I would find deferring the decision until runtime to be highly desirable. This would allow very useful abstraction like writing commands that are agnostic to whether a color, for example, is an attribute or a uniform. I'd like to modify the structure to defer the decision, but I'm interested in whether @mikolalysenko thinks the penalty (performance, complexity, or otherwise) might be prohibitive.

[rreusser]

Oh. I might be totally wrong about that. There clearly is some runtime allowance if the state is neither pointer nor const. Okay, investigating how to get to that branch. Maybe a dynamic property……

[rreusser]

Okay, yup, I spoke too soon. The problem appears to be this conditional:

regl/lib/core.js

Line 2239 in 6204ca3

'if(!', BINDING, '.buffer){',

That appears to be preventing it from reenabling vertex attrib array when the state is switched away from constant. Simply removing the conditional fixes it, but there's probably a way to target it a bit more precisely.

[rreusser]

Oh. It just needs an else with disableVertexAttribArray otherwise it will never get disabled, which is the whole problem. That will get called quite a bit so that it would make sense to track the state and avoid calling when unneeded.

[rreusser]

Oof, the more I look at this, the more I start to understand what it's trying to do, but what it's trying to do actually seems to actually make sense, just not work. Okay, we can get this.

[rreusser]

Okay, had to agonize over this a bit, but finally got to the bottom of it. I've merged it in #545 and have published v1.3.12 to npm. It's too bad travis isn't working anymore (there's a successor to headless-gl now, isn't there??), but I feel pretty good about it and made sure to get all the tests passing (including odd extension interaction), so I went ahead and published. 🎉

And have confirmed that npm installing regl and running the original code in this issue now produces the desired result 🎉🎉🎉🎉🎉

[dy]

As for the tests, we can try to set up tape-run, it runs them in electron.
The example in fast-on-load.