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

setAttribute() function for defining custom shader attributes #7276

Draft
wants to merge 23 commits into
base: dev-2.0
Choose a base branch
from

Conversation

lukeplowden
Copy link

@lukeplowden lukeplowden commented Sep 20, 2024

Resolves #5120

In response to the issue above I've added a global setAttribute() function and a method of the same name on p5.Geometry.

2024-09-20.12-35-05.mp4

Attributes are per vertex data declared at the top of the vertex shader like so:

  attribute vec3 aPosition;

Currently, p5.js has a number of predefined attributes including aPosition, aVertexColor, aNormal, etc. These changes allow the user to define custom attributes in two ways, using an API similar to Shader.setUniform().

  1. By calling the global function setAttribute(attributeName, data) which applies the custom attribute to subsequent calls to vertex(), bezierVertex(), or curveVertex(), within a beginShape()->endShape() block, i.e. on immediate mode geometry. This is the same behaviour as fill() or normal() for per-vertex data.
  setAttribute('aCustom', [1, 0, 0]);
  1. By calling the member functionGeometry.setAttribute() in case the user wants to directly interface with p5.Geometry objects, i.e. on retained mode geometry. This essentially creates an array and pushes a new value on the p5.Geometry object, which is unlike other methods on p5.Geometry.
  myGeometry.setAttribute('aCustom', [1, 0, 0]);

The parameters for both functions are the same, a string attributeName and a number or array of numbers data.

PR Checklist

  • npm run lint passes
  • [Inline documentation] is included / updated
  • [Unit tests] are included / updated

This is a draft pull request as I'm finishing off the tests and documentation right now. Would like to hear other WebGL contributor's thoughts on progress @perminder-17 @Garima3110


Edge cases

There are a few edge cases related to the need for attributes to be properly set for each vertex. If attribute vec3 aCustom; is declared and used in a shader, there should be as many values for aCustom as there are for aPosition.

Global function (immediate mode)

  1. The user may call setAttribute() after the first vertex() call. In this case vertex() zero-fills the previous values for each call which came before. It behaves like this:
function draw(){
	shader(customAttributeShader);
	beginShape();
	vertex(-1,0,0); // This has no custom attribute set, so it is [0, 0, 0]
	setAttribute('aCustom', [1,1,1]); // Attribute set for following vertices
	vertex(1,0,0); // [1, 1, 1]
	vertex(0,1,0);// [1, 1, 1]
	endShape();
}
  1. The user may not set any attributes but their shader uses them. We could either give a friendly error that the shader is using undefined attributes, or fill all values with zero explicitly, or a combination.
function draw(){
	shader(customAttributeShader);
	beginShape();
	vertex(-1,0,0); // no value set for aCustom
	vertex(1,0,0);
	vertex(0,1,0);
	endShape();
}

Retained mode

Because the Geometry.setAttribute(attributeName, data) method pushes to an array Geometry[attributeName]:

let geo;

async function setup(){
	geo = new p5.Geometry();
	let v0 = new p5.Vertex(-1,0,0);
	let v1 = new p5.Vertex(1,0,0);
	let v2 = new p5.Vertex(0,1,0);
	geo.setAttribute('aCustom', [1,0,0]); // Only one custom attribute has been set
	geo.vertices.push(v0, v1, v2); // Three vertex attributes have been set
} 

In this case we plan to just give a Friendly Error saying that one of the vertex attributes has not been properly filled.

@lukeplowden lukeplowden marked this pull request as draft September 20, 2024 12:03
@davepagurek
Copy link
Contributor

It looks like we've got some changes from the main branch in here, do we have any conflicts if we git rebase dev-2.0? If it tries to replay some of the main branch commits, maybe we can skip the main branch commits by doing an interactive rebase with git rebase -i dev-2.0 and then deleting the lines with the unrelated commits?

@lukeplowden
Copy link
Author

It looks like we've got some changes from the main branch in here, do we have any conflicts if we git rebase dev-2.0? If it tries to replay some of the main branch commits, maybe we can skip the main branch commits by doing an interactive rebase with git rebase -i dev-2.0 and then deleting the lines with the unrelated commits?

Thanks for spotting that, should be good now!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants