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

Should setCamera() also call resetMatrix()? #7056

Closed
1 of 17 tasks
davepagurek opened this issue May 17, 2024 · 6 comments · Fixed by #7067
Closed
1 of 17 tasks

Should setCamera() also call resetMatrix()? #7056

davepagurek opened this issue May 17, 2024 · 6 comments · Fixed by #7067
Assignees

Comments

@davepagurek
Copy link
Contributor

Most appropriate sub-area of p5.js?

  • Accessibility
  • Color
  • Core/Environment/Rendering
  • Data
  • DOM
  • Events
  • Image
  • IO
  • Math
  • Typography
  • Utilities
  • WebGL
  • Build process
  • Unit testing
  • Internationalization
  • Friendly errors
  • Other (specify if possible)

p5.js version

1.9.3

Web browser and version

All

Operating system

All

Steps to reproduce this

Currently, setting a camera just updates the projection matrix and does not change the model view matrix. I assume that normally this is not an issue because people don't switch cameras in the middle of draw. However, this leads to some confusion when using cameras on framebuffers, which must be applied within draw. If you comment out the resetMatrix call, changing cameras has no effect:

// 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;
  }
}

Live: https://editor.p5js.org/davepagurek/sketches/8uENBvPU3

I think setCamera should also update uMVMatrix, basically also updating the camera position.

@nickmcintyre
Copy link
Member

@davepagurek would this approach also affect/reset transformations applied elsewhere in code?

@davepagurek
Copy link
Contributor Author

I think it would only affect transforms if you're calling setCamera in the middle of draw rather than in setup or in an event handler. But if you were previously calling setCamera mid-draw, arguably the resulting things you draw wouldn't have been in the correct spot anyway since they wouldn't be using the camera's transform yet?

@nickmcintyre
Copy link
Member

As long as the reference is clear about where/when to call setCamera() I think we're good. This test case is a little contrived but it surprised me.

@davepagurek
Copy link
Contributor Author

Right, I guess the rule would be, set the camera before you draw the content?

Also, we have this PR basically ready to start testing as soon as we're done making quick docs releases for the website release: #6761 I think after this change, since the camera and model transform matrices will not be merged into a single variable, we actually might not need to reset at all, since we'll be able to discern what part came from transforms vs what part came from the camera. So maybe the thing to do is to wait until we can merge and test to see if that refactor fixes this for us? (The reason for the wait is that it's a pretty big change to a core part of the rendering algorithm, which I think warrants a beta testing period before release, which we've been skipping for our expedited docs releases.)

@nickmcintyre
Copy link
Member

Agreed that beta testing for this kind of change is a good call.

@davepagurek davepagurek self-assigned this May 22, 2024
@davepagurek
Copy link
Contributor Author

Just put up #7067 with something that augments #6761 so that only the view matrix updates when you change cameras. We can play around with that and see if there are any situations that would cause issues

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

Successfully merging a pull request may close this issue.

2 participants