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

maxDistance is never set and defaults to 10 #20

Open
vorg opened this issue Jul 2, 2022 · 3 comments
Open

maxDistance is never set and defaults to 10 #20

vorg opened this issue Jul 2, 2022 · 3 comments

Comments

@vorg
Copy link
Member

vorg commented Jul 2, 2022

It got lost here and causes large gilt scenes to be zoomed in into the mesh.

6e2881c#diff-78aa8fa08464681459d97cd46fe608ab83a8944ec030fa9796ad452ddaa8d1cfL80

Screenshot 2022-07-02 at 01 23 41

@dmnsgn
Copy link
Member

dmnsgn commented Jul 2, 2022

True, I see its purpose. A few points that come to mind:

  • It is a convenience method and I assume / 10 and * 10 work in most case but is it just an arbitrary number?
  • It forces you to pass opts.min/maxDistance if you are setting a new camera on the orbiter
  • Overrides already set defaults on the orbiter object

I see 3 options:

  • Make it be a user space concern:
orbiter.set({
  maxDistance: vec3.add(aabb.size(scene.bounds), aabb.center(scene.bounds))
})
  • Add back previous behaviour and make a note on how it works

  • Change defaults to:

minDistance: Number.EPSILON,
maxDistance: Infinity,

Thoughts?

@vorg
Copy link
Member Author

vorg commented Jul 2, 2022

It's convenience to protect you from inertia scrolling on Mac but pain on bigger scenes like Expo.

It is a convenience method and I assume / 10 and * 10 work in most case but is it just an arbitrary number?

In most cases you set your camera outside of the scene if you use orbiter to rotate around it. 10x was good guesstimate to prevent it shrinking to 1x1 px if you zoom out too far.

Overrides already set defaults on the orbiter object

That's ok. Because if you know what you want you just provide maxDistance and otherwise (most cases) you don't have to do extra steps.

@vorg
Copy link
Member Author

vorg commented Jul 4, 2022

I think we should probably disable it by default, remove to automagic guessing on position set and let user set it manually if really needed.

minDistance: Number.EPSILON,
maxDistance: Infinity,

Cases where you zoom out too far should be handled on the user side by e.g. having reset camera button.

dmnsgn added a commit that referenced this issue Jul 4, 2022
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

No branches or pull requests

2 participants