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

Add Framebuffer support #6072

Merged
merged 20 commits into from
Apr 4, 2023
Merged

Conversation

davepagurek
Copy link
Contributor

@davepagurek davepagurek commented Mar 15, 2023

This is still WIP! I appreciate any incremental feedback anyone has in the mean time though! Here's my to-do list:

  • Add unit tests for various format/channel/alpha combinations
  • Add unit tests for texture() using fbos, and specific fbo textures (color/depth)
  • Add doc comments everywhere (e.g. what options are supported, example usage)
  • Make follow-up issues for:
    • Verifying that all the format/channel/alpha combinations work on Windows too (I don't have a Windows PC so unfortunately I can't test this easily)
    • An API for reading back pixels
    • A Learn page on the reference including:
      • An example using a framebuffer on a p5.Graphics
      • An example demonstrating autosize vs fixed size
      • An example for working with fbo.defaultCamera and fbo.createCamera()
      • An explainer about the difference between Framebuffers and p5.Graphics
      • An explainer about the purpose of FLOAT colors
    • More interesting Framebuffer examples (maybe a fog shader?)
    • Making ortho cameras resize when the canvas/framebuffer resizes

Resolves #6018
Resolves #5516

Changes

  • Adds a p5.Framebuffer class that can be used in WebGL mode to draw to a texture
    • manual-test-examples/webgl/framebuffer/formats lets you test different combinations of inputs in one spot to make sure they all work
  • Refactors texture storage in p5.RendererGL to use a Map from source to texture rather than iterating through a list
  • Adds a Set of framebuffers in p5.RendererGL so that ones defaulting to the full size of the canvas also get updated when you resize the canvas

Screenshots of the change

manual-test-examples/webgl/framebuffer/feedback, showing how floating point Framebuffers can be used to fade out to white without leaving a trail:
Screen Shot 2023-03-15 at 1 40 23 PM

manual-test-examples/webgl/framebuffer/depth, showing reading the depth texture (note that only the red channel is reliably present; in some browsers, depth info might be copied to all three channels, making it look black and white instead of tinted red):
Screen Shot 2023-03-15 at 1 46 43 PM

PR Checklist

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

@davepagurek davepagurek requested a review from aferriss March 15, 2023 18:00
src/core/rendering.js Outdated Show resolved Hide resolved
this.target._renderer,
this.color,
{
glMinFilter: gl.LINEAR,
Copy link
Contributor

@aferriss aferriss Mar 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same Q as above. I think there is a strong argument to be made for having control over the interpolation mode or at least respecting the original one. Framebuffers are often used to store data, using linear scaling would corrupt that data. Maybe we can preserve whatever mode was used in the previously bound textures.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, do you think we should add these as options you can supply in settings?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we didn't supply them as settings, would the method for changing the interpolation mode to be to call those methods yourself on the textures after the framebuffer has been constructed?

Personally I think they'd be useful settings in the constructor but I know there are a lot of possible values there already. If we added settings for interpolation, then we might also want to add settings for texture wrap (REPEAT, CLAMP, MIRROR). If you give a mouse a cookie...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'm in favour of adding it to the constructor -- it's probably not the end of the world having those all in the constructor since they're all optional and we're using a settings object, so you can just specify the bits you want. I think probably the existing textureWrap() function will let people set the wrap mode so we can keep that out for now, although I think adding it to the constructor in the future could make sense too.

Copy link
Contributor Author

@davepagurek davepagurek Mar 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm also thinking, for simplicity should we assume that the min and mag filter should be the same for now? I'm wondering if it makes sense to expose this as something like textureSmoothing: Boolean which sets the min/mag filters to LINEAR if smoothed or NEAREST otherwise. It would be abstracting the underlying settings a bit, but maybe would be more aligned with how people would be thinking about the settings

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

update: I'm taking out the option for LINEAR for depth since it turns out it's just not supported: https://computergraphics.stackexchange.com/questions/9719/does-webgl-2-support-linear-depth-texture-sampling

Copy link
Contributor

@aferriss aferriss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is AWESOME! Thanks so much for getting this PR together @davepagurek. I left a number of comments on the PR but overall I think once you pull together the documentation and unit tests we'll be very close.

I think we should also consider adding in a couple tests for the various camera functions (ortho, perspective, camera, frustum) to make sure they're all working correctly with framebuffers.

Maybe this will be part of the documentation but we should also write some small explainer for what framebuffers are and why you would use a framebuffer over p5.graphics and what the various pros and cons might be.

src/core/rendering.js Outdated Show resolved Hide resolved
src/core/rendering.js Outdated Show resolved Hide resolved
@davepagurek
Copy link
Contributor Author

I added a few camera tests, although right now it's just matching the behaviour of default cameras in general (https://github.com/processing/p5.js/blob/main/src/webgl/p5.Camera.js#L1607-L1618) so that means only the default camera gets automatically resized, and it doesn't try to do anything to handle ortho cameras.

How do you feel about framebuffers matching this behaviour for now, and then I'll make an issue to try to make both framebuffer default cameras and all other default cameras update the aspect ratio rather than resetting the whole camera?

@davepagurek
Copy link
Contributor Author

davepagurek commented Mar 22, 2023

Just noticed something else we might want to deal with: the textures used in framebuffers will already have premultiplied alpha. Currently the shaders in p5 output premultiplied alpha, but assume their inputs (colors, textures) are not premultiplied. This means drawing transparent framebuffers to the main canvas currently ends up darkening the transparent bits: https://editor.p5js.org/davepagurek/sketches/xevCeEf9t

Seems like to make everything consistent, we'll need to turn on UNPACK_PREMULTIPLY_ALPHA_WEBGL to get premultiplied textures from images/graphics, and premultiply colors we pass in too, and then update the shaders accordingly

edit: Actually, I'm just going to assume images are premultiplied and colors aren't, because premultiplying all colors everywhere turns out to be a huge task, and we already treat images differently in the two shaders that use them :') Here's a new sketch making sure I can fade out both images and framebuffer textures without them darkening: https://editor.p5js.org/davepagurek/sketches/SGyLfCIN8

src/core/rendering.js Outdated Show resolved Hide resolved
src/core/rendering.js Outdated Show resolved Hide resolved
@aferriss
Copy link
Contributor

Sorry for the delay in reviewing, I've been traveling the past two weeks! Do you think that image() should be able to draw framebuffers? I'm inclined to include this behavior and it brings framebuffers closer in parity to p5.Graphics.

code pointer for the image function

@davepagurek
Copy link
Contributor Author

No worries, I hope your travels went well! I think that makes sense to add.

@davepagurek
Copy link
Contributor Author

The main issue with image before was that the resulting image would be flipped because the raw WebGL textures store their y axes flipped compared to how they get read in from images and graphics. Rather than flip the latter two with UNPACK_FLIP_Y_WEBGL and updating all texture coordinates, I'm flipping just framebuffer textures by making FramebufferCamera projection matrices flip the y value.

This means image will Just Work™, but we'll have to differentiate between reading from main canvases and framebuffers when we implement #6082 because we currently flip y when reading canvases but won't need to for framebuffers.

Copy link
Contributor

@aferriss aferriss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Super excited to see this addition, thanks for all the hard work @davepagurek!

@davepagurek
Copy link
Contributor Author

Thanks for your patience reviewing and going through all the details with me!

@davepagurek davepagurek merged commit d086bae into processing:main Apr 4, 2023
@davepagurek davepagurek deleted the feat/framebuffers branch April 4, 2023 17:15
@aferriss
Copy link
Contributor

aferriss commented Apr 4, 2023

And thank YOU for being patient with my slow reviews!

@Qianqianye
Copy link
Contributor

This is exciting! Thanks @davepagurek and @aferriss 🎉

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

Successfully merging this pull request may close these issues.

Support for drawing to/reading from framebuffers Questions about p5.RendererGL.getTexture()
3 participants