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

feature: documentation update #223

Merged
merged 7 commits into from
Sep 29, 2021
Merged

feature: documentation update #223

merged 7 commits into from
Sep 29, 2021

Conversation

klich3
Copy link

@klich3 klich3 commented Sep 27, 2021

No description provided.

@klich3
Copy link
Author

klich3 commented Sep 27, 2021

documentation update related with issue #215 and others one...

@yomotsu
Copy link
Owner

yomotsu commented Sep 27, 2021

Thank you so much for the update. Really helpful😊

readme.md Outdated
Comment on lines 179 to 184
```js
const azi = 0;
cameraController.addEventListener("control", (_event) => {
azi = _event.target.azimuthAngle % (360 * THREE.MathUtils.DEG2RAD);
}, false);
```
Copy link
Owner

Choose a reason for hiding this comment

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

What does this code example for?

Copy link
Author

Choose a reason for hiding this comment

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

when you rotate to the side .azimuthAngle is summing
0 -> 360º -> 720º -> 1080º + 45º = 1125º
now if you use rotateTo() camera spinning back all 4x right? okey if you use this code
.azimuthAngle = .azimuthAngle % (360 * THREE.MathUtils.DEG2RAD) you are reset 4x turns to 0 + 45º
now if you execute rotateTo() camera rotate only -45º to 0

Copy link
Owner

Choose a reason for hiding this comment

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

That's right.
However, I think, the code example doesn't solve the situation, because azi is just a variable (also it should not be a const?).
Maybe the example could be omitted at this time?

Copy link
Author

Choose a reason for hiding this comment

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

Hmm yes you right, ok no problem I change it, omitting this sample

Copy link
Author

Choose a reason for hiding this comment

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

maybe can add new parameter with this function .azimuthAbsoluteAngle what you thing about it?

Copy link
Owner

@yomotsu yomotsu Sep 29, 2021

Choose a reason for hiding this comment

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

Thanks for understanding.

maybe can add new parameter

Thanks for your suggestion.
I think It can be done easily with THREE.MathUtils.euclideanModulo.
Maybe it should be in user-land rather than a part of camera-controls?

e.g.

THREE.MathUtils.euclideanModulo( -2890, 360 ); // 350

Copy link
Author

Choose a reason for hiding this comment

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

I think people do not know the existence of many of the functions, which integrates in Three for that reason to be confused when using the plugin. Or they just do not understand the hierarchy of elements. You should put plugin for user Mid / Pro Level XD

Copy link
Owner

@yomotsu yomotsu Sep 29, 2021

Choose a reason for hiding this comment

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

Thanks for considering the users😊

Although we already have cameraControls.normalizeRotations() method to fix, We could add an explanation of how to normalize the accumulated angle on the user's side with THREE.MathUtils.euclideanModulo.
Maybe like the below:

If you need a normalized accumulated azimuth angle (between 0 and 360 deg), compute with THREE.MathUtils.euclideanModulo.
e.g.

const normalizedAzimuthAngle = THREE.MathUtils.euclideanModulo( cameraControls.azimuthAngle, 360 * THREE.MathUtils.DEG2RAD );

However, regarding adding the helper on the library side, That must be done with another github issue or PR.

Copy link
Author

Choose a reason for hiding this comment

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

You are welcome :)

@yomotsu
Copy link
Owner

yomotsu commented Sep 29, 2021

@klich3
Thanks for adjusting to my comments.
I think it's almost okay to merge.
If you are ready, let me know.

(Let me edit a little bit after merged)

@klich3
Copy link
Author

klich3 commented Sep 29, 2021

Yes, of course, enchanted in contributing :)
Im ready

@yomotsu yomotsu merged commit 3df7b94 into yomotsu:dev Sep 29, 2021
@yomotsu
Copy link
Owner

yomotsu commented Sep 29, 2021

Thanks! merged!

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.

2 participants