Modify bezierVertex() and quadraticVertex() so that curFillColor becomes an interpolated value #5920

inaridarkfox4231 · 2022-12-28T18:40:45Z

Inside the vertex functions called inside the bezierVertex and quadraticVertex functions, curFillColor is now all the same.
I want this to be interpolated between the curFillColor value when the previous vertex function was called (external or internal) and the current curFillColor value.

Resolves #5919

Changes:

Both bezierVertex and quadraticVertex functions must always be executed after at least one vertex function has been called. So the vertexColors array is always non-empty and we can get the previous color. Rewrite it like that.
In addition, we want to interpolate based on the length of the polygonal line that connects the control points, and then determine the color at each point from the color assigned to the control point and the Bezier coefficient.

Screenshots of the change:

PR Checklist

WEBGL

Inside the vertex functions called inside the bezierVertex and quadraticVertex functions, curFillColor is now all the same. We want this to be interpolated between the curFillColor value when the previous vertex function was called (external or internal) and the current curFillColor value.

I fixed some unnecessary spaces and lines that were too long.

Some lines were too long, so I shortened them.

Since sqrt cannot be referenced from this file, I rewrote it to Math.sqrt.

inaridarkfox4231 · 2022-12-29T01:30:23Z

I was using the name previousColor, but since it's the end of the array, I thought lastColor would be more appropriate, so I renamed it.

davepagurek

Thanks for taking this on! I have a few comments, all of which are minor, and are just about the code style. Let me know what you think!

And then afterwards we can maybe add a unit test similar to what we did for the line color interpolation. It doesn't need to be as exact as for line color, just something that will let us know if an unrelated code change accidentally changes the behaviour of bezier vertex fills.

davepagurek · 2022-12-29T15:00:50Z

src/webgl/3d_primitives.js

+    // Get the last fill color
+    const _vertexColors = this.immediateMode.geometry.vertexColors;
+    const _vertexColorSize = _vertexColors.length;
+    const lastFillColor = [


This might be more concise if we do this.immediateMode.geometry.vertexColors.slice(-4) to get the last 4 elements. Do we need to handle this array being empty? I think immediate mode always copies the fill color into this array, so probably not, right?

That's more concise...I'd like to fix it! thank you.

davepagurek · 2022-12-29T15:03:05Z

src/webgl/3d_primitives.js

    if (argLength === 6) {
      this.isBezier = true;

      w_x = [this.immediateMode._bezierVertex[0], args[0], args[2], args[4]];
      w_y = [this.immediateMode._bezierVertex[1], args[1], args[3], args[5]];
+      // calCalculate intermediate colors


Maybe we can also describe in this comment, at a high level, the strategy we're using to pick intermediate colours? Something about using the fraction of the distance along the control points

I see. For example, "The ratio of the distances between the start point, the two control points, and the end point determines the intermediate color."?
Certainly, I think it would be easier to understand if you touched on the ratio!

yeah that sounds good!

davepagurek · 2022-12-29T15:04:40Z

src/webgl/3d_primitives.js

    if (argLength === 6) {
      this.isBezier = true;

      w_x = [this.immediateMode._bezierVertex[0], args[0], args[2], args[4]];
      w_y = [this.immediateMode._bezierVertex[1], args[1], args[3], args[5]];
+      // calCalculate intermediate colors
+      let d0 = Math.sqrt(


We can use Math.hypot() here instead of sqrt of sums of pows to be a little more concise

I didn't know Math.hypot existed...I think I'll use it because it's convenient!

davepagurek · 2022-12-29T15:07:32Z

src/webgl/3d_primitives.js

+      const secondFillColor = [];
+      for (k = 0; k < 4; k++) {
+        firstFillColor.push(
+          lastFillColor[k] * (1-d0) + finalFillColor[k] * d0


davepagurek · 2022-12-29T15:11:23Z

src/webgl/3d_primitives.js

+        this.curFillColor = [0, 0, 0, 0];
+        for (k = 0; k < 4; k++) {
+          this.curFillColor[k] +=
+            this._lookUpTableBezier[i][0] * lastFillColor[k];


I wonder if there are different names we can use here, as it's maybe a tad unintuitive for readers if "first" is the first control point but not the first point in the bezier's hull, and also that it starts with "last" (your explanation for using last instead of previous makes sense, but still might not be what people first assume this variable represents.) Maybe we could even avoid naming each color individually if we just make an array of all four colors? Then lastFillColor[k] would be fillColors[0][k] (and we could loop over them if we wanted, too.)

Certainly, it may not be necessary to prepare four arrays... I thought that the meaning of first might be difficult to understand, so I'm thinking of rewriting it in the form of an array.

davepagurek · 2022-12-29T15:12:34Z

src/webgl/3d_primitives.js

@@ -1633,6 +1724,7 @@ p5.RendererGL.prototype.bezierVertex = function(...args) {
          w_z[3] * this._lookUpTableBezier[i][3];
        this.vertex(_x, _y, _z);
      }
+      this.curFillColor = finalFillColor; // Set curFillColor to finalFillColor


Maybe instead of the comment describing what this line does, we could instead describe why it needs to do that? e.g. so that we leave curFillColor with the last value the user set it to?

My comment here was too easy...I'll fix it!

There was a part where it was difficult to understand what kind of processing was being done, so I rewrote the comment to make it easier to understand. Also, some processing has been simplified using convenient functions.

inaridarkfox4231 · 2022-12-29T17:33:15Z

Thank you for your advice! I felt that it was difficult to understand fillColors if it was just an array, so I decided to add what each component means. I will do my best to make a unit test as well.

davepagurek · 2022-12-29T20:40:47Z

Using an array but with comments is a great compromise, thanks!

The quadraticVertex has only one control point, so I reworded the comment accordingly.

inaridarkfox4231 · 2022-12-29T23:53:11Z

thank you! Edited my comment a bit...

I have a question, can I rewrite this for loop part like this?
before:

for (i = 0; i < LUTLength; i++) {
  // Interpolate colors using control points
  this.curFillColor = [0, 0, 0, 0];
  for (m = 0; m < 4; m++) {
    for (k = 0; k < 4; k++) {
      this.curFillColor[k] +=
        this._lookUpTableBezier[i][m] * fillColors[m][k];
    }
  }
  _x =
    w_x[0] * this._lookUpTableBezier[i][0] +
    w_x[1] * this._lookUpTableBezier[i][1] +
    w_x[2] * this._lookUpTableBezier[i][2] +
    w_x[3] * this._lookUpTableBezier[i][3];
  _y =
    w_y[0] * this._lookUpTableBezier[i][0] +
    w_y[1] * this._lookUpTableBezier[i][1] +
    w_y[2] * this._lookUpTableBezier[i][2] +
    w_y[3] * this._lookUpTableBezier[i][3];
  this.vertex(_x, _y);
}

after:

for (i = 0; i < LUTLength; i++) {
  // Interpolate colors using control points
  this.curFillColor = [0, 0, 0, 0];
  _x = _y = 0;
  for (m = 0; m < 4; m++) {
    for (k = 0; k < 4; k++) {
      this.curFillColor[k] +=
        this._lookUpTableBezier[i][m] * fillColors[m][k];
    }
    _x += w_x[m] * this._lookUpTableBezier[i][m];
    _y += w_y[m] * this._lookUpTableBezier[i][m];
  }
  this.vertex(_x, _y);
}

I'm reluctant to rewrite the original code, but this seems more concise...By the way, I prepared m as a variable to cross 4 or 3 points in the loop.

inaridarkfox4231 · 2022-12-30T01:12:28Z

Also, I created a unit test like this:
bezierFill test
quadraticFill test

test('bezierVertex() should interpolate colors between vertices', function(done) {
  const renderer = myp5.createCanvas(256, 256, myp5.WEBGL);

  // start color: (255, 255, 255)
  // end color: (255, 0, 0)
  // Intermediate values are expected to be approximately half the value.

  renderer.noStroke();
  renderer.beginShape();
  renderer.fill(255);
  renderer.vertex(-128, -128);
  renderer.fill(255, 0, 0);
  renderer.bezierVertex(128, -128, 128, 128, -128, 128);
  renderer.endShape();

  assert.deepEqual(myp5.get(128, 128), [255, 129, 129, 255]);

  done();
});

test('quadraticVertex() should interpolate colors between vertices', function(done) {
  const renderer = myp5.createCanvas(256, 256, myp5.WEBGL);

  // start color: (255, 255, 255)
  // end color: (255, 0, 0)
  // Intermediate values are expected to be approximately half the value.

  renderer.noStroke();
  renderer.beginShape();
  renderer.fill(255);
  renderer.vertex(-128, -128);
  renderer.fill(255, 0, 0);
  renderer.quadraticVertex(256, 0, -128, 128);
  renderer.endShape();

  assert.deepEqual(myp5.get(128, 128), [255, 128, 128, 255]);

  done();
});

Should this test be in beginShape() of p5.Renderer.GL like it is for interpolating line colors? Or should it be on the 3d_primitives side...

davepagurek · 2022-12-30T15:04:54Z

I have a question, can I rewrite this for loop part like this?

That rewrite looks good to me!

Should this test be in beginShape() of p5.Renderer.GL like it is for interpolating line colors? Or should it be on the 3d_primitives side...

Maybe 3d_primitives since it's just supplying color data that the renderer then interpolates along the line segments?

davepagurek · 2022-12-30T15:05:28Z

Oh also, one other thought, should we be interpolating stroke color similarly to how we're interpolating fill color now?

inaridarkfox4231 · 2022-12-30T15:14:21Z

It certainly looks like it could be implemented in the same way... I'll try!
(Actually, I was thinking of interpolating the line colors after the method of color interpolation was established, but since it's this time, I'll do it all at once.)
As for the unit test, I'm just changing the internal specifications of the function, so 3d_primitives looks good... I'll put it there!

davepagurek · 2022-12-30T15:25:20Z

As long as this change doesn't break line colors, feel free to implement it as a separate change if that's easier, whichever you're more comfortable with!

I thought it would be possible to simplify by nesting some of the internal processing of the for-loop that performs the interpolation processing, so I rewrote it. I added the roles of the variables used in the loop in the form of comments. Also, I thought that "control" was more appropriate than "intermediate", so I renamed it.

inaridarkfox4231 · 2022-12-30T16:04:43Z

In the same way, if I prepared strokeColors, I could also interpolate the color of the line, so I'm going to implement it.

// fillColors[0]: start point color
// fillColors[1],[2]: control point color
// fillColors[3]: end point color
const fillColors = [];
for (m = 0; m < 4; m++) fillColors.push([]);
fillColors[0] = this.immediateMode.geometry.vertexColors.slice(-4);
fillColors[3] = this.curFillColor.slice();

// Do the same for strokeColor.
const strokeColors = [];
for (m = 0; m < 4; m++) strokeColors.push([]);
strokeColors[0] = this.immediateMode.geometry.lineVertexColors.slice(-4);
strokeColors[3] = this.curStrokeColor.slice();
etc...

source code:

function setup(){
  createCanvas(640, 640, WEBGL);

  noFill();
  strokeWeight(3);
  beginShape();
  stroke(255);
  vertex(-160, -160);
  stroke(255, 0, 0);
  bezierVertex(160, -160, 160, 0, -160, 0);
  stroke(0, 0, 255);
  quadraticVertex(0, 160, 0, 160, 0, 0);
  stroke(255);
  bezierVertex(160, 320, 0, -160, 320, 0, -160, 80, 0);
  endShape();
}

Also, regarding the unit test, I'm not sure how to put it in 3d_primitives, so I'd like to put it in beginShape() of p5.Renderer.GL... is it OK?

Exactly the same form of interpolation is applied to curStrokeColor as for curFillColor.

davepagurek · 2022-12-30T16:37:47Z

Those gradient strokes look great!

And sure thing, the RendererGL tests are fine too.

inaridarkfox4231 · 2022-12-30T16:52:40Z

Thank you! Then, I would like to put it in p5.Renderer.GL.

Add a unit test to make sure intermediate vertex colors are interpolated with start and end colors when using bezierVertex and quadraticVertex in immediateMode.

noStroke() was not a renderer function. Prior to that, the noStroke function was unnecessary for this test, so I decided to remove it.

davepagurek

Thanks for all your hard work, this looks great!

inaridarkfox4231 · 2023-01-03T20:01:22Z

thanks very much, too! ('ω')
Happy new year!

inaridarkfox4231 added 4 commits December 29, 2022 03:28

Cleaned up some spaces and shorten some lines.

3b75ee8

I fixed some unnecessary spaces and lines that were too long.

shorten some lines

0cbf66a

Some lines were too long, so I shortened them.

Change sqrt, pow to Math.sqrt, Math.pow

46ff92c

Since sqrt cannot be referenced from this file, I rewrote it to Math.sqrt.

inaridarkfox4231 changed the title ~~Modify bezierVertex() and quadraticVertex(). so that curFillColor becomes an interpolated value~~ Modify bezierVertex() and quadraticVertex() so that curFillColor becomes an interpolated value Dec 29, 2022

davepagurek reviewed Dec 29, 2022

View reviewed changes

Clarified comments and simplified some processing

b2c4dbd

There was a part where it was difficult to understand what kind of processing was being done, so I rewrote the comment to make it easier to understand. Also, some processing has been simplified using convenient functions.

Minor comment correction

4e9225d

The quadraticVertex has only one control point, so I reworded the comment accordingly.

Add process related to line color interpolation

6e81bb2

Exactly the same form of interpolation is applied to curStrokeColor as for curFillColor.

inaridarkfox4231 added 2 commits December 31, 2022 01:52

test for fill with bezier and quadraticVertex

25a799f

Add a unit test to make sure intermediate vertex colors are interpolated with start and end colors when using bezierVertex and quadraticVertex in immediateMode.

Eliminate unnecessary noStroke functions

6633fad

noStroke() was not a renderer function. Prior to that, the noStroke function was unnecessary for this test, so I decided to remove it.

davepagurek approved these changes Jan 3, 2023

View reviewed changes

davepagurek merged commit 4717214 into processing:main Jan 3, 2023

inaridarkfox4231 deleted the inaridarkfox4231-bezierFill branch January 3, 2023 19:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modify bezierVertex() and quadraticVertex() so that curFillColor becomes an interpolated value #5920

Modify bezierVertex() and quadraticVertex() so that curFillColor becomes an interpolated value #5920

inaridarkfox4231 commented Dec 28, 2022

inaridarkfox4231 commented Dec 29, 2022

davepagurek left a comment

davepagurek Dec 29, 2022

inaridarkfox4231 Dec 29, 2022

davepagurek Dec 29, 2022

inaridarkfox4231 Dec 29, 2022

davepagurek Dec 29, 2022

davepagurek Dec 29, 2022

inaridarkfox4231 Dec 29, 2022

davepagurek Dec 29, 2022

inaridarkfox4231 Dec 29, 2022

davepagurek Dec 29, 2022

inaridarkfox4231 Dec 29, 2022

davepagurek Dec 29, 2022

inaridarkfox4231 Dec 29, 2022

inaridarkfox4231 commented Dec 29, 2022

davepagurek commented Dec 29, 2022

inaridarkfox4231 commented Dec 29, 2022

inaridarkfox4231 commented Dec 30, 2022

davepagurek commented Dec 30, 2022

davepagurek commented Dec 30, 2022

inaridarkfox4231 commented Dec 30, 2022

davepagurek commented Dec 30, 2022

inaridarkfox4231 commented Dec 30, 2022

davepagurek commented Dec 30, 2022

inaridarkfox4231 commented Dec 30, 2022

davepagurek left a comment

inaridarkfox4231 commented Jan 3, 2023

Modify bezierVertex() and quadraticVertex() so that curFillColor becomes an interpolated value #5920

Modify bezierVertex() and quadraticVertex() so that curFillColor becomes an interpolated value #5920

Conversation

inaridarkfox4231 commented Dec 28, 2022

Changes:

Screenshots of the change:

PR Checklist

inaridarkfox4231 commented Dec 29, 2022

davepagurek 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

inaridarkfox4231 commented Dec 29, 2022

davepagurek commented Dec 29, 2022

inaridarkfox4231 commented Dec 29, 2022

inaridarkfox4231 commented Dec 30, 2022

davepagurek commented Dec 30, 2022

davepagurek commented Dec 30, 2022

inaridarkfox4231 commented Dec 30, 2022

davepagurek commented Dec 30, 2022

inaridarkfox4231 commented Dec 30, 2022

davepagurek commented Dec 30, 2022

inaridarkfox4231 commented Dec 30, 2022

davepagurek left a comment

Choose a reason for hiding this comment

inaridarkfox4231 commented Jan 3, 2023