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

Update p5.Framebuffer references #7044

Merged
merged 4 commits into from
May 21, 2024

Conversation

nickmcintyre
Copy link
Member

Preparing references in src/webgl/p5.Framebuffer.js for the new website.

PR Checklist

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

@Qianqianye @davepagurek @limzykenneth

@@ -731,48 +980,53 @@ class Framebuffer {
}

/**
* Removes the framebuffer and frees its resources.
* Removes the framebuffer and from the web page.
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need the 'and' in the sentence 'Removes the framebuffer and from the web page'?
@nickmcintyre @davepagurek

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch @Qianqianye – fixed!

Copy link
Member Author

Choose a reason for hiding this comment

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

@Qianqianye @davepagurek also, this method seems broken. I'll open issues soon for a few bugs found while writing.

@Qianqianye Qianqianye requested a review from davepagurek May 16, 2024 01:01
* cam2.lookAt(0, 0, 0);
*
* // Set the current camera to cam1.
* setCamera(cam1);
Copy link
Contributor

@davepagurek davepagurek May 16, 2024

Choose a reason for hiding this comment

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

Something a little tricky here is that the framebuffer camera is active here even on the main canvas. Due to some technical internal things (the browser stores main canvas textures upside down compared to other textures), framebuffer cameras render upside down when used on the main canvas.

Probably the way to avoid that here is to do a createCamera() (for the main canvas, not on the framebuffer) at the end, and only call setCamera(cam1) within draw between begin and end.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe a key point we should mention here is that you only ever want to apply a framebuffer camera when you're actively drawing to the framebuffer.

In the current example, because it it's being applied outside, the camera is actually only affecting the image call. I added a red background to draw, and it looks like this:

function draw() {
  background('red')
  // Draw to the p5.Framebuffer object.
  myBuffer.begin();
  background(200);
  box();
  myBuffer.end();

  // Display the p5.Framebuffer object.
  image(myBuffer, -50, -50);
}
image

I was looking into what this needs to be to work, and came to something like this:

// Double-click to toggle between cameras.

let myBuffer;
let cam1;
let cam2;
let usingCam1 = true;

function setup() {
  createCanvas(100, 100, WEBGL);

  // Create a p5.Framebuffer object.
  myBuffer = createFramebuffer();

  // Create cameras between begin/end.
  myBuffer.begin();
  // Create the first camera.
  // Keep its default settings.
  cam1 = myBuffer.createCamera();

  // Create the second camera.
  // Place it at the top-left.
  // Point it at the origin.
  cam2 = myBuffer.createCamera();
  cam2.setPosition(400, -400, 800);
  cam2.lookAt(0, 0, 0);
  myBuffer.end();

  describe('A white cube on a gray background. The camera toggles between frontal and aerial views when the user double-clicks.');
}

function draw() {
  // Draw to the p5.Framebuffer object.
  myBuffer.begin();
  if (usingCam1) {
    setCamera(cam1);
  } else {
    setCamera(cam2);
  }
  background(200);
  resetMatrix()
  box();
  myBuffer.end();

  // Display the p5.Framebuffer object.
  image(myBuffer, -50, -50);
}

// Toggle the current camera when the user double-clicks.
function doubleClicked() {
  if (usingCam1 === true) {
    usingCam1 = false;
  } else {
    usingCam1 = true;
  }
}

Unfortunately it looks like setCamera right now doesn't update the position from the previous camera position, it just sets the projection matrix, so you have to call resetMatrix() for it to apply, which isn't great. I'll open an issue for that.

I think it might be helpful in the future if cameras didn't auto-set themselves as the active camera when you create them because I think right now it basically means you always have to do something to prevent making a new framebuffer camera from applying to the main canvas.

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried reworking this example to call setCamera() within doubleClicked() instead of draw() but can't seem to get it working. Not sure whether this a bug or I'm missing something obvious.

The current draft is accurate, but I figured I'd try to follow the best practice of setting the camera outside of draw(). If there's not a quick fix, then I suggest we go ahead with the current draft.

Copy link
Contributor

Choose a reason for hiding this comment

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

Framebuffers don't save any state (begin/end also act like a push/pop) so you'll still have to set framebuffer cameras within draw. I think the rule is maybe not so much that it must be set outside of draw, but just that it should be set before you actually draw anything. So setting it outside of draw is one way to accomplish that for the main canvas, but the closest we can get for framebuffers is doing it right after its begin.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ohh gotcha!

Copy link
Contributor

@davepagurek davepagurek left a comment

Choose a reason for hiding this comment

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

Thanks for the updates, looks good!

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

Successfully merging this pull request may close these issues.

3 participants