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

Support rendering to custom framebuffer #3742

Merged
merged 3 commits into from Oct 5, 2019
Merged

Conversation

Pessimistress
Copy link
Collaborator

Change List

  • Add experimental _framebuffer prop to Deck
  • Add test app
  • Documentation

@coveralls
Copy link

coveralls commented Oct 3, 2019

Coverage Status

Coverage increased (+0.005%) to 80.038% when pulling 1e66c6b on x/framebuffer into fddf1ab on master.

@tsherif
Copy link
Contributor

tsherif commented Oct 4, 2019

Could this not be handled by the application just binding a framebuffer in the onBeforeRender callback?

@Pessimistress
Copy link
Collaborator Author

@tsherif I think any user setting will be overwritten by https://github.com/uber/deck.gl/blob/master/modules/core/src/passes/layers-pass.js#L9

@Pessimistress Pessimistress merged commit 5af2287 into master Oct 5, 2019
@Pessimistress Pessimistress deleted the x/framebuffer branch October 5, 2019 00:58
Copy link
Collaborator

@ibgreen ibgreen left a comment

Choose a reason for hiding this comment

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

Looks good!

I remember suggesting supporting a View.framebuffer prop in the View classes RFC allowing both screen and framebuffer rendering at the same time. I suppose that would still be possible, and would offer slightly different functionality, as this top-level framebuffer prop will cover all views.

@@ -46,16 +47,14 @@ export default class DeckRenderer {

opts.layerFilter = this.layerFilter;
opts.effects = opts.effects || [];
opts.target = opts.target || Framebuffer.getDefaultFramebuffer(this.gl);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: In the spirit of consistency / typing things out and making them self-documenting, should we call this options.framebuffer or options.targetFramebuffer?

@@ -83,11 +82,11 @@ export default class DeckRenderer {
}

// Private
_preRender(effects, params) {
_preRender(effects, opts) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: Type out options.

@@ -57,7 +56,8 @@ export default class PickLayersPass extends LayersPass {
blend: !pickZ
},
() => {
this.drawLayers({
super.render({
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: Any chance we can avoid introducing a call to super? What happens if we call this.render()? If we rename super.render()?

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.

None yet

5 participants