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

Implemented roll function #6819

Closed

Conversation

haroon10725
Copy link
Contributor

@haroon10725 haroon10725 commented Feb 18, 2024

Implemented roll function #6760

@haroon10725
Copy link
Contributor Author

Hey @davepagurek please review it. Thanks.

@davepagurek
Copy link
Contributor

I was taking a look at the example, and it seems like the implementation doesn't quite do what a roll actually is. Here, it seems to do something kind of like tilt(), but where the object is kept centered:
image

What I would expect it to do is produce something kind of like how rotate() works in 2D (the image below is just an approximation, made using rotate()):
image

It looks like that's because _rotateView only modifies the point the camera is looking at (centerX/Y/Z), leaving the camera position (eyeX/Y/Z) and the up vector in place:

p5.js/src/webgl/p5.Camera.js

Lines 1052 to 1057 in 4c13650

rotatedCenter[0],
rotatedCenter[1],
rotatedCenter[2],
this.upX,
this.upY,
this.upZ

I think the correct output of roll() would leave the center and eye the same, but just rotate the up vector. @SableRaf does that make sense, given how much you've been looking into cameras recently?

If that's true, then I think (but am not certain, definitely test this!) that we could edit _rotateView to also apply the rotation matrix to the up vector:

const rotatedUp = [
  this.upX * rotation.mat4[0] + this.upY * rotation.mat4[4] + this.upZ * rotation.mat4[8],
  this.upX * rotation.mat4[1] + this.upY * rotation.mat4[5] + this.upZ * rotation.mat4[9],
  this.upX * rotation.mat4[2] + this.upY * rotation.mat4[6] + this.upZ * rotation.mat4[10]
];

* createCanvas(100, 100, WEBGL);
* normalMaterial();
* cam = createCamera();
* // set initial pan angle
Copy link
Contributor

Choose a reason for hiding this comment

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

There are a few uses of pan in comments, should these be roll?

* delta *= -1;
* }
*
* rotateX(frameCount * 0.01);
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we're going to be doing something akin to a rotateZ by applying the roll(), I think it'll probably look clearer visually if we keep a vertical stack of cubes that are rotating about the vertical axis since the whole stack will be spinning clockwise/counterclockwise from the roll. That'd be the same as the cubes in the tilt example:

  rotateY(frameCount * 0.01);
  translate(0, -100, 0);
  box(20);
  translate(0, 35, 0);
  box(20);
  translate(0, 35, 0);
  box(20);
  translate(0, 35, 0);
  box(20);
  translate(0, 35, 0);
  box(20);
  translate(0, 35, 0);
  box(20);
  translate(0, 35, 0);
  box(20);

@SableRaf
Copy link
Contributor

SableRaf commented Feb 23, 2024

I think the correct output of roll() would leave the center and eye the same, but just rotate the up vector. @SableRaf does that make sense, given how much you've been looking into cameras recently?

Yes! The camera should rotate around its own forward vector.

A good approach would be to compute the camera rotation using quaternions, then compute the up and forward vectors from the resulting rotation and apply them to the camera. This demo by scudly is a good example of how to do it: https://infinitefunspace.com/p5/fly/

I made a minimal example based on it here: https://editor.p5js.org/SableRaf/sketches/WPYb6PRMu

@SableRaf
Copy link
Contributor

@haroon10725 are you still working on this? Otherwise we'll look if someone else would like to tackle it.

@haroon10725
Copy link
Contributor Author

@SableRaf I am busy, you can look if some one can solve it.

@davepagurek
Copy link
Contributor

If anyone else is interested in taking this up, feel free to branch off of this PR so that the work @haroon10725 has put in also gets included! A next step could be to try out the suggestion in #6819 (comment).

@rohanjulka19
Copy link
Contributor

Hi,
Have added the suggested changes with the existing changes of this PR in a new PR here #7093.

@davepagurek davepagurek closed this Sep 9, 2024
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