Keep camera outside terrain #4300
Draft
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
HarelM
reviewed
Jun 19, 2024
| // this.transformCameraUpdate = resolvedOptions.transformCameraUpdate; | ||
| this.transformCameraUpdate = (tr: Transform) => { | ||
| try { | ||
| const buffer = Math.min(500, 20 * (25 - tr.zoom)); |
There was a problem hiding this comment.
Not sure it's clear what this buffer holds, a better name is probably a good idea here.
HarelM
reviewed
Jun 19, 2024
| @@ -589,7 +589,28 @@ export class Map extends Camera { | |||
| this._clickTolerance = resolvedOptions.clickTolerance; | |||
| this._overridePixelRatio = resolvedOptions.pixelRatio; | |||
| this._maxCanvasSize = resolvedOptions.maxCanvasSize; | |||
| this.transformCameraUpdate = resolvedOptions.transformCameraUpdate; | |||
| // this.transformCameraUpdate = resolvedOptions.transformCameraUpdate; | |||
| this.transformCameraUpdate = (tr: Transform) => { | |||
There was a problem hiding this comment.
I'm guessing this entire method should be part of terrain class? Not sure, but it shouldn't be here I think...
HarelM
reviewed
Jun 19, 2024
| try { | ||
| const buffer = Math.min(500, 20 * (25 - tr.zoom)); | ||
| const camera = tr.getCameraPosition(); | ||
| const minAllowedAltitude = this.terrain.getElevationForLngLatZoom(camera.lngLat, tr.zoom) + buffer; |
There was a problem hiding this comment.
Not sure I understand why this is the minimal altitude, can you add some diagram with some explanation on how you know if the camera is inside the terrain or not?
|
I think the approach presented here is a very elegant one. Thanks! This still needs some love and care, but otherwise a very good idea to solve this issue. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fix for #1542. Currently proof of concept, comments appreciated.
It uses the
transformCameraUpdatefunctionality to validate the next camera position, possibly adjusting it. By usingtransformCameraUpdate, the changes can be kept small. The assigned function checks if the next camera position will be inside terrain. If that would be the case, the camera is moved up (keeping its center), so that it is above the terrain. The pitch is adjusted to keep the map's center. IMHO this leads to a good user experience. Tried other options (stopping the camera or just adjusting height), but this seems best.Proof of concept: Needs some more testing.
transformCameraUpdatetype signature is changed to pass the whole transform. Might not be needed, did not look at this yet. If needed, an internal wrapper fortransformCameraUpdatemight be the solution.Includes a temporary workaround for #4233.
Launch Checklist
CHANGELOG.mdunder the## mainsection.