User selected shader is ignored in some cases

[JasonGoemaat]

Increasing Access

Unsure

Most appropriate sub-area of p5.js?

• Accessibility
• Color
• Core/Environment/Rendering
• Data
• DOM
• Events
• Image
• IO
• Math
• Typography
• Utilities
• WebGL
• Build Process
• Unit Testing
• Internalization
• Friendly Errors
• Other (specify if possible)

Feature enhancement details

I tried to write a simple shader and was confused. It seemed like my shader wasn't being used on the sphere I created at all. I finally found in the source code that when P5 selects what shader to use, it will ignore the shader you've set if some criteria are met. For example if you have lighting enabled and your shader doesn't contain the 13 attributes and uniforms in p5.Shader.js, P5 will fall back to it's default lighting shader. A couple of things would be nice to avoid confusion:

1. A minimal shader example that works with lighting and/or textures
2. Notes in the documentation about what is required in a shader if lighting and/or textures are enabled
3. A warning in the console if the user has set their own shader and it is being ignored

[welcome]

Welcome! 👋 Thanks for opening your first issue here! And to ensure the community is able to respond to your issue, please make sure to fill out the inputs in the issue forms. Thank you!

[deveshidwivedi]

hi, can i work on this issue? @davepagurek

[deveshidwivedi]

I can add a shader example with lightning or texture. For documentation part, requirement of attributes and uniforms could be added, along with an example code, mentioning particularly about shaders using lightning and texture. Could you please suggest what else could be done and how I should proceed? @davepagurek

[davepagurek]

Hi @deveshidwivedi! I think starting by documenting the logic makes sense. In general, we replace default shaders if the user shader uses some of the same uniforms. You can take a look at the implementation of this and the following few functions in the source code to see which count:

p5.js/src/webgl/p5.Shader.js

Lines 554 to 570 in cab0ff1

	isLightShader() {
	return (
	this.attributes.aNormal !== undefined ||
	this.uniforms.uUseLighting !== undefined ||
	this.uniforms.uAmbientLightCount !== undefined ||
	this.uniforms.uDirectionalLightCount !== undefined ||
	this.uniforms.uPointLightCount !== undefined ||
	this.uniforms.uAmbientColor !== undefined ||
	this.uniforms.uDirectionalDiffuseColors !== undefined ||
	this.uniforms.uDirectionalSpecularColors !== undefined ||
	this.uniforms.uPointLightLocation !== undefined ||
	this.uniforms.uPointLightDiffuseColors !== undefined ||
	this.uniforms.uPointLightSpecularColors !== undefined ||
	this.uniforms.uLightingDirection !== undefined ||
	this.uniforms.uSpecular !== undefined
	);
	}

A minor correction to the initial issue: since these conditions currently use ||, you just need at least one of the uniforms mentioned, but don't need all of them. In the future this will be replaced with a more explicit method of replacing default shaders, but in the mean time, documenting this publicly would be helpful!

[deveshidwivedi]

Alright! @davepagurek I'll look into it and get back.

[deveshidwivedi]

Hi! @davepagurek. I went through this, how do i start?

[davepagurek]

I think we'll need to decide where to document this (maybe in the docs for shader(), since that's what applies the shader), and then edit the description in the doc comments for that function to explain under what circumstances we'll use a shader.

e.g.
The shader will be used for:

• fills when a texture is enabled if it includes a uniform sampler2D
• fills when lights are enabled if it includes the attribute aNormal, or if it has any of the following uniforms: uUseLighting, uAmbientLightCount, uDirectionalLightCount, uPointLightCount, uAmbientColor, uDirectionalDiffuseColors, uDirectionalSpecularColors, uPointLightLocation, uPointLightDiffuseColors, uPointLightSpecularColors, uLightingDirection, or uSpecular
• fills whenever there are no lights or textures
• strokes if it includes the uniform uStrokeWeight

We should maybe also add a note that we can consider this "experimental", as we plan to change this behaviour in the future.

[deveshidwivedi]

Sounds great, will there be shader examples also? @davepagurek

[davepagurek]

I think probably not for now, if only because the "minimal example" for a fully compatible lighting shader is pretty huge and involves copying everything from lighting.glsl in this repo. For that we'll just wait for #6144.

[deveshidwivedi]

Got it!

Where exactly is the documentation needed in shaders? Would it be somewhere close to isLightShader() ?
Is it just comments that we'll use for documenting, what would be the format if you could give an idea?

[davepagurek]

I think we'll want the docs to appear in the description here: https://p5js.org/reference/#/p5/shader

For any reference page, to find where the source code for the docs live, you can scroll to the bottom of the page to where it says this:

If you click the Github link, it takes you to where the docs are. In this case, it's here:

p5.js/src/webgl/material.js

Lines 331 to 345 in 37d3324

	/**
	* Sets the <a href="#/p5.Shader">p5.Shader</a> object to
	* be used to render subsequent shapes.
	*
	* Shaders can alter the positioning of shapes drawn with them.
	* To ensure consistency in rendering, it's recommended to use the vertex shader in the <a href="#/p5/createShader">createShader example</a>.
	*
	* Custom shaders can be created using the
	* <a href="#/p5/createShader">createShader()</a> and
	* <a href="#/p5/loadShader">loadShader()</a> functions.
	*
	* Use <a href="#/p5/resetShader">resetShader()</a> to
	* restore the default shaders.
	*
	* Note, shaders can only be used in WEBGL mode.

So I think we'll want to add to the doc comments in that block.

[deveshidwivedi]

Okay! Thank you so much!
Will it be in the same format like you mentioned earlier (fills and strokes)?

[davepagurek]

I think that would make sense!

[deveshidwivedi]

Please take a look, is this how it should be?
Added additional information about shader behavior

[davepagurek]

The content looks good! I think there might be some formatting issues with the * at the start of each line of the doc comment? Maybe also try running grunt yui:dev to launch a local version of the reference so you can make sure it looks correct in your browser too. Feel free to make a PR though, we can iron out details there 🙂

[deveshidwivedi]

@davepagurek I opened a PR, please have a look!