Round one of changing shaders for data-driven styling #1257
Conversation
| for (var i = 0; i < elementGroups.groups.length; i++) { | ||
| vertex.bind(gl); |
There was a problem hiding this comment.
Is this call important? Why wasn't it here before?
There was a problem hiding this comment.
@ansis Can you explain what's going on here?
There was a problem hiding this comment.
I reckon this is only necessary if we are actually binding a buffer to the attribute.
There was a problem hiding this comment.
I don't think the call is necessary either way
|
@jfirebaugh What's the strategy here? Do you want to merge this PR once it gets 👍? Or do you want to do all shader changes in one PR? Or do you want to have everything end-to-end in one PR? I'd like to avoid doing this in a separate branch, as long as we can maintain backwards compatibility. |
|
Yes, let's get as far as we can with short-lived, backwards compatible branches. But let's also make all the uniform ⇢ attribute changes at once, so we can gauge the full performance effect, if any. @ansis Can you take the lead on code reviewing shader changes? |
|
Got it @jfirebaugh. I'll keep hacking on that today. |
|
I'm going to focus on support for the these style properties first because they are paint styles that only require simple shader changes
|
lucaswoj
changed the title
| // - every shader has an `a_pos` attribute | ||
| // | ||
| // see: https://developer.mozilla.org/en-US/docs/Web/WebGL/WebGL_best_practices | ||
| this.bindAttribLocation(shader.program, 0, 'a_pos'); |
There was a problem hiding this comment.
@jfirebaugh @ansis @kkaefer Do you think this is sufficiently robust and explanatory for now? Or would you architect this differently?
|
Ok, I think this is ready for review! This PR includes all the straightforward shader modifications for data driven styling. This means we only support a subset of the style properties we eventually want to support. The style properties enabled by this pull request are listed (above)[https://github.com//pull/1257#issuecomment-110443499]. The more complex changes needed to support other style properties (they all need different sorts of changes) should be ticketed out done in separate pull requests from here. |
|
Per if is looking great! (without per-vertex buffers) |
|
looks good! can you squash some of the lint and whitespace fixes? |
|
@ansis Yes, I'll take a swing at it. I almost want to squash the whole PR into one commit... Its pretty messy. Call it a "learning experience." |
…ibs after disabling
… don't do smart diffing
|
Ok. I removed the egregious lint and whitespace fix commits. |
|
Awesome. |
|
🎆 |
The first step to data driven styling is refactoring the shaders to allow per-feature colors. This pull request makes all three line shaders compatible with this functionality. It does not seem to degrade performance measurably -- all benchmarks still run at 60fps.
cc @jfirebaugh @ansis