-
-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
Image light performance improvements by implementing Framebuffers instead of Graphics. #6599
Conversation
Let me know @davepagurek if you have any suggestions/changes regarding this PR. |
src/webgl/p5.RendererGL.js
Outdated
// create framebuffer is like making a new sketch, all functions on main | ||
// sketch it would be available on framebuffer | ||
newFramebuffer.draw(() => { | ||
let irradiance = this._pInst.createShader( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think we can do the same thing we do for this.specularShader
, and make it reuse a single shader for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have a look now, I have tested it btw and it's totally working.
src/webgl/p5.RendererGL.js
Outdated
newGraphic.rect(0, 0, newGraphic.width, newGraphic.height); | ||
this.diffusedTextures.set(input, newGraphic); | ||
return newGraphic; | ||
newFramebuffer = this._pInst.createFramebuffer(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this makes the framebuffer match the size of the main canvas, but we calculate a different size for it above. We can use that size if we do:
newFramebuffer = this._pInst.createFramebuffer(); | |
newFramebuffer = this._pInst.createFramebuffer({ | |
width, | |
height, | |
density: 1, | |
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great suggestion, it was an oversight by me. I have changed it in specular framebuffer too as we were matching the size of the main canvas before the loop. Although, when we are inside the loop, we are resizing it again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work on this, the performance improvements look great! Just a few minor changes and then we're good to go!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, let's merge this in!
Great.... Love to contribute more❤❤ |
Resolves #6585
Changes:
In this pull request we have optimised time to a large extent. Previously it was taking around 4 second to render image for a specific sketch and now it's just taking around 2 seconds.
Screenshots of the change:
Before:-
After:-
PR Checklist
npm run lint
passes