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

[p5.js 2.0 RFC Proposal]: Make shaders define what they get used for #6759

Closed
2 of 21 tasks
davepagurek opened this issue Jan 21, 2024 · 7 comments
Closed
2 of 21 tasks

Comments

@davepagurek
Copy link
Contributor

davepagurek commented Jan 21, 2024

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?

  • Breaking change (Add-on libraries or sketches will work differently even if their code stays the same.)
  • Systemic change (Many features or contributor workflows will be affected.)
  • Overdue change (Modifications will be made that have been desirable for a long time.)
  • Unsure (The community can help to determine the type of change.)

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
  • Internationalization
  • Friendly errors
  • Other (specify if possible)

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

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 (#6327, #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.

  • Fill shaders will apply regardless of whether there is a current texture or lights.
  • Stroke shaders apply to all strokes.
  • Neither affects calls to image(...): if one wants to send an image to a shader, one must call texture(...) or set a uniform, and then draw a rectangle.

To define what a shader applies to, I suggest adding an options object to createShader and loadShader that looks like this (in Typescript syntax):

createShader(
  vertSrc: string,
  fragSrc: string,
  { strokes = false, fills = true }: { strokes: boolean; fills: boolean } = {}
)

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)

  • Simplifies documentation and requires the user to understand less about p5's internal material system
  • Reduces fewer discrepancies across browsers by no longer relying on the presence of uniforms that may be optimized away on some but not others
  • Allows a separate fill + stroke shader to be applied at once rather than trying to handle both in the same shader

Cons (updated based on community comments)

  • If a user shader has been set that does not handle lights or textures, and then one tries to add that in, it will make no visual difference

Proposal status

Under review

@perminder-17
Copy link
Contributor

perminder-17 commented Jan 22, 2024

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 :)

@deveshidwivedi
Copy link
Contributor

deveshidwivedi commented Jan 22, 2024

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.

@deveshidwivedi
Copy link
Contributor

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!

@davepagurek
Copy link
Contributor Author

@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 #6144.)

@aferriss
Copy link
Contributor

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?

@davepagurek
Copy link
Contributor Author

davepagurek commented Jan 25, 2024

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.

@davepagurek
Copy link
Contributor Author

btw, just another thought: it might make more sense to not add an options object, and instead:

  • Calling shader(yourShader) will always use your shader for fills, and only for fills
  • Add a new api, strokeShader(yourShader), which will always use your shader for strokes, and only for strokes

This 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.

@davepagurek davepagurek self-assigned this Jun 18, 2024
@Qianqianye Qianqianye moved this from Proposal to Implementation in p5.js 2.0 Sep 10, 2024
@perminder-17 perminder-17 moved this from Implementation to Completed in p5.js 2.0 Oct 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Completed
Development

No branches or pull requests

5 participants