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

Resolves Issue#5958 3D orbitControl allowing +-90 degrees not enough. #6012

Merged
merged 5 commits into from
Feb 13, 2023

Conversation

AryanKoundal
Copy link
Contributor

@AryanKoundal AryanKoundal commented Feb 10, 2023

Resolves #5958

Changes:

  • camPhi was bound in the range [0,pi] and upY was only 1,
  • a directionflag is added to change the direction of upY of the camera object,
  • camPhi gets increment with respect to the direction of upY
  • Now camPhi can rotate 360 deg and continue rotating in any direction,
  • Along with that camTheta is modified by multiplying the sign of directionflag to the increment/decrement
  • directionflag used to determine the upY property of the camera object
  • Changing of the limits of camPhi resulted in a need of new tests
  • 2 new tests added related to the to and fro motion at 0 rad
  • 2 new tests added related to the to and fro motion at PI rad
  • 1 new test for continuous multiple rotations
  • Modified 1 test for checking the camRadius correctly.
  • Detailed Inline documentation also added.

PR Checklist

  • npm test
  • npm run build passes
  • npm run lint passes
  • Inline documentation is included / updated
  • Unit tests are included(5) / updated(1)

@davepagurek davepagurek self-requested a review February 11, 2023 00:13
@AryanKoundal
Copy link
Contributor Author

@davepagurek Kindly review this.

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 making this change, it feels great when playing around with it! The code works great, so my only minor suggestions are about code style/comments 🙂

camPhi += dPhi;
camRadius += dRadius;
// add change according to the direction of this.upY
let directionflag = this.upY > 0 ? 1 : -1;
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we can name this variable newUpY since that's what it will end up getting used for?

And then the comment about changing according to the direction of this.upY would maybe then be better placed at the if statement where we flip it, with a small explanation of why we have to flip it (something about orbiting to the other side and now being upside-down.)

let directionflag = this.upY > 0 ? 1 : -1;
camTheta += directionflag*dTheta;
camPhi += directionflag*dPhi;
if (camPhi <= 0 || camPhi>=Math.PI) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Super minor, but on this line and the ones above, can we add spaces around the operators so they match the style of the rest of the file?

@AryanKoundal
Copy link
Contributor Author

@davepagurek I made all the changes. Kindly review it now.

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.

Looks good, great work!

@davepagurek davepagurek merged commit 05d0910 into processing:main Feb 13, 2023
@AryanKoundal
Copy link
Contributor Author

Looks good, great work!

Thanks, wouldn't have been possible without your tips :)

@AryanKoundal AryanKoundal deleted the issue#5958 branch June 15, 2023 09:00
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.

3D orbitControl allowing +-90 degrees not enough.
2 participants