Open davepagurek opened 7 months ago
Hi @davepagurek , I appreciate your proposal, but I must admit I'm a bit confused, I'm sorry for any misunderstanding. I completely agree with your point about reducing the need for users to delve deep into p5.js's internal material systems. The idea of separating fill and stroke shaders makes sense to me. Rather than having a single shader for both stroke and fill, having two distinct shaders sounds like a good approach.
Reduces fewer discrepancies across browsers by no longer relying on the presence of uniforms that may be optimized away on some but not others
In this point, what is being conveyed? Is it something like for Strokes to be applied previously we need to include uniform uStrokeWeight? But after the implementation we would not have such requirment??
I apologize once again if my question seems silly. So, in a scenario where a user has lighting enabled and includes 13 attributes and uniforms in their shaders, will our shader be ignored at that point after the implementation? Can lights be handled in any cases? Mind explaning a bit on this context :)
Hello! @perminder-17
In this point, what is being conveyed? Is it something like for Strokes to be applied previously we need to include uniform uStrokeWeight? But after the implementation we would not have such requirment??
From what I could understand, the options object allows us to explicitly specify whether a shader should apply to fills or strokes. I think creation of this object avoids the need to rely on uniforms,
uStrokeWeight
in this case, that might be subject to optimization.I apologize once again if my question seems silly. So, in a scenario where a user has lighting enabled and includes 13 attributes and uniforms in their shaders, will our shader be ignored at that point after the implementation? Can lights be handled in any cases? Mind explaning a bit on this context :)
I don't think the shader will be ignored at that point.. I think the main aim is to provide a more controlled way to specify where the shader should be applied at the time of creation. Their functionality might not be affected.
Please correct me if I'm wrong.
The suggested changes seem very helpful. @davepagurek
It would be great to introduce an options object, separate fill and stroke shaders. The ability to specify different combinations of strokes and fills provide great flexibility to the user.
Moreover, in order to send an image to a shader, addressing use of image(...)
by calling texture(...)
or setting a uniform, unaffected by either of stroke or fill shaders resolves a number of issues related to image(...)
. Avoiding dependence on uniforms make great sense! These changes would definitely make usage of shaders easier for everyone!
@deveshidwivedi that's right, it would be used for strokes if the user says it should be used for strokes regardless of whether or not it uses uStrokeWeight
, and similarly, fill shaders would be used even if they don't support lights.
@perminder-17 The idea would be, if a user wants to override fills or strokes, they shouldn't need to implement every fill/stroke feature p5 supports, and we should just use whatever parts of their shader happen to work with p5. So regardless of what uniforms/attributes their shader uses, if they define the shader as applying to strokes, we will try to use it for strokes, and report any encountered errors to the user rather than silently ignoring it.
I think it'd also be useful to make it easier for shaders to include functionality like lighting or stroke weight without having to do it all themselves, but that would be addressed in a separate effort (something like https://github.com/processing/p5.js/issues/6144.)
I like the idea to specifically target fills / strokes separately but am a little concerned about the breaking changes this would introduce. Do you see any path to do this that could leave in place existing functionality? I do think that removing the ability for a shader to affect image()
is probably the right move though.
If a user omits the settings object, what would the defaults be (stroke: false, fill: true)?
Do we actually need a settings object or could it just be an optional enum as a third parameter FILL
, STROKE
, & FILLANDSTROKE
?
If a user omits the settings object, what would the defaults be (stroke: false, fill: true)?
Right, I was imagining this is the most common usage.
Do you see any path to do this that could leave in place existing functionality?
The 2.0 release already will have breaking changes, which is why I was thinking this could be an opportunity to simplify. But if we don't include this in 2.0, then to make this fully backwards compatible, I think we'd also need fillWithLight
and fillWithTexture
to handle cases where lights are included, and textures are included, and the other fill
flag would just be for remaining fills. We'd then default fillWithTexture
to whether or not a sampler2D
uniform exists, and default fillWithLight
to something similar, but for that long list of lighting attributes/uniforms.
The problem with the current state of those defaults is that even if we list them, that's still kind of not really enough for users who want to make use of them: we'd need to explain what each one does and why they'd need to use them. I feel like that dumps a lot of info at once onto users.
Somewhat related, I haven't written up a proposal for this in an issue yet, but in terms of documenting built-in shader features for users to tap into, in the future I want to selectively expose some shader internals people can build off of via an interface like this:
const myShader = p5.RendererGL.lightingShaderGraph.fillColor.replaceInput(`
uniform float millis;
vec4 makeColor() {
return vec4(sin(millis)*0.5 + 0.5, 0.0, 0.0, 1.0);
}
`).build();
...and then we could document the properties available on a shader graph, and let people replace their inputs with shader snippets. The benefit is that we can keep many of the properties abstracted if we choose, document properties in the way other properties in the reference are documented (shader uniforms/attributes don't really fit in with the current reference setup), and lets us change implementation under the hood without breaking shaders built this way.
Do we actually need a settings object or could it just be an optional enum as a third parameter
FILL
,STROKE
, &FILLANDSTROKE
?
Possibly! I think if these are the only two properties we'll ever have, then that works. I kind of like settings objects in general as it gives us room to grow in the future if we need (e.g. if we ever want break up fill
into more categories later) -- just puts less weight on the API decisions right now.
btw, just another thought: it might make more sense to not add an options object, and instead:
shader(yourShader)
will always use your shader for fills, and only for fillsstrokeShader(yourShader)
, which will always use your shader for strokes, and only for strokesThis might make it even more clear for users since everything happens at the site where you try to use the shader, and not having part of the logic up where the shader was created.
Increasing access
Shaders are arguably the part of p5 with the biggest barrier to entry. There's some existing confusion about why they get applied some times rather than others. Resolving this confusion would help to make shaders easier for users to learn and grasp.
Which types of changes would be made?
Most appropriate sub-area of p5.js?
What's the problem?
Currently, p5 tries to determine if a shader should be used for fills, strokes, images, and more based on what uniforms/attributes they include. This is currently documented like so:![image](https://github.com/processing/p5.js/assets/5315059/a7194813-f116-4afe-ae4c-bbddfce8df48)
If a shader doesn't need these properties, adding it into a shader unused is an unreliable solution, as some browsers optimize away unused shader properties.
Additionally, this doesn't fully address other open issues (https://github.com/processing/p5.js/issues/6327, https://github.com/processing/p5.js/issues/6564) as there are other scenarios where p5 will opt instead for a default shader, such as when using
image(...)
.What's the solution?
I propose simplifying the scenarios where user shaders can be applied to just fills and strokes.
image(...)
: if one wants to send an image to a shader, one must calltexture(...)
or set a uniform, and then draw a rectangle.To define what a shader applies to, I suggest adding an options object to
createShader
andloadShader
that looks like this (in Typescript syntax):This means that every shader you create must define what it should be applied to upon creation. By default, shaders just apply to fills and not strokes, as I think this is the most common use case. If one wants, one can specify a different combination of strokes and fills.
This also means that rather than having a single user shader set, one can possibly have a separate stroke and fill shader set.
Pros (updated based on community comments)
Cons (updated based on community comments)
Proposal status
Under review