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

Modify the shader so that the line can be colored per vertex #5915

Merged
merged 19 commits into from
Dec 28, 2022
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 15 additions & 2 deletions src/webgl/p5.Geometry.js
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ p5.Geometry = function(detailX, detailY, callback) {
//based on faces for most objects;
this.edges = [];
this.vertexColors = [];
this.lineVertexColors = [];
this.detailX = detailX !== undefined ? detailX : 1;
this.detailY = detailY !== undefined ? detailY : 1;
this.dirtyFlags = {};
Expand All @@ -62,6 +63,7 @@ p5.Geometry.prototype.reset = function() {
this.vertices.length = 0;
this.edges.length = 0;
this.vertexColors.length = 0;
this.lineVertexColors.length = 0;
this.vertexNormals.length = 0;
this.uvs.length = 0;

Expand Down Expand Up @@ -238,12 +240,16 @@ p5.Geometry.prototype._makeTriangleEdges = function() {
* @chainable
*/
p5.Geometry.prototype._edgesToVertices = function() {
const data = this.lineVertexColors.slice();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to make a copy of lineVertexColors or does it not work if we reference it directly? If we do need to copy it, maybe we can call this something more descriptive, like lineColorData

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here, lineVertices are newly created from vertices, but since the color arrangement of lineVertexColors corresponds to vertices, it does not correspond to lineVertices as it is.
So, after making a copy of lineVertexColors corresponding to vertices in advance, we use that data to create an array of colors corresponding to lineVertices. So I thought I should make a copy.
As for the name, lineColorData is certainly easier to understand, so I would like to do so!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It makes sense that lineVertexColors is built using info in lineColorData to correspond to the data in lineVertices. Does the code work if we do const lineColorData = this.lineVertexColors;? So, replacing this.lineVertexColors.slice() with lineVertexColors? Having a separate variable for it is fine if that communicates the intention of the variable more clearly, but I'm just trying to avoid the performance cost of doing an extra copy if we can.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After thinking about it overnight, I realized I didn't need to make a copy, so I'd like to rewrite it! I'm sorry...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to apologize! that's what code review is for :D

this.lineVertexColors.length = 0;
this.lineVertices.length = 0;
this.lineNormals.length = 0;

for (let i = 0; i < this.edges.length; i++) {
const begin = this.vertices[this.edges[i][0]];
const end = this.vertices[this.edges[i][1]];
const e0 = this.edges[i][0];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a little more verbose but maybe we can call these endIndex0 and endIndex1 to be clearer what they represent

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's certainly easier to understand... I think it's better to read and understand even if it's verbose, so I'm going to do it.

const e1 = this.edges[i][1];
var begin = this.vertices[e0];
var end = this.vertices[e1];
const dir = end
.copy()
.sub(begin)
Expand All @@ -260,6 +266,13 @@ p5.Geometry.prototype._edgesToVertices = function() {
dirSub.push(-1);
this.lineNormals.push(dirAdd, dirSub, dirAdd, dirAdd, dirSub, dirSub);
this.lineVertices.push(a, b, c, c, b, d);
if(data.length > 0){
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Super minor, but do you mind adding spaces around the brackets to match the style of the if statements elsewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It sure looks better with some space in it... ok!

var beginColor = [data[4*e0], data[4*e0+1], data[4*e0+2], data[4*e0+3]];
var endColor = [data[4*e1], data[4*e1+1], data[4*e1+2], data[4*e1+3]];
this.lineVertexColors.push(
beginColor, beginColor, endColor, endColor, beginColor, endColor
);
}
}
return this;
};
Expand Down
9 changes: 9 additions & 0 deletions src/webgl/p5.RendererGL.Immediate.js
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,13 @@ p5.RendererGL.prototype.vertex = function(x, y) {
vertexColor[2],
vertexColor[3]
);
var lineVertexColor = this.curStrokeColor || [0.5, 0.5, 0.5, 1];
this.immediateMode.geometry.lineVertexColors.push(
lineVertexColor[0],
lineVertexColor[1],
lineVertexColor[2],
lineVertexColor[3]
);

if (this.textureMode === constants.IMAGE) {
if (this._tex !== null) {
Expand Down Expand Up @@ -405,6 +412,8 @@ p5.RendererGL.prototype._drawImmediateFill = function() {
p5.RendererGL.prototype._drawImmediateStroke = function() {
const gl = this.GL;
const shader = this._getImmediateStrokeShader();
this._useLineColor =
(this.immediateMode.geometry.lineVertexColors.length > 0);
this._setStrokeUniforms(shader);
for (const buff of this.immediateMode.buffers.stroke) {
buff._prepareBuffer(this.immediateMode.geometry, shader);
Expand Down
1 change: 1 addition & 0 deletions src/webgl/p5.RendererGL.Retained.js
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,7 @@ p5.RendererGL.prototype.drawBuffers = function(gId) {

if (this._doStroke && geometry.lineVertexCount > 0) {
const strokeShader = this._getRetainedStrokeShader();
this._useLineColor = (geometry.model.lineVertexColors.length > 0);
this._setStrokeUniforms(strokeShader);
for (const buff of this.retainedMode.buffers.stroke) {
buff._prepareBuffer(geometry, strokeShader);
Expand Down
5 changes: 5 additions & 0 deletions src/webgl/p5.RendererGL.js
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,8 @@ p5.RendererGL = function(elt, pInst, isMainCanvas, attr) {
this._useNormalMaterial = false;
this._useShininess = 1;

this._useLineColor = false;

this._tint = [255, 255, 255, 255];

// lightFalloff variables
Expand Down Expand Up @@ -149,6 +151,7 @@ p5.RendererGL = function(elt, pInst, isMainCanvas, attr) {
geometry: {},
buffers: {
stroke: [
new p5.RenderBuffer(4, 'lineVertexColors', 'lineColorBuffer', 'aVertexColor', this, this._flatten),
new p5.RenderBuffer(3, 'lineVertices', 'lineVertexBuffer', 'aPosition', this, this._flatten),
new p5.RenderBuffer(4, 'lineNormals', 'lineNormalBuffer', 'aDirection', this, this._flatten)
],
Expand Down Expand Up @@ -184,6 +187,7 @@ p5.RendererGL = function(elt, pInst, isMainCanvas, attr) {
new p5.RenderBuffer(2, 'uvs', 'uvBuffer', 'aTexCoord', this, this._flatten)
],
stroke: [
new p5.RenderBuffer(4, 'lineVertexColors', 'lineColorBuffer', 'aVertexColor', this, this._flatten),
new p5.RenderBuffer(3, 'lineVertices', 'lineVertexBuffer', 'aPosition', this, this._flatten),
new p5.RenderBuffer(4, 'lineNormals', 'lineNormalBuffer', 'aDirection', this, this._flatten)
],
Expand Down Expand Up @@ -1257,6 +1261,7 @@ p5.RendererGL.prototype._setStrokeUniforms = function(strokeShader) {
strokeShader.bindShader();

// set the uniform values
strokeShader.setUniform('uUseLineColor', this._useLineColor);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks!('ω')

strokeShader.setUniform('uMaterialColor', this.curStrokeColor);
strokeShader.setUniform('uStrokeWeight', this.curStrokeWeight);
};
Expand Down
7 changes: 5 additions & 2 deletions src/webgl/shaders/line.frag
Original file line number Diff line number Diff line change
@@ -1,8 +1,11 @@
precision mediump float;
precision mediump int;

varying vec4 vColor;

uniform bool uUseLineColor;
uniform vec4 uMaterialColor;

void main() {
gl_FragColor = uMaterialColor;
}
gl_FragColor = (uUseLineColor ? vColor : uMaterialColor);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can maybe avoid doing this in the fragment shader if, in the vertex shader, we set vColor as either uUseLineColor ? aVertexColor : uMaterialColor. This is a pretty insignificant optimization, but there are usually more fragments than vertices, so it's marginally faster to do if statements and such in the vertex shader and do less checks in the fragment shader (but again, it's just one if statement, so this is more for "best practices" than for a measurable gain)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, the vertexShader receives uMaterialColor and writes the branch processing...certainly, considering the number of calls, that might be better...Okay, I'll give it a try!

}
7 changes: 6 additions & 1 deletion src/webgl/shaders/line.vert
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,10 @@ uniform int uPerspective;

attribute vec4 aPosition;
attribute vec4 aDirection;

attribute vec4 aVertexColor;

varying vec4 vColor;

void main() {
// using a scale <1 moves the lines towards the camera
// in order to prevent popping effects due to half of
Expand Down Expand Up @@ -94,4 +97,6 @@ void main() {

gl_Position.xy = p.xy + offset.xy * curPerspScale;
gl_Position.zw = p.zw;

vColor = aVertexColor;
}