-
-
Notifications
You must be signed in to change notification settings - Fork 719
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
Fix unwanted roll #5083 #5092
base: main
Are you sure you want to change the base?
Fix unwanted roll #5083 #5092
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5092 +/- ##
==========================================
- Coverage 89.06% 88.95% -0.11%
==========================================
Files 269 269
Lines 38386 38410 +24
Branches 2370 2426 +56
==========================================
- Hits 34189 34169 -20
- Misses 3188 3210 +22
- Partials 1009 1031 +22 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
src/ui/camera.ts
Outdated
@@ -746,6 +746,10 @@ export abstract class Camera extends Evented { | |||
return this; | |||
} | |||
|
|||
setRollEnabled(rollEnabled: boolean) { |
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 world become part of the public API, is this intended?
Do we have similar API methods like this?
What about symmetry of getRollEnabled?
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.
No, it is not intended that this become part of the public API. Is there a way to prevent that? Is adding @internal
sufficient?
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.
peotected should probably do it, and in terms of docs internal should do.
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.
OK, I've made setRollEnabled()
protected
and @internal
. Do you still want a getRollEnabled()
even though there is currently no use for one?
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.
protected
prevents the tests from running, so I'm going to have to do something else.
@@ -643,6 +643,7 @@ export class Map extends Camera { | |||
this._trackResize = resolvedOptions.trackResize === true; | |||
this._bearingSnap = resolvedOptions.bearingSnap; | |||
this._centerClampedToGround = resolvedOptions.centerClampedToGround; | |||
this.cameraHelper.setRollEnabled(resolvedOptions.rollEnabled === true); |
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.
Can someone set roll value while roll is disabled? How does these two interact?
Launch Checklist
Fix #5083.
Use spherical linear interpolation only when
rollEnabled
is true. Otherwise, use linear interpolation of Euler angles.CHANGELOG.md
under the## main
section.