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

Sky Shader Example Clean Up #7143

Merged
merged 1 commit into from
Sep 10, 2015
Merged

Sky Shader Example Clean Up #7143

merged 1 commit into from
Sep 10, 2015

Conversation

WestLangley
Copy link
Collaborator

mrdoobapproves formatting, switched to OrbitControls, removed animation loop.

@mrdoob
Copy link
Owner

mrdoob commented Sep 10, 2015

Nice!

mrdoob added a commit that referenced this pull request Sep 10, 2015
@mrdoob mrdoob merged commit cd4d0d7 into mrdoob:dev Sep 10, 2015
@mrdoob
Copy link
Owner

mrdoob commented Sep 10, 2015

Thanks!

What do you think about adding requestAnimationFrame inside the controls directly so the user doesn't have to worry about calling .update()?

@WestLangley
Copy link
Collaborator Author

It would be nice if we could improve the API. Users almost always get this wrong -- especially since TrackballControls is different. Here is a summary.

If there is no animation loop, then only this line is required.

controls.addEventListener( 'change', render ); // add this only if there is no animation loop

If there is an animation loop, and controls damping or auto-rotation is desired, then only this line is required.

controls.update(); // required if controls.enableDamping = true, or if controls.autoRotate = true

Otherwise, neither pattern is required.

@mrdoob
Copy link
Owner

mrdoob commented Sep 10, 2015

Yeah... That's too much to know I think... If we added rAF to all the controls there would only be two cases:

  1. There is an animation loop. The user doesn't need to do anything.
  2. There is no animation loop. The user will need to add controls.addEventListener( 'change', render );

@WestLangley
Copy link
Collaborator Author

And update() would no longer need to be exposed.

@mrdoob
Copy link
Owner

mrdoob commented Sep 10, 2015

Yep!

@WestLangley WestLangley deleted the dev-sky branch September 10, 2015 20:11
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