-
-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
Improve orbitControl() to make it smoother #6140
Improve orbitControl() to make it smoother #6140
Conversation
Let material and texture coexist.
Prepare velocity variables in the renderer. These are used in orbitControl() to smooth things out.
By introducing damping to orbitControl(), the processing is smoothed and more comfortable operation is realized. This idea was inspired by p5.EasyCam.
Variable "mouseZoomScaleFactor" is too long, so shorten process to pass linter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is feeling really good so far, and definitely feels more polished/professional! I left a few little notes, let me know what you think!
src/webgl/interaction.js
Outdated
this._renderer._curCamera._orbit(0, 0, deltaRadius); | ||
|
||
// zoom process | ||
if (accelerateZoomVelocity) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we initialize deltaRadius
to 0, maybe we could simplify this section by removing the need for an accelerateZoomVelocity
flag? And maybe similarly for the rotation/move flags
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If initialize it to 0 and only process it if it's not 0 then probably don't need the flag... ok, I'll think about it!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that the flag has been eliminated, let's rewrite the code.
src/webgl/interaction.js
Outdated
let executeRotateProcess = false; | ||
let executeMoveProcess = false; | ||
// The idea of using damping is based on the following website. thank you. | ||
// https://github.com/freshfork/p5.EasyCam/blob/master/p5.easycam.js |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might want to consider using a link to a commit hash (and maybe a relevant line number?) in case the easycam repo changes any branch or file names. Maybe this? https://github.com/freshfork/p5.EasyCam/blob/9782964680f6a5c4c9bee825c475d9f2021d5134/p5.easycam.js#L1124
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Certainly, it would be easier to understand if put a link directly to the place. I got it!
In the case of comments, is it okay if the number of characters is large?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems to be fine!
src/webgl/interaction.js
Outdated
// For touches, the appropriate scale is different | ||
// because the distance difference is multiplied. | ||
const mouseZoomScaleFactor = 0.02; | ||
const touchZoomScaleFactor = 0.0008; | ||
const mouseZoomScaleFactor = 0.00008; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think my impression of zooming using my laptop trackpad is that it feels just a little bit slow to start, how would you feel about increasing this a little bit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made it a little bigger.
const mouseZoomScaleFactor = 0.0001;
const touchZoomScaleFactor = 0.0004;
how is it? If possible, I would like you to rewrite the code and try it...
Here is the rewritten code. newOrbitControl
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, the reason I messed with the touch was because I was trying to change these at the same time.
If it's better not to change the touch, I'm going to try not to change it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that's an improvement, thanks!
I've rewritten the link address to show exactly where the referenced idea is.
If you initialize the variables used to move the camera to 0, you don't need to make judgments with flags.
src/webgl/interaction.js
Outdated
@@ -175,43 +177,81 @@ p5.prototype.orbitControl = function( | |||
// if mouseLeftButton is down, rotate | |||
// if mouseRightButton is down, move | |||
if (this._mouseWheelDeltaY !== 0) { | |||
executeZoomProcess = true; | |||
// zoom according to direction of mouseWheelDeltaY rather than value. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh one more tiny thing, is this comment still accurate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, the original comment is still there... not good.
For example, "zoom the camera depending on the value of _mouseWheelDeltaY. Move away if positive, move closer if negative" ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, I would like to add a comment to the zoom section for touch.
Increased some constants for better usability. Part of the explanation about zooming was inappropriate, so I rewrote it. Also, added a comment to the zoom section for touch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for making these changes, everything looks good!
Thank you very much for all! ('ω') |
Introduce the idea of using p5.EasyCam's damping to make the behavior of orbitControl() smoother.
Resolves #6139
Changes:
When you set up your renderer, you provide velocity variables to handle each of the scaling, rotation, and translation.
Interactions accelerate these velocity variables.
The velocity variable is moderately damped every frame.
You can smooth out the behavior by performing an update on these velocity variables.
Demo
Demo is here:newOrbitControl
PR Checklist
npm run lint
passes