fixed geometry builder to accomodate vertexStrokeColors

[Vaivaswat2244]

Resolves #7840

Changes:
Added per-vertex stroke color handling to GeometryBuilder.addGeometry() method, following the same pattern as the existing fill color logic:

• Copies stroke colors from input geometry to built geometry
• Pads missing stroke colors with current renderer stroke state
• Uses [-1, -1, -1, -1] fallback indicator when no stroke color is set
• Maintains 4 values per vertex format (r, g, b, a)

The following screenshot shows the given fixes...

preview/index.html:

    import p5 from '../src/app.js';

    const sketch = function (p) {
      let geom;
      
      p.setup = async function () {
        p.createCanvas(400, 400, p.WEBGL);
        p.angleMode(p.DEGREES);
        
        p.strokeWeight(5);
        p.noFill();
        p.randomSeed(5);
        
        geom = p.buildGeometry(() => {
          // *** remove/replace POINTS and works as expected
          p.beginShape(p.LINES); // LINES or TRIANGLES or empty works fine
          for(let i = 0; i < 50; i++){
            p.push();
            p.stroke(p.random(255), p.random(255), p.random(255));
            let x = p.random(-p.width/2, p.width/2);
            let y = p.random(-p.width/2, p.width/2);
            let z = p.random(-p.width/2, p.width/2);
            p.vertex(x, y, z);
            p.vertex(x, y, z+10);
            p.pop();
          }
          p.endShape();
        });
      };

      p.draw = function () {
        p.background(0);
        p.orbitControl(3);
        p.rotateY(p.frameCount/3);
        
        p.strokeWeight(5);
        // p.stroke(255); // uncomment to show points
        p.model(geom);
      };
    };

    new p5(sketch);

Screenshots of the change:

PR Checklist

• npm run lint passes
• Inline reference is included / updated
• Unit tests are included / updated

[davepagurek]

This is looking good! I left a comment about how we handle default strokes, and then think before we merge, it'd be good to add some tests (maybe a visual test where we generate a geometry without any stroke colors and then draw it later with a stroke('red') or something, and another where we do generate it with internal stroke colors, and ensure that those colors aren't overridden when we draw the geometry?)

[davepagurek]

this.renderer.states.strokeColor will always exist, it might just be the default color. What we do for the fill colors is, when we start building a p5.Geometry, set the current color to -1, -1, -1, -1 so that we only get non-negative colors when someone calls fill() or stroke() inside of the buildGeometry callback:

p5.js/src/webgl/p5.RendererGL.js

Lines 483 to 491 in d51184b

	beginGeometry() {
	if (this.geometryBuilder) {
	throw new Error(
	"It looks like `beginGeometry()` is being called while another p5.Geometry is already being build."
	);
	}
	this.geometryBuilder = new GeometryBuilder(this);
	this.geometryBuilder.prevFillColor = this.states.fillColor;
	this.fill(new Color([-1, -1, -1, -1]));

Can we do the same for strokes?

[Vaivaswat2244]

okay so this should look something like this if I got this right...

beginGeometry() {
  if (this.geometryBuilder) {
    throw new Error(
      "It looks like `beginGeometry()` is being called while another p5.Geometry is already being build."
    );
  }
  this.geometryBuilder = new GeometryBuilder(this);
  
  this.geometryBuilder.prevFillColor = this.states.fillColor;
  this.geometryBuilder.prevStrokeColor = this.states.strokeColor; 
  this.fill(new Color([-1, -1, -1, -1]));
  this.stroke(new Color([-1, -1, -1, -1]));

[davepagurek]

I think so, yep!

[davepagurek]

There's also an extra step at the end where we reset the previous fill color:

p5.js/src/webgl/p5.RendererGL.js

Line 510 in d51184b

this.fill(this.geometryBuilder.prevFillColor);

So we'd do something similar for strokes there too

[Vaivaswat2244]

Right. Got it.

[Vaivaswat2244]

Hi @davepagurek ! I've been working on implementing changes you suggested, but I'm running into some test failures that I'd like your guidance on.

Changes Made

Following your recommendation, I added the stroke color state management similar to how fill colors are handled:

In p5.RendererGL.js:

// Modified beginGeometry()
beginGeometry() {
 if (this.geometryBuilder) {
   throw new Error(
     "It looks like `beginGeometry()` is being called while another p5.Geometry is already being build."
   );
 }
 this.geometryBuilder = new GeometryBuilder(this);
 this.geometryBuilder.prevFillColor = this.states.fillColor;
 this.geometryBuilder.prevStrokeColor = this.states.strokeColor; // Added this
 this.fill(new Color([-1, -1, -1, -1]));
 this.stroke(new Color([-1, -1, -1, -1])); // Added this
}

// Modified endGeometry() 
endGeometry() {
 // ... existing code ...
 this.fill(this.geometryBuilder.prevFillColor);
 this.stroke(this.geometryBuilder.prevStrokeColor); // Added this
 // ... rest of method
}

For the test sections,
I wrote two tests,

visualSuite('buildGeometry stroke colors', () => {
    visualTest('Geometry without stroke colors, global stroke override', (p5, screenshot) => {
      p5.createCanvas(50, 50, p5.WEBGL);
      
      // Build geometry without any stroke() calls inside
      const geom = p5.buildGeometry(() => {
        p5.beginShape();
        p5.vertex(-15, -15, 0);
        p5.vertex(15, -15, 0);
        p5.vertex(15, 15, 0);
        p5.vertex(-15, 15, 0);
        p5.endShape(p5.CLOSE);
      });
      
      p5.background(220);
      p5.stroke('red'); // Should override and make all strokes red
      p5.strokeWeight(2);
      p5.noFill();
      p5.model(geom);
      screenshot();
    });

    visualTest('Geometry with internal stroke colors not overridden', (p5, screenshot) => {
      p5.createCanvas(50, 50, p5.WEBGL);
      
      // Build geometry WITH stroke() calls inside
      const geom = p5.buildGeometry(() => {
        p5.beginShape();
        
        p5.stroke('blue');
        p5.vertex(-15, -15, 0);
        
        p5.stroke('green');
        p5.vertex(15, -15, 0);
        
        p5.stroke('purple');
        p5.vertex(15, 15, 0);
        
        p5.stroke('orange');
        p5.vertex(-15, 15, 0);
        
        p5.endShape(p5.CLOSE);
      });
      
      p5.background(220);
      p5.stroke('red'); // This should NOT override the internal colors
      p5.strokeWeight(2);
      p5.noFill();
      p5.model(geom);
      screenshot();
    });
  });

So now some of the tests are failing after the changes in p5.Renderer file,

Test 1: /buildgeometry()/can draw models

visualTest('can draw models', (p5, screenshot) => {
    p5.createCanvas(50, 50, p5.WEBGL);

    const sphere = p5.buildGeometry(() => {
      p5.scale(0.25);
      p5.sphere();
    });

    const geom = p5.buildGeometry(() => {
      p5.model(sphere);
    });

    p5.background(255);
    p5.lights();
    p5.model(geom);
    screenshot();
  });

This test was passing before the renderer changes

Test 2: Test 2: Geometry without stroke colors, global stroke override

visualTest('Geometry without stroke colors, global stroke override', (p5, screenshot) => {
      p5.createCanvas(50, 50, p5.WEBGL);
      
      // Build geometry without any stroke() calls inside
      const geom = p5.buildGeometry(() => {
        p5.beginShape();
        p5.vertex(-15, -15, 0);
        p5.vertex(15, -15, 0);
        p5.vertex(15, 15, 0);
        p5.vertex(-15, 15, 0);
        p5.endShape(p5.CLOSE);
      });
      
      p5.background(220);
      p5.stroke('red'); // Should override and make all strokes red
      p5.strokeWeight(2);
      p5.noFill();
      p5.model(geom);
      screenshot();
    });

This test passes when I undo the renderer changes

image of diffs of first and second test

Should I commit the changes and tests so you could have a look at them. we can revert them if needed

[davepagurek]

It looks like we also need to update the line shader.

In the fill shader, it checks that the x value of the color is at least 0 as a quick way to see if it's a real color:

p5.js/src/webgl/shaders/normal.vert

Line 40 in 2606c21

inputs.color = (uUseVertexColor && aVertexColor.x >= 0.0) ? aVertexColor : uMaterialColor;

but currently in the stroke shader, it doesn't, it just looks like this:

p5.js/src/webgl/shaders/line.vert

Line 102 in 2606c21

inputs.color = uUseLineColor ? aVertexColor : uMaterialColor;

[davepagurek]

Looks like this test is failing now:

p5.js/test/unit/webgl/p5.RendererGL.js

Lines 1406 to 1429 in 2606c21

	test('transparency works the same with per-vertex colors', function() {
	myp5.createCanvas(20, 20, myp5.WEBGL);
	myp5.noStroke();
	function drawShapes() {
	myp5.fill(255, 0, 0, 100);
	myp5.rect(-10, -10, 15, 15);
	myp5.fill(0, 0, 255, 100);
	myp5.rect(-5, -5, 15, 15);
	}
	drawShapes();
	myp5.loadPixels();
	const eachShapeResult = [...myp5.pixels];
	myp5.clear();
	const shapes = myp5.buildGeometry(drawShapes);
	myp5.model(shapes);
	myp5.loadPixels();
	const singleShapeResult = [...myp5.pixels];
	assert.deepEqual(eachShapeResult, singleShapeResult);
	});
	});

This test should probably be a visual test since it's hard to understand what's actually failing by looking at a big array of color channel values, but I wonder if the noStroke() is getting overridden. After building geometry, we set stroke() again with the previous color. We probably need to call noStroke() if the previous stroke is null, or stroke(...) otherwise. This is likely also something we have to do with fills but we just didn't notice because the noFill() case isn't tested? Maybe it's worth adding a test for that too.