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

reuse stencilbuffers in prerendering #601

Merged
merged 8 commits into from
Jul 23, 2014
Merged

reuse stencilbuffers in prerendering #601

merged 8 commits into from
Jul 23, 2014

Conversation

lbud
Copy link
Contributor

@lbud lbud commented Jul 23, 2014

@ansis does this cover #592 — or should it also be doing something about fbo?

var stencil = this.stencilBuffer = gl.createRenderbuffer();
var stencils = this.painter.prerenderStencils[this.size];
var stencil = this.stencilBuffer = stencils && stencils.length > 0 ? stencils.pop() : gl.createRenderbuffer();
stencil.size = this.size;
gl.bindRenderbuffer(gl.RENDERBUFFER, stencil);
gl.renderbufferStorage(gl.RENDERBUFFER, gl.STENCIL_INDEX8, this.size, this.size);
Copy link
Contributor

Choose a reason for hiding this comment

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

This should only be called if the stencil buffer is newly created

@ansis
Copy link
Contributor

ansis commented Jul 23, 2014

or should it also be doing something about fbo?

Yes, good catch. It should be creating the fbo once per size only binding the stencil buffer the fbo once

Lauren Budorick added 2 commits July 23, 2014 13:53
ansis added a commit that referenced this pull request Jul 23, 2014
reuse stencilbuffers in prerendering
@ansis ansis merged commit 3a9f482 into dev-pages Jul 23, 2014
@ansis ansis deleted the stencilreuse branch July 23, 2014 21:16
@mourner
Copy link
Member

mourner commented Jul 23, 2014

I wonder how other's commits (from John & Ansis here) end up in branches like this... What do you use for git? Command line, GitHub app, or something other?

@lbud
Copy link
Contributor Author

lbud commented Jul 23, 2014

Ha, I know, sorry -- I rebased (command line) dev-pages onto this so the Travis builds would pass. Not sure if I did it right...? 😕

@mourner
Copy link
Member

mourner commented Jul 23, 2014

Usually you'd do git rebase dev-pages from your branch, and after that all your commits would end up after all recent dev-pages commit, so they wouldn't appear in the diff of the pull request.

@lbud
Copy link
Contributor Author

lbud commented Jul 23, 2014

Weird -- that's exactly what I did. Not sure why those commits showed up here and caused merge conflicts within this branch...

@mourner
Copy link
Member

mourner commented Jul 23, 2014

Was the rebase conflicting? How did you resolve conflicts?

@lbud
Copy link
Contributor Author

lbud commented Jul 23, 2014

No, the rebase was fine -- I rebased successfully, then tried to push to this branch and got an error that I needed to pull remote changes first, so I did so and had merge conflicts between my previous commit and most recent commit (in js/render/prerendered and js/render/painter), opened those and deleted the conflicts (deletions from previous to current commit), then pushed 94f452a

@mourner
Copy link
Member

mourner commented Jul 23, 2014

@lbud oh I see, so here's the culprit. After you rebase, the history is rewritten so that your rebased commits are modified. So when you try to push, it doesn't allow you to because history has diverged, and pulling results in new commits along with new dev-pages commits being merged on top of old commits, which results in a mess. After rebase, just do git push --force (without pulling), and it will look clean.

@lbud
Copy link
Contributor Author

lbud commented Jul 23, 2014

Aha, ok. Very helpful -- thanks :)

lucaswoj pushed a commit that referenced this pull request Jan 11, 2017
stepankuzmin pushed a commit that referenced this pull request Jun 21, 2023
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.

4 participants