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

OrbitControls: support more flexible azimuthal limits #19298

Merged
merged 3 commits into from
Jun 24, 2020

Conversation

WestLangley
Copy link
Collaborator

@WestLangley WestLangley commented May 4, 2020

Continues #19266 and #19296.

Here is what I came up with last week... You guys can fight it out. :-)

Known limitation: If the azimuthal gap is very narrow, it can "jump across the gap" and continue to rotate, particularly with larger values of the damping factor. Not worth fixing, IMO.

Full disclosure: this was based on the work of @sciecode.

@munrocket
Copy link
Contributor

I like that we don't do anything, when we don't have limits. Because it's the most popular usage.

if ( isFinite ( min ) && isFinite( max ) ) {

This branch will be never reached if we will say that one angle need to be in range [-pi, pi].

;else if ( min > Math.PI ) min -= twoPI;

WestLangley stole a commit from ScieCode without references :D

@sciecode
Copy link
Contributor

sciecode commented May 4, 2020

I'm fine with any of the proposed PRs.

But to be honest, we are only struggling to find a good solution to this simple problem, because we are having to fight against a really bad API. The current API forces us to make a bunch of unnecessary checks and data-manipulation inside update, because we allow the user to directly alter the values. And, IMO, it's still non-intuitive for the users to reason about it.

That's really all I have to say about this.

@munrocket
Copy link
Contributor

I like your approach, just need to update documentation.

Also good enchantment is vertical scroll since OrbitControls also MapControls.

@DefinitelyMaybe
Copy link
Contributor

noted.

@WestLangley
Copy link
Collaborator Author

if we will say that one angle need to be in range [-pi, pi].

Actually, I did not say that... From the in-line comments in this PR:

// If set, the interval [ min, max ] must be a sub-interval of [ - 2 PI, 2 PI ], with ( max - min < 2 PI )

Although not explicitly stated, min is always less than max -- as one would typically assume.

This is backwards-compatible, and IMO, it is a perfectly-fine API. But, we can use a different API if that is preferred.

@WestLangley WestLangley added this to the r118 milestone Jun 13, 2020
@WestLangley
Copy link
Collaborator Author

@sciecode It seems this will only be merged if you are OK with it.

@sciecode
Copy link
Contributor

sciecode commented Jun 18, 2020

As the hip kids say, ship it.

@mrdoob mrdoob merged commit a811352 into mrdoob:dev Jun 24, 2020
@mrdoob
Copy link
Owner

mrdoob commented Jun 24, 2020

Thanks!

@WestLangley WestLangley deleted the dev_azimuth_limits branch June 24, 2020 17:14
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.

None yet

5 participants