-
-
Notifications
You must be signed in to change notification settings - Fork 724
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/remaining issues camera inside terrain #4551
base: main
Are you sure you want to change the base?
Changes from all commits
6ce770b
2efeea5
d569899
fe6191c
db287d1
b5efbf6
9950b94
3484b91
1d83c6e
c2825a0
103794d
1ee53ee
cba97c1
a5dc76d
cd88615
9940ea5
0f0d4de
a177ba4
e412a62
dfeb449
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1205,7 +1205,9 @@ export abstract class Camera extends Evented { | |
_finalizeElevation() { | ||
this._elevationFreeze = false; | ||
if (this.getCenterClampedToGround()) { | ||
this.transform.recalculateZoomAndCenter(this.terrain); | ||
const tr = this._getTransformForUpdate(); | ||
tr.recalculateZoomAndCenter(this.terrain); | ||
this._applyUpdatedTransform(tr); | ||
} | ||
} | ||
|
||
|
@@ -1238,13 +1240,14 @@ export abstract class Camera extends Evented { | |
* | ||
* @param tr - The transform to check. | ||
*/ | ||
_elevateCameraIfInsideTerrain(tr: ITransform) : { pitch?: number; zoom?: number } { | ||
_elevateCameraIfInsideTerrain(tr: ITransform): { pitch?: number; zoom?: number } { | ||
if (!this.terrain && tr.elevation >= 0 && tr.pitch <= 90) { | ||
return {}; | ||
} | ||
const surfacePadding = Math.min(500, 20 * (25 - tr.zoom)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we need this surface padding? At a typical zoom of 15, this sets the minimum altitude at 200m. This amount of surface padding is problematic for ground-level use cases (like an aircraft on the runway, or touring a city at eye-level.) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It can help prevent graphic glitches when the camera is directly moving on the surface. Do these use cases have terrain enabled? If so, one could make the padding optional. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The linear scaling with zoom level doesn't make sense to me. I think it would be more appropriate to have something like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't have strong arguments for the current calculation, but why do you think non-linear is better? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because the distance from camera to center point (in meters) is inversely proportional to If you increase zoom by 1, you are cutting the distance from camera to center point in half. So it seems like you should also cut the padding in half. |
||
const cameraLngLat = tr.getCameraLngLat(); | ||
const cameraAltitude = tr.getCameraAltitude(); | ||
const minAltitude = this.terrain ? this.terrain.getElevationForLngLatZoom(cameraLngLat, tr.zoom) : 0; | ||
const minAltitude = this.terrain ? this.terrain.getElevationForLngLatZoom(cameraLngLat, tr.zoom) + surfacePadding : 0; | ||
if (cameraAltitude < minAltitude) { | ||
const newCamera = this.calculateCameraOptionsFromTo( | ||
cameraLngLat, minAltitude, tr.center, tr.elevation); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think any of the "expected" images for these render tests should change. The fact that they are changing means that the pitch angle is being adjusted, and I don't think it should be in any of these cases. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's shifted because the padding is added which results in a changed angle (the center stays the same). |
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The "expected" image shows this bug: #4859 |
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 should be
recalculateZoomAndCenter()
now